Hi John, Can you see if your new patch handles SHINDIG-1183 as well?
On Tue, Oct 27, 2009 at 6:08 PM, John Hjelmstad <[email protected]> wrote: > Ah -- I see Paul committed this one. That's fine by me -- interestingly > enough, I'm not sure if my patch will cleanly apply to loading > sub-resources > of OpenSocialI18NGadgetRewriter's use here. Strike 1 for the new model! :) > > Seriously though, the generic/underlying idea here seems to be > lang/country-specific JS. We could A) implement a delegating loader that > uses lang/country context to resolve FeatureResources (@see my CL's > BrowserSpecificFeatureResourceLoader as an analogue) or B) treat > opensocial-i18n JS specially in the rewriter. (A) has the property > (problem?) that we'd effectively invent a lang/country matching expression > language in feature.xml. [B] could involve a special OpenSocialI18NJSLoader > class if we wanted. > > --j > > On Tue, Oct 27, 2009 at 6:01 PM, John Hjelmstad <[email protected]> > wrote: > > > Hey Jon- > > > > Interesting where you're going with this one, but IMO the need for this > > particular Factory pattern calls for a more thorough reworking of the > > JsLibrary/JsFeatureLoader/GadgetFeature implementation to better > accommodate > > extensions to the feature.xml mechanism. > > > > The main tactical trouble I see with JsLibraryFactory is that its methods > > are A) largely duplicative (what's the difference between create1 and > > create2?), B) somewhat unnecessary (create1 needn't have HttpFetcher > passed > > in; that can be @Inject'ed), and C) above all, these are just glorified > > wrappers for resource loading. The class/interface's raison d'etre isn't > > clear - what does it do? Loads a JsLibrary? What is a JsLibrary? A > > sub-resource in a <gadget> or <container> clause in a feature.xml? A full > > JS-based feature.xml itself? Something else? > > > > Much of this is naming, I'll admit, but I guess what I'm getting at goes > > back to fundamental changes. > > > > This discussion, as well as one I've had with Jas regarding Caja's > > tamings.js inclusion, has inspired me to do a rewrite of the JS feature > > system I've long wanted to do anyway. I just sent you the relevant CL, > but > > for reference it's here: http://codereview.appspot.com/143046 > > > > I'd love to hear your thoughts. I apologize for not getting this out to > you > > sooner; I'll now take a look at the patch you just sent today. Hopefully > it > > will be easy to adapt to the new proposed extension model. > > > > Cheers, > > John > > > > > > On Mon, Oct 19, 2009 at 2:48 PM, <[email protected]> wrote: > > > >> For option B there are actually 2 "public/protected static create" > >> methods, plus some other private/protected methods that could become > >> protected member methods, If we go the whole way I propose (we could > >> skip the interface if people like): > >> > >> public interface JsLibraryFactory { > >> > >> public JsLibrary create(Type type, String content, String feature, > >> HttpFetcher fetcher) > >> > >> public JsLibrary create(String feature, Type type, String content, > >> String debugContent) > >> > >> } > >> > >> public class DefaultJsLibraryFactory { > >> > >> public JsLibrary create(Type type, String content, String feature, > >> HttpFetcher fetcher) > >> > >> public JsLibrary create(String feature, Type type, String content, > >> String debugContent) > >> > >> protected void loadOptimizedAndDebugData(String content, Type type, > >> StringBuffer opt, StringBuffer dbg) > >> > >> Might even be good to do loadFile, loadResource, loadData, > >> loadDataFromUrl as protected. > >> > >> Looks like someone tried to do these as "protected static" methods. > >> These cannot be @Overridden, so not sure the full intent of them. > >> > >> } > >> > >> -- > >> > >> This is what we do, and why I'm interested: > >> > >> 1) Some of our JS libraries are different from Shindig source by a few > >> lines. For maintainability we reference the original source and "patch" > >> the libraries at load time. > >> > >> 2) We don't use mvn, so JS minimization is also done a load time. > >> > >> 3) For development of features, there is a small hook in the code to > >> load the libraries dynamically - rather than once. > >> > >> > >> > >> > >> http://codereview.appspot.com/135048 > >> > > > > >
