LGTM. Good catches. The addition of tests is great. I did a quick review and things look okay. Anyway, I will reviewing this code more carefully over the next few days, as I refactor it and use it in Htmlunit plugin.
http://gwt-code-reviews.appspot.com/51835/diff/4011/4014 File dev/oophm/src/com/google/gwt/dev/shell/BrowserChannel.java (right): http://gwt-code-reviews.appspot.com/51835/diff/4011/4014#newcode709 Line 709: protected static class InvokeOnClientMessage extends Message { Great. On 2009/08/07 00:26:56, jat wrote: > Split this into different messages for each direction. Not only does this allow > testing of the Java code, but it will be needed for the hosted mode HTMLUnit > client as well. http://gwt-code-reviews.appspot.com/51835/diff/4011/4014#newcode712 Line 712: DataInputStream stream = channel.getStreamFromOtherSide(); Previous code used 'final' and there are other places in the code where 'final' is used. http://gwt-code-reviews.appspot.com/51835/diff/4011/4014#newcode754 Line 754: writeUntaggedString(stream, methodName); why are the method names not symmetric? readUtf8String vs. writeUntaggedString http://gwt-code-reviews.appspot.com/51835/diff/4011/4014#newcode1026 Line 1026: For symmetry and clarity, why not have a writeMessageType method that takes care of writing the tag? The write method in each method subclass can then just take care of writing everything else. This refactoring does not need to be done immediately. http://gwt-code-reviews.appspot.com/51835 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
