http://codereview.appspot.com/32111/diff/1061/1069 File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right):
http://codereview.appspot.com/32111/diff/1061/1069#newcode144 Line 144: .setResizeQuality(request.getParamAsInteger(PARAM_RESIZE_QUALITY)); The cache having explicit knowledge of image manipulation is really not appropriate. It would be a lot better to simply take all of those extra params that getParam* are returning and append those. The cache key can then become a set of key / value pairs (a map), without any explicit knowledge of image processing. http://codereview.appspot.com/32111/diff/1061/1068 File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CacheKeyBuilder.java (right): http://codereview.appspot.com/32111/diff/1061/1068#newcode33 Line 33: public class CacheKeyBuilder { The strongly typed nature of this class is really overkill, and it creates a situation where one class (this one) has to have explicit knowledge of many parts of the system. Just using the sorted map to build cache keys avoids this problem. http://codereview.appspot.com/32111/diff/1061/1070 File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpRequest.java (right): http://codereview.appspot.com/32111/diff/1061/1070#newcode55 Line 55: private final Map<String, String> params = Maps.newHashMap(); When adding this, all of the existing ad hoc params should be included as well. This is probably best handled with a follow up patch for now though, to keep the size of this change small. http://codereview.appspot.com/32111/diff/1061/1070#newcode396 Line 396: String value = params.get(paramName); This is a redundant hash lookup. Use params.get and perform a null check on that instead of using containsKey. http://codereview.appspot.com/32111/diff/1061/1070#newcode401 Line 401: params.put(paramName, paramValue.toString()); String.valueOf is a bit more robust here. http://codereview.appspot.com/32111/diff/1061/1075 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/ImageUtils.java (right): http://codereview.appspot.com/32111/diff/1061/1075#newcode156 Line 156: { This should be on the same line as the closing paren above. http://codereview.appspot.com/32111/diff/1061/1071 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (right): http://codereview.appspot.com/32111/diff/1061/1071#newcode129 Line 129: // additional referrer checks. The HTTP spec actually spells this 'referer'. It' stupid, but it's not a mistake here. http://codereview.appspot.com/32111/diff/1061/1071#newcode148 Line 148: return true; This makes it so that resized images will never be cached. I don't think that's the behavior you actually want. http://codereview.appspot.com/32111/diff/1061/1072 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/32111/diff/1061/1072#newcode69 Line 69: private HttpRequest buildHttpRequest(HttpServletRequest servletRequest) throws GadgetException { Please don't rename parameters or variables unless it's actually material to the code in question. It complicates diffs unnecessarily. http://codereview.appspot.com/32111
