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

[johnfargo, 2009-10-19 19:06:04]

http://codereview.appspot.com/135048/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/135048/diff/1/2#newcode79
Line 79: JsLibrary numberConstants = createJsLibrary(JsLibrary.Type.RESOURCE,
To me this isn't injected so much as overridable. Actual injection would make
sense to me: make createJsLibrary in JsFeatureLoader public, then @Inject the
JsFeatureLoader here.

The downside of this is that JsFeatureLoader wasn't really designed from the
start to be @Injectable... but that will have to be cleaned up later anyway.

[Jon Weygandt, 2009-10-19 20:53:32]

http://codereview.appspot.com/135048/diff/1/2
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/135048/diff/1/2#newcode79
Line 79: JsLibrary numberConstants = createJsLibrary(JsLibrary.Type.RESOURCE,
The pattern you are suggesting is a "JsLibrary factory" pattern, which I do
like. I could do this one of 3 ways, and would like to get some comments on it:

A) Simply inject JsFeatureLoader and it becomes the factory.

B) Create a class (and maybe an interface and default impl) JsLibraryFactory,
and put the static methods of JsLibrary into this. Personally I like this, it
would be very clear you have a factory pattern at work, and would work better
for me, but it is a larger code change.

C) Simply stay with this minimalist change.

On 2009/10/19 19:06:04, johnfargo wrote:
> To me this isn't injected so much as overridable. Actual injection would make
> sense to me: make createJsLibrary in JsFeatureLoader public, then @Inject the
> JsFeatureLoader here.
> 
> The downside of this is that JsFeatureLoader wasn't really designed from the
> start to be @Injectable... but that will have to be cleaned up later anyway.

[Jon Weygandt, 2009-10-19 21:48:06]

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.

[Jon Weygandt, 2009-10-27 16:22:41]

This patch implements Option A

[Jon Weygandt, 2009-10-27 16:23:38]

This patch implements Option A (hopefully the right one this time)

[Jon Weygandt, 2009-10-27 16:58:04]

http://codereview.appspot.com/135048/diff/2011/3004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/135048/diff/2011/3004#newcode47
Line 47: private Map<Locale, String> i18nConstantsCache = new
ConcurrentHashMap<Locale, String>();
This is a concurrency bug, unrelated to the feature, but included with this
patch. This class is for all practical purposes a Singleton, thus mutable
members need proper synchronization. Since the data being stored is generated in
an idempotent manner and is immutable the simple "put" (rather than
"pubIfAbsent" is all that is requried)