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 <johnfa...@gmail.com> 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, <jon.weyga...@gmail.com> 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 >> > >
