Replace ExportJsCompiler with Processor

[johnfargo, 2011-04-04 22:35:38]

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

http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:75:
neededExportJs = true;
it's not clear to me this ever happens, since we error out when receiving a
pipeline request that has no content. granted, as a generic pipeline it's
theoretically possible.

http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:80:
builder.appendAllJs(getExportJsContents());
two ideas for simplification:
1) Just introduce prependJs() for exportJsContents(). It's an easy enough API to
offer.
2) the processor model becomes a whole lot easier if we either put all exports
at the end, or we implement insertAt().

http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:82:
builder.appendAllJs(resp.build());
this semantically belongs in if (neededExportJs), which sort of underscores that
"neededExportJs" might be unnecessary

http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:105:
return JsContent.fromFeature(sb.toString(), "[generated-symbol-exports]",
this will add a meaningless empty string if there are no exports needed; we
should avoid that.

[mhermanto, 2011-04-05 01:31:16]

On Mon, Apr 4, 2011 at 3:35 PM, <johnfargo@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java
> (right):
>
>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:75:
> neededExportJs = true;
> it's not clear to me this ever happens, since we error out when
> receiving a pipeline request that has no content. granted, as a generic
> pipeline it's theoretically possible.
>

If none of the requests are feature-based, ie: all are JsContent.fromText().


>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:80:
> builder.appendAllJs(getExportJsContents());
> two ideas for simplification:
> 1) Just introduce prependJs() for exportJsContents(). It's an easy
> enough API to offer.
>

Added/used, but does not matter here since it's clearJs()'ed.


> 2) the processor model becomes a whole lot easier if we either put all
> exports at the end,


Yes, we talked about this. Features don't really share namespace so we don't
get the shorter exportJs statements benefit. I see one nice thing of having
exportJs after every feature -- debugging compiled output.


> or we implement insertAt().
>

I thought of this, but I'd have to keep track of indexes. Added API anyway.


>
>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:82:
> builder.appendAllJs(resp.build());
> this semantically belongs in if (neededExportJs), which sort of
> underscores that "neededExportJs" might be unnecessary
>

Restructured. Done.


>
>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:105:
> return JsContent.fromFeature(sb.toString(),
> "[generated-symbol-exports]",
> this will add a meaningless empty string if there are no exports needed;
> we should avoid that.
>
>
> Was maintaining status-quo from existing file. Improved now nevertheless.



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

[mhermanto, 2011-04-05 01:32:50]

Addressing John's comments

[fargo, 2011-04-05 01:35:38]

On Mon, Apr 4, 2011 at 6:30 PM, Michael Hermanto <mhermanto@gmail.com>wrote:

>
>
> On Mon, Apr 4, 2011 at 3:35 PM, <johnfargo@gmail.com> wrote:
>
>>
>>
>>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>> File
>>
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java
>> (right):
>>
>>
>>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:75:
>> neededExportJs = true;
>> it's not clear to me this ever happens, since we error out when
>> receiving a pipeline request that has no content. granted, as a generic
>> pipeline it's theoretically possible.
>>
>
> If none of the requests are feature-based, ie: all are
> JsContent.fromText().
>

Yeah, that situation is weird but I guess it's probably innocuous to account
for it.


>
>
>>
>>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:80:
>> builder.appendAllJs(getExportJsContents());
>> two ideas for simplification:
>> 1) Just introduce prependJs() for exportJsContents(). It's an easy
>> enough API to offer.
>>
>
> Added/used, but does not matter here since it's clearJs()'ed.
>
>
>> 2) the processor model becomes a whole lot easier if we either put all
>> exports at the end,
>
>
> Yes, we talked about this. Features don't really share namespace so we
> don't get the shorter exportJs statements benefit. I see one nice thing of
> having exportJs after every feature -- debugging compiled output.
>

Agree, and that's a very real benefit at least until we implement easy
sourcemaps-driven debugging. Worth keeping for now.


>
>
>> or we implement insertAt().
>>
>
> I thought of this, but I'd have to keep track of indexes. Added API anyway.
>

Nice.


