Mostly LG, but I don't really get why UmbrellaException is rolled into this. Actually, I think I mean that two different ways.
1) I don't understand why the UmbrellaException stuff is mixed into this patch. Unrelated changes should be in their own patches. 2) I don't understand why the UmbrellaException tests are rolled into the RPC test suites at all. Seems like it should be in some tests in the event package. http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/src/com/google/gwt/event/shared/UmbrellaException.java File user/src/com/google/gwt/event/shared/UmbrellaException.java (right): http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/src/com/google/gwt/event/shared/UmbrellaException.java#newcode38 user/src/com/google/gwt/event/shared/UmbrellaException.java:38: this.causes = new HashSet<Throwable>(); Instead of duplicating code, do "this(new HashSet<Throwable>()). http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/ExceptionsTestService.java File user/test/com/google/gwt/user/client/rpc/ExceptionsTestService.java (right): http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/ExceptionsTestService.java#newcode23 user/test/com/google/gwt/user/client/rpc/ExceptionsTestService.java:23: public interface ExceptionsTestService extends RemoteService { There's an annotation you can use on this class that keeps you from having to do an explicit ServiceDefTarget.setServiceEntryPoint() call in the test code. http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/ExceptionsTestServiceAsync.java File user/test/com/google/gwt/user/client/rpc/ExceptionsTestServiceAsync.java (right): http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/ExceptionsTestServiceAsync.java#newcode2 user/test/com/google/gwt/user/client/rpc/ExceptionsTestServiceAsync.java:2: * Copyright 2008 Google Inc. Update copyright year. I think some other new files had this too. http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/TestSetValidator.java File user/test/com/google/gwt/user/client/rpc/TestSetValidator.java (right): http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/TestSetValidator.java#newcode287 user/test/com/google/gwt/user/client/rpc/TestSetValidator.java:287: } A nice pattern for this goes something like, if ((expected == null) != (actual == null)) { return false; } http://gwt-code-reviews.appspot.com/1395804/diff/8001/user/test/com/google/gwt/user/client/rpc/TestSetValidator.java#newcode429 user/test/com/google/gwt/user/client/rpc/TestSetValidator.java:429: /* Check here for null keys or values to avoid repeated checks later. */ This section of code doesn't seem to hold its weight. You're always using non-null data right? So don't bother with the null checks and if something goes drastically wrong, you'll get an NPE, which will tell you that something went drastically wrong. http://gwt-code-reviews.appspot.com/1395804/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
