http://gwt-code-reviews.appspot.com/1310801/diff/1/3 File user/src/com/google/gwt/jsonp/client/JsonpRequest.java (right):
http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode70 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:70: } On 2011/01/24 14:31:26, bobv wrote:
Extra whitespace. GPE usually prevents formatting weirdness.
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode75 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:75: // The "P" suffix must stay in sync with ExternalTextResourceGenerator.java On 2011/01/24 14:31:26, bobv wrote:
Use a shared constant field?
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode149 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:149: String id) { On 2011/01/24 14:31:26, bobv wrote:
Formatting.
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode219 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:219: return callbackId.startsWith("P"); On 2011/01/24 14:31:26, bobv wrote:
This seems kind of hacky. Is there a way to do this in a way that
would prevent
someone from accidentally triggering this behavior with a
poorly-chosen prefix? Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode279 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:279: if (!canHaveMultipleRequestsForId) { On 2011/01/24 14:31:26, bobv wrote:
Could you reverse the order of the then and else clauses to prevent a
negative
comparison?
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode289 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:289: if (callbackWrapper == null) { On 2011/01/24 14:31:26, bobv wrote:
if (!callbackWrapper) is more idiomatic.
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/5 File user/src/com/google/gwt/resources/Resources.gwt.xml (right): http://gwt-code-reviews.appspot.com/1310801/diff/1/5#newcode83 user/src/com/google/gwt/resources/Resources.gwt.xml:83: <!-- This can be used to make ExternalTextResource use JSONP rather than XHR --> On 2011/01/24 14:31:26, bobv wrote:
Why not always use JSONP? It's guaranteed to work across more
deployment
schemes than XHR, right?
Mostly because I'm just a little nervous about changing the default behavior. I could be convinced if you feel strongly... Another option would be to leave it as is for now, and then flip the default value for the property (or remove the XHR implementation entirely if you feel strongly) once AW3 is successfully using this version. http://gwt-code-reviews.appspot.com/1310801/diff/1/6 File user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java (right): http://gwt-code-reviews.appspot.com/1310801/diff/1/6#newcode87 user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java:87: }; On 2011/01/24 14:31:26, bobv wrote:
Extra whitespace.
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/6#newcode170 user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java:170: // } On 2011/01/24 14:31:26, bobv wrote:
Delete this?
Actually - it should stay - I just commented it out for some debugging - good catch! http://gwt-code-reviews.appspot.com/1310801/diff/1/6#newcode172 user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java:172: if (!"".equals(md5Hash)) { On 2011/01/24 14:31:26, bobv wrote:
How about a null comparison instead?
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/7 File user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java (right): http://gwt-code-reviews.appspot.com/1310801/diff/1/7#newcode54 user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:54: static final String JSONP_CALLBACK_PREFIX = "__gwt_jsonp__.P"; On 2011/01/24 14:31:26, bobv wrote:
If the .P is special, it should be in a constant field.
We discussed this in the previous CL - the problem is that if I want to make shared with the other file, then I cross the user/dev dependency boundary, so you said to go ahead and just add a comment to keep them in sync http://gwt-code-reviews.appspot.com/1310801/diff/1/7#newcode184 user/src/com/google/gwt/resources/rg/ExternalTextResourceGenerator.java:184: if (Boolean.parseBoolean(useJsonpProp)) { On 2011/01/24 14:31:26, bobv wrote:
How about
return Boolean.parseBoolean(useJsonProp);
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/11 File user/test/com/google/gwt/resources/client/ExternalTextResourceJsonpTest.java (right): http://gwt-code-reviews.appspot.com/1310801/diff/1/11#newcode2 user/test/com/google/gwt/resources/client/ExternalTextResourceJsonpTest.java:2: * Copyright 2010 Google Inc. On 2011/01/24 14:31:26, bobv wrote:
2011
Done. http://gwt-code-reviews.appspot.com/1310801/diff/1/12 File user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java (right): http://gwt-code-reviews.appspot.com/1310801/diff/1/12#newcode2 user/test/com/google/gwt/resources/client/ExternalTextResourceTest.java:2: * Copyright 2008 Google Inc. On 2011/01/24 14:31:26, bobv wrote:
2011
Done. http://gwt-code-reviews.appspot.com/1310801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
