http://codereview.appspot.com/63225/diff/1/7 File java/common/src/main/java/org/apache/shindig/common/servlet/HttpServletUserAgentProvider.java (right):
http://codereview.appspot.com/63225/diff/1/7#newcode32 Line 32: private Provider<HttpServletRequest> reqProvider; These should both be final. http://codereview.appspot.com/63225/diff/1/6 File java/common/src/main/java/org/apache/shindig/common/servlet/UserAgent.java (right): http://codereview.appspot.com/63225/diff/1/6#newcode54 Line 54: public boolean isOtherHtml5() { This function doesn't really make a lot of sense. Why not have something that simply returns isHtml5 to test for HTML5 support? Why not just use OTHER when we don't know anything about the browser and let the calling code make its own assumptions? If we actually need to add support for a new browser at some point in the future, we're going to have to add logic to the code that handles this anyway. http://codereview.appspot.com/63225/diff/1/6#newcode70 Line 70: public static class Entry { Why isn't this class UserAgent? UserAgent here is just being used as a pseudo namespace for containing 3 other classes. This should either be 3 separate classes (well, 1 class 1 enum and 1 interface), or everything in Entry should just be moved to UserAgent. http://codereview.appspot.com/63225/diff/1/10 File java/gadgets/src/main/java/org/apache/shindig/gadgets/JsLibrary.java (right): http://codereview.appspot.com/63225/diff/1/10#newcode74 Line 74: return true; This is more or less a pointless check since almost every gadget rendered is going to wind up pulling in rpc (and those that don't will have it forced on them via the libs parameter anyway). I think it's better to avoid the complexity and just assume that we always use private caching for the library code. http://codereview.appspot.com/63225/diff/1/9 File java/gadgets/src/main/java/org/apache/shindig/gadgets/SmartRpcJsFeatureLoader.java (right): http://codereview.appspot.com/63225/diff/1/9#newcode30 Line 30: public class SmartRpcJsFeatureLoader extends JsFeatureLoader { Some comment explaining what this class is and what it's used for would be nice. Is the regular JsFeatureLoader dumb? Why is this one smart? It looks to me like this is just doing special stuff for the "rpc" feature, not really anything particularly smart. Wouldn't it be a lot easier to just extend the regular JsFeatureLoader to support User-Agent blocks (say an attribute on <gadget> elements, just like we already have the 'container' attribute)? If I need to customize rpc.js, I have to edit this class, whereas for any other feature I only have to modify javascript. I don't see why we cant just add UA support to the feature manifest. http://codereview.appspot.com/63225
