[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-15 Thread unnurg 

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-15 Thread unnurg 


http://gwt-code-reviews.appspot.com/1214801/diff/20001/21004
File user/src/com/google/gwt/resources/Resources.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21004#newcode83
user/src/com/google/gwt/resources/Resources.gwt.xml:83: 
On 2010/12/15 12:51:57, bobv wrote:

... by setting the value to "true"


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21005
File
user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21005#newcode138
user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java:138:
TextResource[] cache, int index, String md5Hash, boolean useJsonp) {
On 2010/12/15 12:51:57, bobv wrote:

The presence of md5Hash implies useJsonp.  It would be better to add

the jsonp

variant as a separate constructor.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21007
File user/test/com/google/gwt/jsonp/client/JsonpRequestTest.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21007#newcode239
user/test/com/google/gwt/jsonp/client/JsonpRequestTest.java:239: public
void testPredeterminedIds() {
On 2010/12/15 12:51:57, bobv wrote:

Sort order.  It's not enforced by the checkstyle rules in test code.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21009
File user/test/com/google/gwt/resources/ResourcesSuite.java (right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21009#newcode62
user/test/com/google/gwt/resources/ResourcesSuite.java:62:
suite.addTestSuite(UnknownAtRuleTest.class);
On 2010/12/15 12:51:57, bobv wrote:

The order of these tests doesn't matter, can you sort the

addTestSuite() while

you're in the area?


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21011
File
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21011#newcode26
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:26:
static interface Resources extends ClientBundleWithLookup {
On 2010/12/15 12:51:57, bobv wrote:

Redundant modifier, interfaces are implicitly static.



To make sure the bundling is working correctly, there should be more

than one

ExternalTextResource in the ClientBundle.  Also, make sure that the

JSON

escaping is working correctly by adding a file containing  characters

that must

be escaped.  A sequence like 

should be

sufficient.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21011#newcode59
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:59:

On 2010/12/15 12:51:57, bobv wrote:

Extra blank line.


Done.

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-15 Thread bobv 

Implementation LGTM, just expand the test a bit and commit.


http://gwt-code-reviews.appspot.com/1214801/diff/20001/21004
File user/src/com/google/gwt/resources/Resources.gwt.xml (right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21004#newcode83
user/src/com/google/gwt/resources/Resources.gwt.xml:83: 
... by setting the value to "true"

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21005
File
user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21005#newcode138
user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java:138:
TextResource[] cache, int index, String md5Hash, boolean useJsonp) {
The presence of md5Hash implies useJsonp.  It would be better to add the
jsonp variant as a separate constructor.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21006
File
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21006#newcode79
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:79:
sw.println("\"" + getMd5HashOfData() + "\", ");
FYI only. Since ETRG was written, the SourceWriter.println() method now
has a printf-style overload.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21007
File user/test/com/google/gwt/jsonp/client/JsonpRequestTest.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21007#newcode239
user/test/com/google/gwt/jsonp/client/JsonpRequestTest.java:239: public
void testPredeterminedIds() {
Sort order.  It's not enforced by the checkstyle rules in test code.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21009
File user/test/com/google/gwt/resources/ResourcesSuite.java (right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21009#newcode62
user/test/com/google/gwt/resources/ResourcesSuite.java:62:
suite.addTestSuite(UnknownAtRuleTest.class);
The order of these tests doesn't matter, can you sort the addTestSuite()
while you're in the area?

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21011
File
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21011#newcode26
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:26:
static interface Resources extends ClientBundleWithLookup {
Redundant modifier, interfaces are implicitly static.

To make sure the bundling is working correctly, there should be more
than one ExternalTextResource in the ClientBundle.  Also, make sure that
the JSON escaping is working correctly by adding a file containing
characters that must be escaped.  A sequence like
 should be sufficient.

http://gwt-code-reviews.appspot.com/1214801/diff/20001/21011#newcode59
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:59:

Extra blank line.

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-14 Thread jat 

LGTM

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-14 Thread unnurg 


http://gwt-code-reviews.appspot.com/1214801/diff/1/2
File user/src/com/google/gwt/jsonp/client/JsonpRequest.java (right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode33
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:33:
On 2010/12/13 21:42:50, jat wrote:

Added whitespace.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode78
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:78:
On 2010/12/13 21:42:50, jat wrote:

Whitespace.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode129
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:129: * Create a
new JSONP request.
On 2010/12/13 21:42:50, jat wrote:

Shouldn't this distinguish from the above constructor, explaining why

this would

be used?


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode142
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:142: String
callbackParam, String failureCallbackParam, String id) {
On 2010/12/13 21:42:50, jat wrote:

Need javadoc for id.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/6
File
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/6#newcode67
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:67:

On 2010/12/13 21:42:50, jat wrote:

Whitespace.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/6#newcode91
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:91:

On 2010/12/13 21:42:50, jat wrote:

Whitespace.


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/6#newcode154
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:154:

On 2010/12/13 21:42:50, jat wrote:

Whitespace.



Something must be set wrong in your IDE to generate all these where

they didn't

exist previously.


It seems like it doesn't it? I have the gwt formatting stuff imported
though, and as far as I can tell, indent is not checked for blank
lines...

http://gwt-code-reviews.appspot.com/1214801/diff/1/8
File
user/test/com/google/gwt/resources/ExternalTextResourceJsonp.gwt.xml
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/8#newcode16
user/test/com/google/gwt/resources/ExternalTextResourceJsonp.gwt.xml:16:

On 2010/12/13 21:42:50, jat wrote:

80 columns


Done.

http://gwt-code-reviews.appspot.com/1214801/diff/1/11
File
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/11#newcode26
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:26:
static interface Resources extends ClientBundleWithLookup {
On 2010/12/13 21:42:50, jat wrote:

Why package protected?


Done.

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-14 Thread unnurg 

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-14 Thread jat 

LGTM


http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-14 Thread unnurg 

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-14 Thread unnurg 

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

[gwt-contrib] Re: Make ExternalTextResource use Jsonp (issue1214801)

2010-12-13 Thread jat 

Mostly LGTM


http://gwt-code-reviews.appspot.com/1214801/diff/1/2
File user/src/com/google/gwt/jsonp/client/JsonpRequest.java (right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode33
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:33:
Added whitespace.

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode78
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:78:
Whitespace.

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode129
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:129: * Create a
new JSONP request.
Shouldn't this distinguish from the above constructor, explaining why
this would be used?

http://gwt-code-reviews.appspot.com/1214801/diff/1/2#newcode142
user/src/com/google/gwt/jsonp/client/JsonpRequest.java:142: String
callbackParam, String failureCallbackParam, String id) {
Need javadoc for id.

http://gwt-code-reviews.appspot.com/1214801/diff/1/3
File user/src/com/google/gwt/jsonp/client/JsonpRequestBuilder.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/3#newcode203
user/src/com/google/gwt/jsonp/client/JsonpRequestBuilder.java:203:
failureCallbackParam, predeterminedId);
Would it be cleaner to just pass predeterminedId to the constructor, and
have existing behavior if it is null?  That would also remove
duplication in the constructors.

http://gwt-code-reviews.appspot.com/1214801/diff/1/6
File
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/6#newcode67
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:67:

Whitespace.

http://gwt-code-reviews.appspot.com/1214801/diff/1/6#newcode91
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:91:

Whitespace.

http://gwt-code-reviews.appspot.com/1214801/diff/1/6#newcode154
user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:154:

Whitespace.

Something must be set wrong in your IDE to generate all these where they
didn't exist previously.

http://gwt-code-reviews.appspot.com/1214801/diff/1/8
File
user/test/com/google/gwt/resources/ExternalTextResourceJsonp.gwt.xml
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/8#newcode16
user/test/com/google/gwt/resources/ExternalTextResourceJsonp.gwt.xml:16:

80 columns

http://gwt-code-reviews.appspot.com/1214801/diff/1/11
File
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java
(right):

http://gwt-code-reviews.appspot.com/1214801/diff/1/11#newcode26
user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:26:
static interface Resources extends ClientBundleWithLookup {
Why package protected?

http://gwt-code-reviews.appspot.com/1214801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors