Issue 4841047: Correcting [Export|Defer]JsProcessor

Can't Edit
Can't Publish+Mail
Start Review

Created:
14 years, 7 months ago by mhermanto

Modified:
14 years, 7 months ago

Reviewers:
johnfargo, Stanton, fargo

CC:
rbaxter85

Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/

Visibility:
Public.

Description

- now calls more-compact-expressed deferJs(). - pull up common functions into BaseSurfaceJsProcessor. - test clean-ups. Providing early review; Shindig tests passed, but not Google size tests.

Patch Set 1 : Update patch #

Total comments: 9

Created: 14 years, 7 months ago

Download [raw] [tar.bz2]

Unified diffs	Side-by-side diffs	Stats (+295 lines, -192 lines)			Patch
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java	View	1 chunk	+158 lines, -0 lines	1 comment	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java	View	2 chunks	+62 lines, -23 lines	5 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java	View	2 chunks	+35 lines, -116 lines	1 comment	Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DeferJsProcessorTest.java	View	4 chunks	+13 lines, -10 lines	1 comment	Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/js/ExportJsProcessorTest.java	View	11 chunks	+27 lines, -43 lines	1 comment	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

johnfargo

http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java File java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java (right): http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java#newcode46 java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java:46: @Inject nit: since it's base, @Inject does nothing http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java ...

14 years, 7 months ago (2011-08-02 20:39:36 UTC) #2

mhermanto

On Tue, Aug 2, 2011 at 1:39 PM, <johnfargo@gmail.com> wrote: > > http://codereview.appspot.com/**4841047/diff/3001/java/** > gadgets/src/main/java/org/**apache/shindig/gadgets/js/** ...

14 years, 7 months ago (2011-08-02 21:06:39 UTC) #3

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

>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
BaseSurfaceJsProcessor.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java>
> File
>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> BaseSurfaceJsProcessor.java
> (right):
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
BaseSurfaceJsProcessor.java#**newcode46<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java#newcode46>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> BaseSurfaceJsProcessor.java:**46:
> @Inject
> nit: since it's base, @Inject does nothing
>

Removed. made protected.


>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessor.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java>
> File
>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessor.java
> (right):
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessor.java#**newcode54<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode54>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessor.java:54:
> FeatureBundle last = null;
> now unused
>

Done.

>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessor.java#**newcode63<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode63>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessor.java:63:
> // TODO: Instead of clearing, do manual deletion of the actual feature
> impl.
> the actual feature impl shouldn't be there since the whole thing is
> predicated on jsUri.isJsload(), right?
>
> Clearing has a side-effect of ignoring previous processors work. I find
this (defer-case) a little funny as implemented, because the we first
collect the feature impl, then we're ignoring that work and replace them
with stubs. I've refined the TODO.


> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessor.java#**newcode81<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode81>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessor.java:81:
> "[generated-symbol-exports]", bundle, null));
> this could probably fit on the line above?
>

101 chars.


>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessor.java#**newcode93<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode93>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessor.java:93:
> for (int i = 0; i < input.properties.size(); i++) {
> maybe just iterate  for (String prop : input.properties)?
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessor.java#**newcode95<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode95>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessor.java:95:
> if (i > 0) result.append(",");
> Joiner.on(",")?
>

Before I join, I need to quote it, as string. Copi'ed and paste'ed from
ExportJsProcessor.

>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
ExportJsProcessor.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java>
> File
>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> ExportJsProcessor.java
> (right):
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>
ExportJsProcessor.java#**newcode117<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java#newcode117>
> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
> ExportJsProcessor.java:117:
> // Global/window namespace.
> for both this and deferred, why use null for namespace, rather than
> having an empty input.properties?


null namespace to mean global/window. namespace just means where the
properties should start to be declared under, and the what goes to what,
much depends on what exportJs() accepts/treats. I'm maintaining status-quo
here from before.


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

johnfargo

LGTM++ Aka "Ship It!" On Tue, Aug 2, 2011 at 2:06 PM, Michael Hermanto <mhermanto@gmail.com>wrote: ...

14 years, 7 months ago (2011-08-02 21:10:45 UTC) #4

LGTM++

Aka "Ship It!"

On Tue, Aug 2, 2011 at 2:06 PM, Michael Hermanto <mhermanto@gmail.com>wrote:

>
>
> On Tue, Aug 2, 2011 at 1:39 PM, <johnfargo@gmail.com> wrote:
>
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
BaseSurfaceJsProcessor.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java>
>> File
>>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> BaseSurfaceJsProcessor.java
>> (right):
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
BaseSurfaceJsProcessor.java#**newcode46<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/BaseSurfaceJsProcessor.java#newcode46>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> BaseSurfaceJsProcessor.java:**46:
>> @Inject
>> nit: since it's base, @Inject does nothing
>>
>
> Removed. made protected.
>
>
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
DeferJsProcessor.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java>
>> File
>>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> DeferJsProcessor.java
>> (right):
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
DeferJsProcessor.java#**newcode54<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode54>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> DeferJsProcessor.java:54:
>> FeatureBundle last = null;
>> now unused
>>
>
> Done.
>
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
DeferJsProcessor.java#**newcode63<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode63>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> DeferJsProcessor.java:63:
>> // TODO: Instead of clearing, do manual deletion of the actual feature
>> impl.
>> the actual feature impl shouldn't be there since the whole thing is
>> predicated on jsUri.isJsload(), right?
>>
>> Clearing has a side-effect of ignoring previous processors work. I find
> this (defer-case) a little funny as implemented, because the we first
> collect the feature impl, then we're ignoring that work and replace them
> with stubs. I've refined the TODO.
>
>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
DeferJsProcessor.java#**newcode81<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode81>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> DeferJsProcessor.java:81:
>> "[generated-symbol-exports]", bundle, null));
>> this could probably fit on the line above?
>>
>
> 101 chars.
>
>
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
DeferJsProcessor.java#**newcode93<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode93>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> DeferJsProcessor.java:93:
>> for (int i = 0; i < input.properties.size(); i++) {
>> maybe just iterate  for (String prop : input.properties)?
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
DeferJsProcessor.java#**newcode95<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DeferJsProcessor.java#newcode95>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> DeferJsProcessor.java:95:
>> if (i > 0) result.append(",");
>> Joiner.on(",")?
>>
>
> Before I join, I need to quote it, as string. Copi'ed and paste'ed from
> ExportJsProcessor.
>

.append("'").append(Joiner.on("','").join(components)).append("'")? :)


