Fix problem with loading i18n constants for RenderingGadgetRewritter

This is a fix for the bug below:

https://issues.apache.org/jira/browse/SHINDIG-1159

[Shaopeng, 2009-08-28 09:48:11]

modified indentation

[johnfargo, 2009-08-28 18:17:39]

Thanks again for doing this Shaopeng! Comments provided.

http://codereview.appspot.com/109114/diff/2001/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/109114/diff/2001/4#newcode61
Line 61: import java.io.File;
out of order

http://codereview.appspot.com/109114/diff/2001/4#newcode272
Line 272: }
As mentioned in our corresponding discussion thread, somewhat duplicative as it
is, this should be in a separate rewriter. In its implementation, we should
inject a separate <script> tag containing the I18N constants to isolate their
effects from other inline JS as a defensive coding measure.

http://codereview.appspot.com/109114/diff/2001/4#newcode319
Line 319: File file = new File(dataPath + "DateTimeConstants__localeName_" +
locale.getCountry());
For less brittleness, let's use org.apache.shindig.common.util.ResourceLoader.
@see JsFeatureLoader.java for an example of use. Because the features JARs are
always loaded when starting a given instance of the gadget server, you're
guaranteed their resources will be available.

In short, that changes new File("features/target/classes/features/i18n/data/" +
file) to:
String content = ResourceLoader.getContent(features/i18n/data/" _ file);

Sorry not to have noted this before.

[Shaopeng, 2009-08-31 08:29:35]

I am uploading a new Patch Set which incorporates all the changes. Please take a
look. Thanks,

Shaopeng

http://codereview.appspot.com/109114/diff/2001/4
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/109114/diff/2001/4#newcode61
Line 61: import java.io.File;
On 2009/08/28 18:17:39, johnfargo wrote:
> out of order

Done.

http://codereview.appspot.com/109114/diff/2001/4#newcode272
Line 272: }
On 2009/08/28 18:17:39, johnfargo wrote:
> As mentioned in our corresponding discussion thread, somewhat duplicative as
it
> is, this should be in a separate rewriter. In its implementation, we should
> inject a separate <script> tag containing the I18N constants to isolate their
> effects from other inline JS as a defensive coding measure.

I agree. It is a good idea to put it into a separate <script> tag.

http://codereview.appspot.com/109114/diff/2001/4#newcode319
Line 319: File file = new File(dataPath + "DateTimeConstants__localeName_" +
locale.getCountry());
On 2009/08/28 18:17:39, johnfargo wrote:
> For less brittleness, let's use org.apache.shindig.common.util.ResourceLoader.
> @see JsFeatureLoader.java for an example of use. Because the features JARs are
> always loaded when starting a given instance of the gadget server, you're
> guaranteed their resources will be available.
> 
> In short, that changes new File("features/target/classes/features/i18n/data/"
+
> file) to:
> String content = ResourceLoader.getContent(features/i18n/data/" _ file);
> 
> Sorry not to have noted this before.

Done.

[Shaopeng, 2009-08-31 08:31:51]

creating OpenSocialI18NGadgetRewriter

[johnfargo, 2009-08-31 20:26:25]

Hi Shaopeng:

This is looking good. I have a few nits and one comment of real substance
(lang_COUNTRY selection).

Do you have a test for this class?

http://codereview.appspot.com/109114/diff/3001/3002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/109114/diff/3001/3002#newcode44
Line 44: }
ctor not needed; @Inject will just find the class automatically.

