Vojtech Szocs has posted comments on this change.
Change subject: core: i18n splash screen.
......................................................................
Patch Set 5: (9 inline comments)
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/LocaleFilter.java
Line 24: * <li>If no cookie, check the accept headers of the request</li>
Line 25: * <li>If no headers, then default to the US locale</li>
Line 26: * </ol>
Line 27: */
Line 28: @WebFilter(filterName = "LocaleFilter")
Minor thing: I'd rather not use @WebFilter here but define filter (and any
potential init-params) directly in web.xml via <filter> element. Alternatively,
you can define everything (name, pattern, etc.) inside @WebFilter annotation,
without modifying web.xml.
IMHO Servlet 3.0 annotations are nice, but shouldn't be used in a way that
something is defined in annotation and something else in web.xml. That being
said, I typically prefer having mapping/configuration represented in web.xml
rather than in Java source code.
Line 29: public class LocaleFilter implements Filter {
Line 30: /**
Line 31: * The logger.
Line 32: */
Line 58: private Locale determineLocale(final HttpServletRequest request) {
Line 59: //Step 1.
Line 60: Locale locale =
LocaleUtils.getLocaleFromString(request.getParameter(LOCALE));
Line 61: //Step 2.
Line 62: if (null == locale) { //No locale parameter.
Minor thing: please use "something <operator> null" convention.
Line 63: locale = getLocaleFromCookies(request.getCookies());
Line 64: }
Line 65: //Step 3.
Line 66: if (null == locale) { //No selected locale in cookies.
Line 62: if (null == locale) { //No locale parameter.
Line 63: locale = getLocaleFromCookies(request.getCookies());
Line 64: }
Line 65: //Step 3.
Line 66: if (null == locale) { //No selected locale in cookies.
Minor thing: please use "something <operator> null" convention.
Line 67: locale = request.getLocale();
Line 68: }
Line 69: //Step 4.
Line 70: if (null == locale) { //No accept headers.
Line 66: if (null == locale) { //No selected locale in cookies.
Line 67: locale = request.getLocale();
Line 68: }
Line 69: //Step 4.
Line 70: if (null == locale) { //No accept headers.
Minor thing: please use "something <operator> null" convention.
Line 71: locale = Locale.US;
Line 72: }
Line 73: log.debug("Filter determined locale to be: " +
locale.toLanguageTag());
Line 74: return locale;
Line 80: * @return The {@code Locale} if a cookie is found, null otherwise.
Line 81: */
Line 82: private Locale getLocaleFromCookies(final Cookie[] cookies) {
Line 83: Locale locale = null;
Line 84: if (null != cookies) {
Minor thing: please use "something <operator> null" convention.
Line 85: for (Cookie cookie: cookies) {
Line 86: if (LOCALE.equalsIgnoreCase(cookie.getName())) {
Line 87: locale =
LocaleUtils.getLocaleFromString(cookie.getValue());
Line 88: break;
....................................................
File
backend/manager/modules/root/src/main/java/org/ovirt/engine/core/SplashServlet.java
Line 1: /**
Is this package-level Javadoc really necessary?
Line 2: *
Line 3: */
Line 4: package org.ovirt.engine.core;
Line 5:
Line 19:
Line 20: import org.apache.log4j.Logger;
Line 21:
Line 22: /**
Line 23: * @author awels
Please include some brief Javadoc about the purpose of this servlet.
Line 24: *
Line 25: */
Line 26: @WebServlet(value="/index")
Line 27: public class SplashServlet extends HttpServlet {
Line 22: /**
Line 23: * @author awels
Line 24: *
Line 25: */
Line 26: @WebServlet(value="/index")
Even though I'd personally prefer having mapping/configuration in web.xml, it's
OK to do it via Servlet 3.0 annotations as well.
Line 27: public class SplashServlet extends HttpServlet {
Line 28: private static final Logger log =
Logger.getLogger(SplashServlet.class);
Line 29:
Line 30: private static final long serialVersionUID = 8289914264581273721L;
Line 47:
Line 48: private void setCookie(HttpServletResponse response, Locale
userLocale) {
Line 49: //Detected locale doesn't match the default locale, set a
cookie.
Line 50: Cookie cookie = new Cookie(LocaleFilter.LOCALE,
userLocale.toString());
Line 51: cookie.setSecure(false); //Doesn't have to be secure.
Is this really necessary? (as far as I can tell, Cookie "secure" field defaults
to false)
Line 52: cookie.setMaxAge(Integer.MAX_VALUE); //Doesn't expire.
Line 53: response.addCookie(cookie);
Line 54: }
Line 55:
--
To view, visit http://gerrit.ovirt.org/12695
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I37156061cbdd7d2df3290c88dee933c41e0087c6
Gerrit-PatchSet: 5
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: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
