Quick fix on undefined ___jsl

- always inject ___jsl.
- minor clean-ups.

[johnfargo, 2011-08-01 21:24:00]

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

http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java:69:
builder.prependJs(BASE_HINT_TEMPLATE, CODE_ID);
This is reasonable to do anyway, but I'm curious about the particulars of the
error condition that you found. In theory nohint should mean "don't modify
___jsl" though what it really means is "don't inject anything dynamic". Mostly
want to clarify what I missed the first go-around.

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

http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java:51:
"window['___jsl'] = window['___jsl'] || {};" +
this is an instance basically of the same thing ;) I'd argue that nohint=1
should perhaps be applied for this processor, under the assertion that nohint ==
"second phase load"

[mhermanto, 2011-08-01 21:53:41]

On Mon, Aug 1, 2011 at 2:24 PM, <johnfargo@gmail.com> wrote:

>
> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
> src/main/java/org/apache/**shindig/gadgets/js/**
>
AddJslInfoVariableProcessor.**java<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java>
> File
>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> AddJslInfoVariableProcessor.**java
> (right):
>
> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
> src/main/java/org/apache/**shindig/gadgets/js/**
>
AddJslInfoVariableProcessor.**java#newcode69<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java#newcode69>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> AddJslInfoVariableProcessor.**java:69:
>
> builder.prependJs(BASE_HINT_**TEMPLATE, CODE_ID);
> This is reasonable to do anyway, but I'm curious about the particulars
> of the error condition that you found. In theory nohint should mean
> "don't modify ___jsl" though what it really means is "don't inject
> anything dynamic". Mostly want to clarify what I missed the first
> go-around.
>

I think we should have ___jsl = {} injection completely in another
processor, given some set of conditions.
This change will not fix the problem that was reported, so I can remove it.
I can't explain why now, but as long as DeferredExportJsProcessor is doing
builder.cleariJs(), whatever I put in here will not matter. I can revert
this file.


>
> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
> src/main/java/org/apache/**shindig/gadgets/js/**
>
ConfigInjectionProcessor.java<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java>
> File
>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> ConfigInjectionProcessor.java
> (right):
>
> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
> src/main/java/org/apache/**shindig/gadgets/js/**
>
ConfigInjectionProcessor.java#**newcode51<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java#newcode51>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> ConfigInjectionProcessor.java:**51:
> "window['___jsl'] = window['___jsl'] || {};" +
> this is an instance basically of the same thing ;) I'd argue that
> nohint=1 should perhaps be applied for this processor, under the
> assertion that nohint == "second phase load"


It is the same, but this will (quickly ;-) fix. I think the condition need
to be revised to what you said (but I don't know the full extent what it
should be), and possibly the ordering of processors in pipeline. Leaving as
is, as it's safe (with possibly bit of extra code).

http://codereview.appspot.com/**4816065/<http://codereview.appspot.com/4816065/>
>

[johnfargo, 2011-08-01 22:03:33]

LGTM++

Agreed on both -- and, with the second change being the only file in the CL
I see what happened here. Let's discuss the more foundational changes (eg.
filtering out nohint=1) after this fix.

On Mon, Aug 1, 2011 at 2:53 PM, Michael Hermanto <mhermanto@gmail.com>wrote:

>
>
> On Mon, Aug 1, 2011 at 2:24 PM, <johnfargo@gmail.com> wrote:
>
>>
>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>> src/main/java/org/apache/**shindig/gadgets/js/**
>>
AddJslInfoVariableProcessor.**java<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java>
>> File
>>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> AddJslInfoVariableProcessor.**java
>> (right):
>>
>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>> src/main/java/org/apache/**shindig/gadgets/js/**
>>
AddJslInfoVariableProcessor.**java#newcode69<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java#newcode69>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> AddJslInfoVariableProcessor.**java:69:
>>
>> builder.prependJs(BASE_HINT_**TEMPLATE, CODE_ID);
>> This is reasonable to do anyway, but I'm curious about the particulars
>> of the error condition that you found. In theory nohint should mean
>> "don't modify ___jsl" though what it really means is "don't inject
>> anything dynamic". Mostly want to clarify what I missed the first
>> go-around.
>>
>
> I think we should have ___jsl = {} injection completely in another
> processor, given some set of conditions.
> This change will not fix the problem that was reported, so I can remove it.
> I can't explain why now, but as long as DeferredExportJsProcessor is doing
> builder.cleariJs(), whatever I put in here will not matter. I can revert
> this file.
>
>
>>
>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>> src/main/java/org/apache/**shindig/gadgets/js/**
>>
ConfigInjectionProcessor.java<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java>
>> File
>>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> ConfigInjectionProcessor.java
>> (right):
>>
>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>> src/main/java/org/apache/**shindig/gadgets/js/**
>>
ConfigInjectionProcessor.java#**newcode51<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java#newcode51>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> ConfigInjectionProcessor.java:**51:
>> "window['___jsl'] = window['___jsl'] || {};" +
>> this is an instance basically of the same thing ;) I'd argue that
>> nohint=1 should perhaps be applied for this processor, under the
>> assertion that nohint == "second phase load"
>
>
> It is the same, but this will (quickly ;-) fix. I think the condition need
> to be revised to what you said (but I don't know the full extent what it
> should be), and possibly the ordering of processors in pipeline. Leaving as
> is, as it's safe (with possibly bit of extra code).
>
>
http://codereview.appspot.com/**4816065/<http://codereview.appspot.com/4816065/>
>>
>
>

[mhermanto, 2011-08-01 22:05:09]

Update patch

[mhermanto, 2011-08-01 22:05:53]

Yap, second change only. Changes re-uploaded.

On Mon, Aug 1, 2011 at 3:03 PM, John Hjelmstad <johnfargo@gmail.com> wrote:

> LGTM++
>
> Agreed on both -- and, with the second change being the only file in the CL
> I see what happened here. Let's discuss the more foundational changes (eg.
> filtering out nohint=1) after this fix.
>
>
> On Mon, Aug 1, 2011 at 2:53 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
>
>>
>>
>> On Mon, Aug 1, 2011 at 2:24 PM, <johnfargo@gmail.com> wrote:
>>
>>>
>>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>>> src/main/java/org/apache/**shindig/gadgets/js/**
>>>
AddJslInfoVariableProcessor.**java<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java>
>>> File
>>>
>>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>>> AddJslInfoVariableProcessor.**java
>>> (right):
>>>
>>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>>> src/main/java/org/apache/**shindig/gadgets/js/**
>>>
AddJslInfoVariableProcessor.**java#newcode69<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/AddJslInfoVariableProcessor.java#newcode69>
>>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>>> AddJslInfoVariableProcessor.**java:69:
>>>
>>> builder.prependJs(BASE_HINT_**TEMPLATE, CODE_ID);
>>> This is reasonable to do anyway, but I'm curious about the particulars
>>> of the error condition that you found. In theory nohint should mean
>>> "don't modify ___jsl" though what it really means is "don't inject
>>> anything dynamic". Mostly want to clarify what I missed the first
>>> go-around.
>>>
>>
>> I think we should have ___jsl = {} injection completely in another
>> processor, given some set of conditions.
>> This change will not fix the problem that was reported, so I can remove
>> it. I can't explain why now, but as long as DeferredExportJsProcessor is
>> doing builder.cleariJs(), whatever I put in here will not matter. I can
>> revert this file.
>>
>>
>>>
>>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>>> src/main/java/org/apache/**shindig/gadgets/js/**
>>>
ConfigInjectionProcessor.java<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java>
>>> File
>>>
>>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>>> ConfigInjectionProcessor.java
>>> (right):
>>>
>>> http://codereview.appspot.com/**4816065/diff/1/java/gadgets/**
>>> src/main/java/org/apache/**shindig/gadgets/js/**
>>>
ConfigInjectionProcessor.java#**newcode51<http://codereview.appspot.com/4816065/diff/1/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ConfigInjectionProcessor.java#newcode51>
>>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>>> ConfigInjectionProcessor.java:**51:
>>> "window['___jsl'] = window['___jsl'] || {};" +
>>> this is an instance basically of the same thing ;) I'd argue that
>>> nohint=1 should perhaps be applied for this processor, under the
>>> assertion that nohint == "second phase load"
>>
>>
>> It is the same, but this will (quickly ;-) fix. I think the condition need
>> to be revised to what you said (but I don't know the full extent what it
>> should be), and possibly the ordering of processors in pipeline. Leaving as
>> is, as it's safe (with possibly bit of extra code).
>>
>>
http://codereview.appspot.com/**4816065/<http://codereview.appspot.com/4816065/>
>>>
>>
>>
>