Gilad Chaplik has posted comments on this change.
Change subject: userportal: Added caching to user portal page webadmin: Added
caching to webadmin page
......................................................................
Patch Set 1: (11 inline comments)
....................................................
File
frontend/webadmin/modules/frontend-overlay/src/main/java/org/ovirt/engine/ui/frontend/server/gwt/CachingFilter.java
Line 53: public void doFilter(ServletRequest request, ServletResponse
response,
Line 54: FilterChain chain) throws IOException, ServletException {
Line 55: // Cast to HttpServletRequest/Response.
Line 56: final HttpServletRequest httpRequest = (HttpServletRequest)
request;
Line 57: final HttpServletResponse httpResponse = (HttpServletResponse)
response;
please consider instanceOf check?
Line 58:
Line 59: if (cacheFilterPatternMathes(httpRequest)) {
Line 60: HttpServletResponseWrapper responseWrapper =
getCacheHeaderResponseWrapper(httpResponse);
Line 61: httpResponse.setHeader(CACHE_CONTROL, CACHE_YEAR);
Line 58:
Line 59: if (cacheFilterPatternMathes(httpRequest)) {
Line 60: HttpServletResponseWrapper responseWrapper =
getCacheHeaderResponseWrapper(httpResponse);
Line 61: httpResponse.setHeader(CACHE_CONTROL, CACHE_YEAR);
Line 62: httpResponse.setHeader(EXPIRES, getNowPlusYearHttpDate());
consider using NOW+[1 year of milliseconds]
Line 63: chain.doFilter(request, responseWrapper);
Line 64: } else if (noCacheFilterPatternMatches(httpRequest)) {
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 59: if (cacheFilterPatternMathes(httpRequest)) {
Line 60: HttpServletResponseWrapper responseWrapper =
getCacheHeaderResponseWrapper(httpResponse);
Line 61: httpResponse.setHeader(CACHE_CONTROL, CACHE_YEAR);
Line 62: httpResponse.setHeader(EXPIRES, getNowPlusYearHttpDate());
Line 63: chain.doFilter(request, responseWrapper);
you pass responseWrapper to the next chain, so why are you setting the header
of httpResponse parameter?
Line 64: } else if (noCacheFilterPatternMatches(httpRequest)) {
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 67: httpResponse.setHeader(EXPIRES, getYesterdayHttpDate());
Line 63: chain.doFilter(request, responseWrapper);
Line 64: } else if (noCacheFilterPatternMatches(httpRequest)) {
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 67: httpResponse.setHeader(EXPIRES, getYesterdayHttpDate());
getYesterday... () == 0?
Line 68: } else {
Line 69: chain.doFilter(request, response);
Line 70: }
Line 71: }
Line 65: chain.doFilter(request, response);
Line 66: httpResponse.setHeader(CACHE_CONTROL, NO_CACHE);
Line 67: httpResponse.setHeader(EXPIRES, getYesterdayHttpDate());
Line 68: } else {
Line 69: chain.doFilter(request, response);
this line is duplicated in each of the if clauses.
Line 70: }
Line 71: }
Line 72:
Line 73: private String getYesterdayHttpDate() {
Line 69: chain.doFilter(request, response);
Line 70: }
Line 71: }
Line 72:
Line 73: private String getYesterdayHttpDate() {
don't think we need this method.
Line 74: // Get Calendar instance
Line 75: Calendar calendar = Calendar.getInstance();
Line 76: // Subtract a day, so it is in the past.
Line 77: calendar.add(Calendar.DAY_OF_MONTH, -1);
Line 81: dateFormat.setTimeZone(TimeZone.getTimeZone(GMT));
Line 82: return dateFormat.format(calendar.getTime());
Line 83: }
Line 84:
Line 85: private String getNowPlusYearHttpDate() {
don't think we need this method.
Line 86: // Get Calendar instance
Line 87: Calendar calendar = Calendar.getInstance();
Line 88: // Add a year to now.
Line 89: calendar.add(Calendar.YEAR, 1);
Line 93: dateFormat.setTimeZone(TimeZone.getTimeZone(GMT));
Line 94: return dateFormat.format(calendar.getTime());
Line 95: }
Line 96:
Line 97: private HttpServletResponseWrapper getCacheHeaderResponseWrapper(
should return HttpServletResponse
Line 98: final HttpServletResponse httpResponse) {
Line 99: return new HttpServletResponseWrapper(httpResponse) {
Line 100: @Override
Line 101: public void setHeader(final String name, final String
value) {
Line 104: httpResponse.setHeader(name, value);
Line 105: }
Line 106: }
Line 107: };
Line 108: }
please explain why we need this override/add comment
Line 109:
Line 110: private boolean cacheFilterPatternMathes(HttpServletRequest
httpRequest) {
Line 111: boolean result = false;
Line 112: if (null != cachePattern) {
Line 135: List<String> filterParams = Collections.list(filterConfig
Line 136: .getInitParameterNames());
Line 137: // No need to worry about concurrency, worst case scenario
the same
Line 138: // pattern is calculated a couple of times.
Line 139: if (null == cachePattern) {
consider switching the null loc (cachePattern == null), that's the standard in
the engine
Line 140: // Get a list of parameters.
Line 141: for (String paramName : filterParams) {
Line 142: if (CACHE.equals(paramName)) {
Line 143: cachePattern = Pattern.compile(filterConfig
Line 137: // No need to worry about concurrency, worst case scenario
the same
Line 138: // pattern is calculated a couple of times.
Line 139: if (null == cachePattern) {
Line 140: // Get a list of parameters.
Line 141: for (String paramName : filterParams) {
use getInitParameter(name) instead of iterating.
Line 142: if (CACHE.equals(paramName)) {
Line 143: cachePattern = Pattern.compile(filterConfig
Line 144: .getInitParameter(paramName));
Line 145: break;
--
To view, visit http://gerrit.ovirt.org/10449
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5d8e02ae542a4aa37bd421bde5582c0f3e9820ad
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
