Why maintain both modes of operation? There's more code and more stuff to test when JSONP is a more general solution than XHR.
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: } Extra whitespace. GPE usually prevents formatting weirdness. 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 Use a shared constant field? http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode149 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:149: String id) { Formatting. 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"); 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? http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode279 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:279: if (!canHaveMultipleRequestsForId) { Could you reverse the order of the then and else clauses to prevent a negative comparison? http://gwt-code-reviews.appspot.com/1310801/diff/1/3#newcode289 user/src/com/google/gwt/jsonp/client/JsonpRequest.java:289: if (callbackWrapper == null) { if (!callbackWrapper) is more idiomatic. 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 --> Why not always use JSONP? It's guaranteed to work across more deployment schemes than XHR, right? 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: }; Extra whitespace. http://gwt-code-reviews.appspot.com/1310801/diff/1/6#newcode170 user/src/com/google/gwt/resources/client/impl/ExternalTextResourcePrototype.java:170: // } Delete this? 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)) { How about a null comparison instead? 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"; If the .P is special, it should be in a constant field. 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)) { How about return Boolean.parseBoolean(useJsonProp); 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. 2011 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. 2011 http://gwt-code-reviews.appspot.com/1310801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
