JSL beacon now keeps track of loaded features

Which comprise of explicit and implicit features. Renamed other JSL-based processors for consistency.

[jtarrio, 2011-05-05 22:57:19]

http://codereview.appspot.com/4486043/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java
(right):

http://codereview.appspot.com/4486043/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:47:
"window['___jsl']['l'] = window['___jsl']['l'].concat(%s);";
You can change these two statements to this single form:
x = (x || []).concat([a, b, c]);

[mhermanto, 2011-05-05 23:16:04]

Addressing jtarrio's comments.

[mhermanto, 2011-05-05 23:16:07]

On Thu, May 5, 2011 at 3:57 PM, <jtarrio@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4486043/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java
> (right):
>
>
>
http://codereview.appspot.com/4486043/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:47:
> "window['___jsl']['l'] = window['___jsl']['l'].concat(%s);";
> You can change these two statements to this single form:
> x = (x || []).concat([a, b, c]);
>
> Done.

>
> http://codereview.appspot.com/4486043/
>

[jtarrio, 2011-05-05 23:26:17]

lgtm

[johnfargo, 2011-05-05 23:30:11]

http://codereview.appspot.com/4486043/diff/6002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java
(right):

http://codereview.appspot.com/4486043/diff/6002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:46:
"window['___jsl']['l'] = (window['___jsl']['l'] || []).concat(%s);";
at first I wanted this to be an Object for map-style lookup, but actually having
an array is valuable to see what got double-loaded (if such occurs).

we should just be sure in downstream loader code to dedupe.

http://codereview.appspot.com/4486043/diff/6002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:67:
.getFeatureResources(ctx, jsUri.getLibs(), unsupported);
shouldn't we be subtracting the loaded libs from the list?

[mhermanto, 2011-05-06 01:40:51]

Addressing John's comments

[mhermanto, 2011-05-06 01:41:19]

On Thu, May 5, 2011 at 4:30 PM, <johnfargo@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4486043/diff/6002/java/gadgets/src/main/java/or...
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java
> (right):
>
>
>
http://codereview.appspot.com/4486043/diff/6002/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:46:
>
> "window['___jsl']['l'] = (window['___jsl']['l'] || []).concat(%s);";
> at first I wanted this to be an Object for map-style lookup, but
> actually having an array is valuable to see what got double-loaded (if
> such occurs).
>
> we should just be sure in downstream loader code to dedupe.
>

Same, was thinking of a set too, but that will generate longer JS.

>
>
>
http://codereview.appspot.com/4486043/diff/6002/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:67:
> .getFeatureResources(ctx, jsUri.getLibs(), unsupported);
> shouldn't we be subtracting the loaded libs from the list?
>
>
We can, and I think we should to detect doubly-loaded libraries.

Updated patch. PTAL.


>
> http://codereview.appspot.com/4486043/
>

[johnfargo, 2011-05-06 01:46:41]

LGTM++

http://codereview.appspot.com/4486043/diff/9002/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java
(right):

http://codereview.appspot.com/4486043/diff/9002/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:69:
private Set<String> getBundleNames(JsUri jsUri, boolean loaded) throws
JsException {
add TODO: factor this logic into somewhere shared. it's now in GetJsContent too,
and might be used elsewhere.

[mhermanto, 2011-05-06 01:50:12]

Done.

On Thu, May 5, 2011 at 6:46 PM, <johnfargo@gmail.com> wrote:

> LGTM++
>
>
>
>
http://codereview.appspot.com/4486043/diff/9002/java/gadgets/src/main/java/or...
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java
> (right):
>
>
>
http://codereview.appspot.com/4486043/diff/9002/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslLoadedVariableProcessor.java:69:
> private Set<String> getBundleNames(JsUri jsUri, boolean loaded) throws
> JsException {
> add TODO: factor this logic into somewhere shared. it's now in
> GetJsContent too, and might be used elsewhere.
>
>
> http://codereview.appspot.com/4486043/
>