http://codereview.appspot.com/109114/diff/3001/3002#newcode52
Line 52: if (!gadget.getAllFeatures().contains("opensocial-i18n")) {
nit: pull out into private static final String I18N_FEATURE_NAME =
"opensocial-i18n";

http://codereview.appspot.com/109114/diff/3001/3002#newcode71
Line 71: String dataPath = "features/target/classes/features/i18n/data/";
change this to:
res://features/i18n/data --> you'll get JS out of the runtime JARs rather than
depending on the filesystem being set up just so.

http://codereview.appspot.com/109114/diff/3001/3002#newcode73
Line 73: String localeName = locale.getLanguage();
For readability, I'd like to see this changed to:
String language = locale.getLanguage();
String localeName = language;

http://codereview.appspot.com/109114/diff/3001/3002#newcode75
Line 75: localeName = "en";
do you think it makes sense to introduce a "default" datafile with lang="all"
that's a copy of the "en" constants?

http://codereview.appspot.com/109114/diff/3001/3002#newcode83
Line 83: localeName += "_" + locale.getCountry();
I think you want getCountry().toUpperCase() and to suffix with ".js," else I
don't see this ever succeeding

http://codereview.appspot.com/109114/diff/3001/3002#newcode95
Line 95:
inlineJs.append(dateTimeConstants.getContent()).append("\n").append(numberConstants.getContent());
>100 char line

[Shaopeng, 2009-08-31 21:34:55]

http://codereview.appspot.com/109114/diff/3001/3002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/109114/diff/3001/3002#newcode44
Line 44: }
On 2009/08/31 20:26:25, johnfargo wrote:
> ctor not needed; @Inject will just find the class automatically.

Done.

