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 >
