Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(89)

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

Messages

Total messages: 6
mhermanto
14 years, 7 months ago (2011-08-02 18:51:56 UTC) #1
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
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
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/>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b