http://codereview.appspot.com/109114/diff/3001/3002#newcode52
Line 52: if (!gadget.getAllFeatures().contains("opensocial-i18n")) {
On 2009/08/31 20:26:25, johnfargo wrote:
> nit: pull out into private static final String I18N_FEATURE_NAME =
> "opensocial-i18n";

Done.

http://codereview.appspot.com/109114/diff/3001/3002#newcode71
Line 71: String dataPath = "features/target/classes/features/i18n/data/";
On 2009/08/31 20:26:25, johnfargo wrote:
> change this to:
> res://features/i18n/data --> you'll get JS out of the runtime JARs rather than
> depending on the filesystem being set up just so.

Done.

http://codereview.appspot.com/109114/diff/3001/3002#newcode73
Line 73: String localeName = locale.getLanguage();
On 2009/08/31 20:26:25, johnfargo wrote:
> For readability, I'd like to see this changed to:
> String language = locale.getLanguage();
> String localeName = language;

Done.

http://codereview.appspot.com/109114/diff/3001/3002#newcode75
Line 75: localeName = "en";
On 2009/08/31 20:26:25, johnfargo wrote:
> do you think it makes sense to introduce a "default" datafile with lang="all"
> that's a copy of the "en" constants?

Not really. Having two files with the same contents but different names is kind
of problematic. Having it in the programming logic is much clearer and easier to
change later.

http://codereview.appspot.com/109114/diff/3001/3002#newcode83
Line 83: localeName += "_" + locale.getCountry();
On 2009/08/31 20:26:25, johnfargo wrote:
> I think you want getCountry().toUpperCase() and to suffix with ".js," else I
> don't see this ever succeeding

Oops, I left out the .js... But toUpperCase() doesn't seem to be necessary, as
getCountry() is already returning the upper case version.

http://codereview.appspot.com/109114/diff/3001/3002#newcode95
Line 95:
inlineJs.append(dateTimeConstants.getContent()).append("\n").append(numberConstants.getContent());
On 2009/08/31 20:26:25, johnfargo wrote:
> >100 char line

Done.

[Shaopeng, 2009-08-31 21:35:45]

more fixes

[Shaopeng, 2009-08-31 21:38:15]

http://codereview.appspot.com/109114/diff/3006/3007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/109114/diff/3006/3007#newcode67
Line 67: String dataPath = "res://features/i18n/data/";
it seems with this change, I start to get problems:

Aug 31, 2009 11:31:34 PM org.apache.shindig.gadgets.JsLibrary loadResource
WARNING: Could not find resource:
res://features/i18n/data/DateTimeConstants__en.opt.js-Can not locate resource:
res://features/i18n/data/DateTimeConstants__en.opt.js
Aug 31, 2009 11:31:34 PM org.apache.shindig.gadgets.JsLibrary loadResource
WARNING: Could not find resource:
res://features/i18n/data/DateTimeConstants__en.js-Can not locate resource:
res://features/i18n/data/DateTimeConstants__en.js
2009-08-31 23:31:34.512::WARN:  /gadgets/ifr
java.lang.IllegalArgumentException: Problems reading resource
res://features/i18n/data/DateTimeConstants__en.js

any idea why is it so?

[Shaopeng, 2009-09-01 13:17:59]

http://codereview.appspot.com/109114/diff/3006/3007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java
(right):

http://codereview.appspot.com/109114/diff/3006/3007#newcode67
Line 67: String dataPath = "res://features/i18n/data/";
On 2009/08/31 21:38:16, Shaopeng wrote:
> it seems with this change, I start to get problems:
> 
> Aug 31, 2009 11:31:34 PM org.apache.shindig.gadgets.JsLibrary loadResource
> WARNING: Could not find resource:
> res://features/i18n/data/DateTimeConstants__en.opt.js-Can not locate resource:
> res://features/i18n/data/DateTimeConstants__en.opt.js
> Aug 31, 2009 11:31:34 PM org.apache.shindig.gadgets.JsLibrary loadResource
> WARNING: Could not find resource:
> res://features/i18n/data/DateTimeConstants__en.js-Can not locate resource:
> res://features/i18n/data/DateTimeConstants__en.js
> 2009-08-31 23:31:34.512::WARN:  /gadgets/ifr
> java.lang.IllegalArgumentException: Problems reading resource
> res://features/i18n/data/DateTimeConstants__en.js
> 
> any idea why is it so?
> 

After reading Shindig code, I found the problem. JsLibrary.create is calling
ResourceLoader.getContent, which in turn calls ResourceLoader.openResource. I
noticed that in ResourceLoader.open, it removes "res://" before calling
ResourceLoader.openResource. If getContent is calling openResource, that would
be working. So I removed "res://" from the dataPath, and it is working now. I
have uploaded the latest patch.

[Shaopeng, 2009-09-01 13:18:58]

Fix loadResource problem

[johnfargo, 2009-09-01 17:25:49]

This is looking great. A few questions:
1. Do you have a test for this?
2. What happens when an invalid/unmatched locale is passed?
3. Please add a simple cache (Map<Locale, String> will do) for the retrieved
data so you don't need to reload the data from JARs all the time.

Thanks!
John

[Shaopeng, 2009-09-01 21:11:01]

On 2009/09/01 17:25:49, johnfargo wrote:
> This is looking great. A few questions:
> 1. Do you have a test for this?

I have been working on the test today, but it seems to be more complicated than
I thought. What exactly do we want to test? On the rendering side, it is
actually exactly the same as RenderngGadgetRewritter, and I don't think we
really want to load the actual data constant from resource to do the test. The
only additional work in this class is the logic of figuring out which file to
load based on the locale. Shall we test on that only?

> 2. What happens when an invalid/unmatched locale is passed?

The data files I checked in covers all language/country that OpenSocial support.
For invalid/unmatched ones, it should fall back to ALL, and we have the logic in
this file to deal with that, so there shouldn't be of any issue.

> 3. Please add a simple cache (Map<Locale, String> will do) for the retrieved
> data so you don't need to reload the data from JARs all the time.
> 

That is a great suggestion. I have made the change.

> Thanks!
> John

[Shaopeng, 2009-09-01 21:13:03]

Adding cache for i18n data constants in OpenSocialI18NGadgetRewriter

[Shaopeng, 2009-09-02 14:11:51]

Added Unit Test, fixed exception on invalid locale

[Shaopeng, 2009-09-02 18:59:40]

fixing two nits