Thanks for the review. Comments inline. New patch to be posted shortly. On Tue, Jun 9, 2009 at 5:36 PM, <[email protected]> wrote:
> > 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. Good catch, done. > > > 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. The idea was to make it possible to write a parser that knows about more browsers than the framework does, and able to signal just this bit for those "new" browsers. I agree this is confusing. I'll just remove the bit. > > > 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. Writing code too late at night I guess. Entry rolled up to UA. > > > 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. The idea here is to make it possible to opt into or out of per-browser rpc rather than force it on all deployments. I would prefer the simplicity though. Paul/others, what do you think? > > > 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. The semantic binding was intended to be between "Smart" and "Rpc" rather than "Smart" and "JsFeatureLoader" :) Renamed to more descriptive "BrowserSpecificRpcJsFeatureLoader" > > > 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. I considered this before implementing the approach seen here. I like the more generalized approach, but chose this one for a few reasons: 1. Above all, the ability to turn this functionality off quickly, eg. through a rebinding or an easily-written wrapper that signals on some type of configuration. At least until the technique is proven (or disproven) to be useful in practice in a wide variety of settings. 2. rpc.js is the most inherently browser-specific code we have. There's room for others (box model differences, dynamic-height) but I'm loath to encourage more browser-specific code to be written than is necessary. 3. Wanted to avoid coming up with a browser/version configuration syntax in feature.xml. After all, all known major browsers are covered by rpc.js at this point, with all new browsers supporting HTML5, so modifying rpc.js is still a matter of modifying the .js files. Thx, --John > > > http://codereview.appspot.com/63225 >
