RE: [SHINDIG-1199] OpenSocialI18NGadgetRewriter's creation of JsLibrary should be consistent with JsFeat

["Weygandt, Jon" <jw...@ebay.com>]

John - thanks
 
Paul - 
SHINDIG-1183 Unsupported Features and 
SHINDIG-1199 OpenSocialI18N...
Touch 2 different sets of files for 2 different reasons. But John's
bigger change spans thoes file sets.
 
Once the larger change (http://codereview.appspot.com/143046
<http://codereview.appspot.com/143046> ) happens, SHINDIG-1183 should be
closed and NOT applied.

________________________________

From: John Hjelmstad [mailto:fargo@google.com] 
Sent: Wednesday, October 28, 2009 1:09 PM
To: shindig-dev@incubator.apache.org
Cc: jon.weygandt@gmail.com; johnfargo@gmail.com
Subject: Re: [SHINDIG-1199] OpenSocialI18NGadgetRewriter's creation of
JsLibrary should be consistent with JsFeat


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 <li...@inuus.com>
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
<jo...@gmail.com> 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
<jo...@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, <jo...@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
	> >>
	> >
	> >
	>