Replaced by: http://codereview.appspot.com/63226
On Tue, Jun 9, 2009 at 6:16 PM, John Hjelmstad <[email protected]> wrote: > 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 >> > >
