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: <!-- This can be
used to make ExternalTextResource use JSONP rather than XHR -->
... 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
<dobule-quote><single-quote><backslash> 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

Reply via email to