>
>
>>
>>
>>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:82:
>> builder.appendAllJs(resp.build());
>> this semantically belongs in if (neededExportJs), which sort of
>> underscores that "neededExportJs" might be unnecessary
>>
>
> Restructured. Done.
>
>
>>
>>
>>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:105:
>> return JsContent.fromFeature(sb.toString(),
>> "[generated-symbol-exports]",
>> this will add a meaningless empty string if there are no exports needed;
>> we should avoid that.
>>
>>
>> Was maintaining status-quo from existing file. Improved now nevertheless.
>

reviewing..


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

[johnfargo, 2011-04-05 01:38:59]

LGTM

On 2011/04/05 01:35:38, fargo wrote:
> On Mon, Apr 4, 2011 at 6:30 PM, Michael Hermanto <mhermanto@gmail.com>wrote:
> 
> >
> >
> > On Mon, Apr 4, 2011 at 3:35 PM, <mailto:johnfargo@gmail.com> wrote:
> >
> >>
> >>
> >>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
> >> File
> >>
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java
> >> (right):
> >>
> >>
> >>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:75:
> >> neededExportJs = true;
> >> it's not clear to me this ever happens, since we error out when
> >> receiving a pipeline request that has no content. granted, as a generic
> >> pipeline it's theoretically possible.
> >>
> >
> > If none of the requests are feature-based, ie: all are
> > JsContent.fromText().
> >
> 
> Yeah, that situation is weird but I guess it's probably innocuous to account
> for it.
> 
> 
> >
> >
> >>
> >>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:80:
> >> builder.appendAllJs(getExportJsContents());
> >> two ideas for simplification:
> >> 1) Just introduce prependJs() for exportJsContents(). It's an easy
> >> enough API to offer.
> >>
> >
> > Added/used, but does not matter here since it's clearJs()'ed.
> >
> >
> >> 2) the processor model becomes a whole lot easier if we either put all
> >> exports at the end,
> >
> >
> > Yes, we talked about this. Features don't really share namespace so we
> > don't get the shorter exportJs statements benefit. I see one nice thing of
> > having exportJs after every feature -- debugging compiled output.
> >
> 
> Agree, and that's a very real benefit at least until we implement easy
> sourcemaps-driven debugging. Worth keeping for now.
> 
> 
> >
> >
> >> or we implement insertAt().
> >>
> >
> > I thought of this, but I'd have to keep track of indexes. Added API anyway.
> >
> 
> Nice.
> 
> 
> >
> >
> >>
> >>
> >>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:82:
> >> builder.appendAllJs(resp.build());
> >> this semantically belongs in if (neededExportJs), which sort of
> >> underscores that "neededExportJs" might be unnecessary
> >>
> >
> > Restructured. Done.
> >
> >
> >>
> >>
> >>
>
http://codereview.appspot.com/4352049/diff/1/java/gadgets/src/main/java/org/a...
> >>
> >>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:105:
> >> return JsContent.fromFeature(sb.toString(),
> >> "[generated-symbol-exports]",
> >> this will add a meaningless empty string if there are no exports needed;
> >> we should avoid that.
> >>
> >>
> >> Was maintaining status-quo from existing file. Improved now nevertheless.
> >
> 
> reviewing..
> 
> 
> >
> >
> >
> >>  http://codereview.appspot.com/4352049/
> >>
> >
> >

[johnfargo, 2011-04-05 01:39:37]

http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java
(right):

http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:66:
neededExportJs |= appendExportJsStatementsForFeature(resp, jsUri, last);
nit: while it's functionally equivalent, I'd choose to use boolean-or (||=)
rather than bitwise-or.

http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java
(right):

http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java:69:
public JsResponseBuilder insertJsAt(int index, JsContent jsContent) {
maybe a TODO to implement ExportJsProcessor in terms of prepend/insert to avoid
a full copy? If you don't like the idea the clear/append model works...

[mhermanto, 2011-04-05 01:45:52]

On Mon, Apr 4, 2011 at 6:39 PM, <johnfargo@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java
> (right):
>
>
>
http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java:66:
> neededExportJs |= appendExportJsStatementsForFeature(resp, jsUri, last);
> nit: while it's functionally equivalent, I'd choose to use boolean-or
> (||=) rather than bitwise-or.
>
>
Good point. Overlook on my side.


>
>
http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java
> (right):
>
>
>
http://codereview.appspot.com/4352049/diff/1004/java/gadgets/src/main/java/or...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsResponseBuilder.java:69:
> public JsResponseBuilder insertJsAt(int index, JsContent jsContent) {
> maybe a TODO to implement ExportJsProcessor in terms of prepend/insert
> to avoid a full copy? If you don't like the idea the clear/append model
> works...


I'm fine with it. :)


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