Vojtech Szocs has posted comments on this change.
Change subject: userportal,webadmin: brand 404 error pages.
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
There are two things we should consider to improve this patch:
(1) duplicate code between different servlet classes:
* GwtDynamicHostPageServlet + WelcomeServlet + PageNotFoundServlet all set
common request attributes: brandingStyle, applicationType
* WelcomeServlet + PageNotFoundServlet both do "ResourceBundle bundle =
ResourceBundle.getBundle(...)" + "Config.set(...)"
* PageNotFoundServlet checks "request.getAttribute(LocaleFilter.LOCALE) !=
null" with LocaleFilter.DEFAULT_LOCALE fallback, but WelcomeServlet assumes
it's always non-null
To avoid code duplication and ease the maintenance, consider creating common
base class, i.e. AbstractBrandedServlet, and make above mentioned servlets
extend this base class. AbstractBrandedServlet can set up branding-specific
request attributes, configure resource bundle, etc. so actual servlet code can
be reduced to this (WelcomeServlet example):
// AbstractBrandedServlet takes care of setting corresponding request attribute
@Override protected BrandingTheme.ApplicationType getApplicationType() {
return BrandingTheme.ApplicationType.WELCOME; }
@Override protected void doGet(HttpServletRequest request, HttpServletResponse
response) {
super.doGet(request, response); // AbstractBrandedServlet common logic
request.setAttribute(LOCALE_KEYS, ...); // custom request attribute
request.setAttribute(VERSION, ...); // custom request attribute
request.setAttribute(SECTIONS, ...); // custom request attribute
response.setContentType("text/html;charset=UTF-8");
RequestDispatcher dispatcher =
request.getRequestDispatcher("/WEB-INF/ovirt-engine.jsp");
if (dispatcher != null) {
dispatcher.include(request, response);
}
}
(2) same 404.jsp for WebAdmin and UserPortal:
We should have a single 404.jsp in "frontend" module (which acts as Servlet 3.0
web-fragment JAR) like this:
.../frontend/src/main/resources/META-INF/resources/404.jsp
Associated servlet (PageNotFoundServlet) should be moved to "frontend" module
as well, updating web-fragment.xml just like for any other common servlet used
by WebAdmin and/or UserPortal.
Root WAR's 404.jsp vs. Frontend's 404.jsp are almost identical, for now I'd
keep them separated in order not to complicate things too much.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/PageNotFoundServlet.java
Line 1: /**
Line 2: *
Line 3: */
Please remove empty package Javadoc.
Line 4: package org.ovirt.engine.core.utils.servlet;
Line 5:
Line 6: import java.io.IOException;
Line 7: import java.util.List;
--
To view, visit http://gerrit.ovirt.org/18196
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b0406ab6dea4a2cacebd4ad6e62c8bb7a0d44c0
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: Alexander Wels <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Greg Sheremeta <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
