Vojtech Szocs has posted comments on this change.
Change subject: engine: root.war cleanup
......................................................................
Patch Set 13:
(12 comments)
Quite a big patch, there are some very nice ideas, comments inline.
....................................................
File
backend/manager/modules/branding/src/main/java/org/ovirt/engine/core/branding/BrandingFilter.java
Line 61: log.warn("Detected Locale is null, setting to default: " +
LocaleFilter.DEFAULT_LOCALE);
Line 62: userLocale = LocaleFilter.DEFAULT_LOCALE;
Line 63: }
Line 64: List<BrandingTheme> brandingThemes =
brandingManager.getBrandingThemes(
Line 65:
request.getServletContext().getInitParameter(APPLICATION_TYPE));
+1 for controlling applicationType via servlet context param per application,
makes much more sense than providing such configuration in source code.
Line 66: request.setAttribute(THEMES_KEY, brandingThemes);
Line 67: // Load the appropriate resource bundle with the right locale.
Line 68: ResourceBundle bundle =
ResourceBundle.getBundle(BRANDING_BASE_NAME, userLocale,
Line 69: BrandingResourceBundle.getBrandingControl(BLANK));
....................................................
File
backend/manager/modules/branding/src/main/java/org/ovirt/engine/core/branding/BrandingManager.java
Line 127: * @param type a string naming the application type to load the
theme for.
Line 128: * @return A {@code List} of {@code BrandingTheme}s.
Line 129: */
Line 130: public synchronized List<BrandingTheme> getBrandingThemes(final
String type) {
Line 131: this.applicationType = type;
This code assumes getBrandingThemes will be called first and other methods
(getMessageMap, getWelcomeSections, getCascadingResource) will reuse
applicationType that was set previously. In other words, this method has
unexpected side effect.
Anyway, BrandingManager is a singleton by definition and shouldn't store
caller-specific context such as applicationType. Sure, each webapp could have
its "own" BrandingManager instance due to how classloader hierarchy works,
however I wouldn't rely on this. (Singleton in sense of one instance for entire
app server vs. one instance for web app classloader.)
Due to reasons mentioned above, please consider removing applicationType field
and introducing applicationType argument into all methods requiring it.
Line 132: if (themes == null && brandingRootPath.exists() &&
brandingRootPath.isDirectory()
Line 133: && brandingRootPath.canRead()) {
Line 134: themes = new ArrayList<BrandingTheme>();
Line 135: List<File> directories = Arrays.asList(
....................................................
File
backend/manager/modules/branding/src/main/java/org/ovirt/engine/core/branding/BrandingTheme.java
Line 74:
Line 75: /**
Line 76: * Post fix for denoting css files
Line 77: */
Line 78: private static final String CSS_POST_FIX = "_css";
+1 good convention is always better than (explicit & mandatory) configuration.
Line 79:
Line 80: /**
Line 81: * The properties associated with the branding theme.
Line 82: */
Line 170: * Get the style sheet type based on the passed in {@code
ApplicationType}.
Line 171: * @param type The application type to get the style sheet string
for.
Line 172: * @return A {@code String} representation of the style sheet
name.
Line 173: */
Line 174: public String getThemeStyleSheet(String applicationType) {
Why do we need applicationType argument here? Shouldn't we use
"this.applicationType" here instead?
(I don't see any usage of "this.applicationType" aside from constructor.)
Line 175: return brandingProperties.getProperty(applicationType +
CSS_POST_FIX);
Line 176: }
Line 177:
Line 178: /**
Line 331: return null;
Line 332: }
Line 333:
Line 334: @Override
Line 335: public String toString() {
Shouldn't toString use "this.applicationType" instead? i.e. something like this:
return "Path to theme: " + getPath() + ", Application css: " +
getThemeStyleSheet(this.applicationType);
(Also see my question above about "applicationType" argument in
getThemeStyleSheet method.)
Line 336: return "Path to theme: " + getPath() + ", User portal css: "
//$NON-NLS-1$ //$NON-NLS-2$
Line 337: + getThemeStyleSheet("userportal") + ", Web admin css: "
//$NON-NLS-1$ //$NON-NLS-2$
Line 338: + getThemeStyleSheet("webadmin") + ", Welcome page css: "
//$NON-NLS-1$ //$NON-NLS-2$
Line 339: + getThemeStyleSheet("welcome"); //$NON-NLS-1$
....................................................
File backend/manager/modules/branding/src/main/resources/META-INF/obrand.tld
Line 1: <?xml version="1.0" encoding="UTF-8" ?>
+1 for reusing common JSP snippets as custom tags.
Line 2: <taglib xmlns="http://java.sun.com/xml/ns/javaee";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
xsi:schemaLocation="http://java.sun.com/xml/ns/javaee
http://java.sun.com/xml/ns/javaee/web-jsptaglibrary_2_1.xsd"; version="2.1">
Line 3: <tlib-version>1.0</tlib-version>
Line 4: <short-name>obrand</short-name>
Line 5: <uri>obrand</uri>
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/DocsServlet.java
Line 63: if (!languagePageShown) {
Line 64: setLangPageShown(response, true);
Line 65: request.setAttribute(LocaleFilter.LOCALE, locale);
Line 66: request.setAttribute(ENGLISH_HREF, redirect);
Line 67: final ServletContext forwardContext =
getServletContext().getContext(localeDocsMissing);
forwardContext can be null, you should check it.
Line 68: final RequestDispatcher dispatcher =
forwardContext.getRequestDispatcher(localeDocsMissing);
Line 69: dispatcher.forward(request, response);
Line 70: } else {
Line 71: //Redirect to English version of the document
Line 64: setLangPageShown(response, true);
Line 65: request.setAttribute(LocaleFilter.LOCALE, locale);
Line 66: request.setAttribute(ENGLISH_HREF, redirect);
Line 67: final ServletContext forwardContext =
getServletContext().getContext(localeDocsMissing);
Line 68: final RequestDispatcher dispatcher =
forwardContext.getRequestDispatcher(localeDocsMissing);
dispatcher can be null, you should check it.
Line 69: dispatcher.forward(request, response);
Line 70: } else {
Line 71: //Redirect to English version of the document
Line 72: response.sendRedirect(redirect);
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/WelcomeServlet.java
Line 23
Line 24
Line 25
Line 26
Line 27
+1 IMHO it's always better to configure servlet mappings in webapp descriptor
(i.e. deployment configuration file) rather than in source code.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/servlet/PageNotFoundForwardServlet.java
Line 69: @Override
Line 70: protected void doGet(final HttpServletRequest request, final
HttpServletResponse response) throws IOException,
Line 71: ServletException {
Line 72: //Forward to the page not found URI of the target context,
which should have all the proper branding/messages.
Line 73: final ServletContext forwardContext =
getServletContext().getContext(targetContext);
forwardContext can be null, shouldn't we check it?
Line 74: final RequestDispatcher dispatcher =
forwardContext.getRequestDispatcher(pageNotFoundURI);
Line 75: dispatcher.forward(request, response);
Line 76: }
Line 77:
Line 70: protected void doGet(final HttpServletRequest request, final
HttpServletResponse response) throws IOException,
Line 71: ServletException {
Line 72: //Forward to the page not found URI of the target context,
which should have all the proper branding/messages.
Line 73: final ServletContext forwardContext =
getServletContext().getContext(targetContext);
Line 74: final RequestDispatcher dispatcher =
forwardContext.getRequestDispatcher(pageNotFoundURI);
dispatcher can be null, shouldn't we check it?
Line 75: dispatcher.forward(request, response);
Line 76: }
Line 77:
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/GwtDynamicHostPageServlet.java
Line 70: public static final String ETAG_HEADER = "Etag"; //$NON-NLS-1$
Line 71:
Line 72: private static final String HOST_JSP = "/GwtHostPage.jsp";
//$NON-NLS-1$
Line 73: private static final String UTF_CONTENT_TYPE = "text/html;
charset=UTF-8"; //$NON-NLS-1$
Line 74: private static final String APPLICATION_TYPE = "applicationType";
//$NON-NLS-1$
Instead of defining applicationType servlet context param per each application
& having each servlet to retrieve this param, why not just create custom
ServletContextListener within the "branding" module?
ServletContextListener.contextInitialized implementation could simply retrieve
current context and extract applicationType param value from it. This is
similar to web frameworks like SpringMVC where you control framework behavior
by specifying custom servlet context params which are then picked up by some
ServletContextListener.
Line 75:
Line 76: private BackendLocal backend;
Line 77:
Line 78: private ObjectMapper mapper;
--
To view, visit http://gerrit.ovirt.org/19848
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iceae0ebf671efc951522c464db1a5b2b95dd5637
Gerrit-PatchSet: 13
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@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
