Alexander Wels has posted comments on this change.
Change subject: userportal, webadmin: added favicon.ico to branding
......................................................................
Patch Set 1:
(4 comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/branding/BrandingManager.java
Line 270: * @return path to favicon to serve, or null if no favicons exist
Line 271: */
Line 272: public String getBrandedFaviconPath() {
Line 273: List<BrandingTheme> brandingThemes = getBrandingThemes(); //
assume these are sorted 00, 01, 02, etc.
Line 274: for (int i = brandingThemes.size() - 1; i >= 0; i--) {
If you reverse the list first, then you can do
List<BrandingTheme> brandingThemes = getBrandingThemes();
String faviconPath = null;
Collections.reverse(brandingThemes);
for(BrandingTheme theme: brandingThemes) {
String faviconPath = theme.getFaviconPath();
if (faviconPath != null) {
break;
}
}
return faviconPath;
It is slightly cleaner to read.
Line 275: String faviconPath =
brandingThemes.get(i).getFaviconPath();
Line 276: if (faviconPath != null) {
Line 277: return faviconPath;
Line 278: }
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/branding/BrandingServlet.java
Line 44: public void doGet(final HttpServletRequest request,
Line 45: final HttpServletResponse response) throws IOException,
Line 46: ServletException {
Line 47:
Line 48: if (request.getRequestURI().equals("/favicon.ico")) {
Alon,
I think you misunderstand this a little bit. This is a 'magic' path, but only
because browsers always look for /favicon.ico when requesting the little
browser icon for the site. We have to do some different work to determine the
correct favicon based on the available themes instead of just returning the
requested file. That is what this particular check is for.
Also note we will have to think about what happens if we have Apache in front
of us. Since I am not sure if right now Apache is serving the favicon or not.
Line 49: String faviconPath =
brandingManager.getBrandedFaviconPath();
Line 50: if (faviconPath == null) {
Line 51: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 52: }
Line 49: String faviconPath =
brandingManager.getBrandedFaviconPath();
Line 50: if (faviconPath == null) {
Line 51: response.sendError(HttpServletResponse.SC_NOT_FOUND);
Line 52: }
Line 53: else {
I would write this code slightly differently so we don't have two almost
identical pieces of code here:
String requestPath = request.getPathInfo();
if ("/favicon.ico".equals(request.getRequestURI()) {
requestPath = brandingManager.getBrandedFaviconPath();
if (requestPath == null) {
response.sendError(HttpServletResponse.SC_NOT_FOUND);
return;
}
}
ServletUtils.sendFile(request, response,
getFile(brandingManager.getBrandingRootPath(), requestPath), null);
Line 54: ServletUtils.sendFile(request, response,
Line 55: getFile(brandingManager.getBrandingRootPath(),
faviconPath), null);
Line 56: }
Line 57: }
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/branding/BrandingTheme.java
Line 288: * @return favicon path, or null if no favicon in this theme
Line 289: */
Line 290: public String getFaviconPath() {
Line 291: File favicon = new File(filePath + "/favicon.ico");
Line 292: if (favicon.exists()) {
I would also check if the file is readable as well as existing.
Line 293: return path + "/favicon.ico";
Line 294: }
Line 295: return null;
Line 296: }
--
To view, visit http://gerrit.ovirt.org/18385
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1dcd9d0f22a7c5867c39576020ad16dd6c53deda
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