>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
ExportJsProcessor.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java>
>> File
>>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> ExportJsProcessor.java
>> (right):
>>
>> http://codereview.appspot.com/**4841047/diff/3001/java/**
>> gadgets/src/main/java/org/**apache/shindig/gadgets/js/**
>>
ExportJsProcessor.java#**newcode117<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/ExportJsProcessor.java#newcode117>
>> java/gadgets/src/main/java/**org/apache/shindig/gadgets/js/**
>> ExportJsProcessor.java:117:
>> // Global/window namespace.
>> for both this and deferred, why use null for namespace, rather than
>> having an empty input.properties?
>
>
> null namespace to mean global/window. namespace just means where the
> properties should start to be declared under, and the what goes to what,
> much depends on what exportJs() accepts/treats. I'm maintaining status-quo
> here from before.
>
>
>>
>>
http://codereview.appspot.com/**4841047/<http://codereview.appspot.com/4841047/>
>>
>
>

Stanton

Found some build failures in a Java5 build this morning. It is related to these ...

14 years, 7 months ago (2011-08-03 11:23:53 UTC) #5

fargo

14 years, 7 months ago (2011-08-03 19:08:26 UTC) #6

Thanks Stanton, fix committed in r1153614.

On Wed, Aug 3, 2011 at 4:23 AM, <sieverssj@gmail.com> wrote:

> Found some build failures in a Java5 build this morning.  It is related
> to these changes.  I have inline comments pointing out exactly where.
>
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/test/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessorTest.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DeferJsProcessorTest.java>
> File
> java/gadgets/src/test/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessorTest.java
> (right):
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/test/java/org/**apache/shindig/gadgets/js/**
>
DeferJsProcessorTest.java#**newcode85<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DeferJsProcessorTest.java#newcode85>
> java/gadgets/src/test/java/**org/apache/shindig/gadgets/js/**
> DeferJsProcessorTest.java:85:
> @Override
> This breaks the Java5 build because FeatureRegistryProvider is an
> interface.
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/test/java/org/**apache/shindig/gadgets/js/**
>
ExportJsProcessorTest.java<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/ExportJsProcessorTest.java>
> File
> java/gadgets/src/test/java/**org/apache/shindig/gadgets/js/**
> ExportJsProcessorTest.java
> (right):
>
> http://codereview.appspot.com/**4841047/diff/3001/java/**
> gadgets/src/test/java/org/**apache/shindig/gadgets/js/**
>
ExportJsProcessorTest.java#**newcode101<http://codereview.appspot.com/4841047/diff/3001/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/ExportJsProcessorTest.java#newcode101>
> java/gadgets/src/test/java/**org/apache/shindig/gadgets/js/**
> ExportJsProcessorTest.java:**101:
> @Override
> This breaks the Java5 build because FeatureRegistryProvider is an
> interface.
>
>
>
http://codereview.appspot.com/**4841047/<http://codereview.appspot.com/4841047/>
>

Expand All Messages | Collapse All Messages