Sorry this took so long -- its a lot of code. In general it looks pretty
good - mostly just minor nits and a couple clarifications requested, plus
one potentially significant issue.
I would like to see example payloads in comments (not sure the best place to
put it -- maybe in the CommandSink implementations, SimplePayloadDecoder,
etc).
*
TestSetValidator.java, others*
I am getting a checkstyle error here on the import order. Eclipse doesn't
reorder them, but checkstyle requires that static imports come at the end of
each group of imports and ordered among themselves. I am attaching a patch
to gwt.importorder which gets close, but it doesn't exactly match checkstyle
(if there are static and non-static imports in the same prefix, they will be
separated by Eclipse but Checkstyle wants them in the same group). After
applying the patch, Ctrl-Shift-O will rearrange imports in the order
Checkstyle is happy with. This also needs to be done in the other files
where static imports are used.
*HybridServiceServlet.java*
writeResponse is private and unused, so it needs to have
@SuppressWarnings("unused") // referenced by JSNI added.
*CustomFieldSerializerTest.java*
Why does this take 10x as long? The build already takes long enough and if
there is an issue getting to the server I would rather it fail in quicker
than 50s. How about 10s if it needs to be increased?
*InheritanceTest.java*
I know it is existing code, but I would change the anonymous subclasses of
AsyncCallback to be AsyncCallback<Object> to avoid raw type warnings.
*CustomFieldSerializerTestSetValidator.java*
line 42: where is it tested that we don't see this STE?
*RpcProxyCreator.java*
- writeSerializationPolicyFile - will this get called? It looks like
ProxyCreator will wind up creating a string "null" if so -- is that an
issue?
- Can you give an overview of what is going on with the artificial
rescues here?
*RPC.java*
- line 55: add a comment explaining the space is needed to avoid
potential collisions
- implementsInterfaceRecursive: perhaps "Recursive helper for
implementsInterface()" would be a more useful comment.
*SimplePayloadSink.java*
- line 231: would it be useful to tie this to -style PRETTY? Like maybe
provide a static setter and have generated code insert a call to it in
PRETTY mode?
- append: where is quoting the RPC_SEPARATOR_CHAR handled if it is
contained within the string?
- I don't see where all the quoting is handled like
ServerSerializationStreamReader deserializeStringTable (and others) did
before -- various browsers have issues with different characters (current
Android doesn't handle any non-ASCII due to a double-decode problem), and
while I am not looking at the web-mode code with OOPHM (and hopefully
eventually an Android plugin among others) I still think this needs to be
solved here. Maybe I am missing it since otherwise I don't see how
UnicodeEscapingTest2 could pass on all browsers.
*SimplePayloadDecoder.java*
- same comment about quoting and encoding
- missing newline at EOF
*HasSetters.java, HasValues.java, **ClientOracle.java**,
AbstractRemoteServiceServlet.java *
missing newline at EOF
*RpcRequestBuilder.java*
line 44: not sure I see the value in this comment, as the IDE will do that
for you. I know you were just keeping it like the existing code, but I
would remove all of them.
--
John A. Tamplin
Software Engineer (GWT), Google
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
Index: eclipse/settings/code-style/gwt.importorder
===================================================================
--- eclipse/settings/code-style/gwt.importorder (revision 5639)
+++ eclipse/settings/code-style/gwt.importorder (working copy)
@@ -1,9 +1,16 @@
#Organize Import Order
-#Thu Jul 20 15:01:42 EDT 2006
-6=javax
-5=java
-4=org
-3=net
-2=junit
-1=com
+#Tue Jun 30 09:38:40 GMT-05:00 2009
+9=\#org
+8=org
+7=\#net
+6=net
+5=\#junit
+13=\#javax
+4=junit
+12=javax
+3=\#com
+11=\#java
+2=com
+10=java
+1=\#com.google
0=com.google
