Vojtech Szocs has posted comments on this change.
Change subject: engine: root.war cleanup
......................................................................
Patch Set 13:
(3 comments)
....................................................
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);
Hm, you're right, if these things are null then there's a bigger problem and it
will blow up on internal server error anyway.
I see that you've added some log.error + response.sendError code to handle both
null cases, I'm fine with that.
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
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/WelcomeServlet.java
Line 23
Line 24
Line 25
Line 26
Line 27
True, it might be a matter of taste, but it makes (in my opinion) much more
sense to control deployment configuration (like servlet mapping) in dedicated
file (like web.xml) rather than in source code.
In a hypothetical situation where you have a webapp bundled as WAR or EAR and
you'd like to tweak deployment configuration depending on specific environment,
it's always more easy and practical to edit XML file than to recompile webapp
from source. This is essentially the reason behind my preference, I agree that
there's no good/bad rule here, in the end, everything depends on our
expectations, requirements and conventions.
....................................................
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$
Yup, BrandingFilter setting "applicationType" request attribute from servlet
context param sounds good to me.
You could even read the servlet context param as part of filter init method
since we don't expect the value to change at runtime, but it's no big deal,
current solution is fine for me.
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
