I've just updated it (patch update on codereview forthcoming) to fix sub-bug #1. I'll merge in Jon's changes re: #2 after committing (assuming the code review goes well) my patch.
--j On Wed, Oct 28, 2009 at 12:58 PM, Paul Lindner <[email protected]> wrote: > 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 > > >> > > > > > > > > >
