Version 2 of your changes looks pretty good. I only had some small nits. You should have someone else review the JunitShell changes; I'm not familiar with that system at all.
I question whether some of these classes should be in the dev.shell package. It seems like we've got a mishmash of related functionality scattered between the dev and dev.shell package; the line definitely seems blurry here. http://gwt-code-reviews.appspot.com/77817/diff/1/35 File dev/core/src/com/google/gwt/dev/DevModeUI.java (right): http://gwt-code-reviews.appspot.com/77817/diff/1/35#newcode32 Line 32: * Callback interface for events from the UI. Nit: instead of "from" the UI, you could say "initiated by" the UI. http://gwt-code-reviews.appspot.com/77817/diff/1/35#newcode81 Line 81: protected Type logLevel; Could make this private, and expose the value via a protected getter. http://gwt-code-reviews.appspot.com/77817/diff/1/35#newcode146 Line 146: public void setCallback(String event, Callback callback) { Could this be final? http://gwt-code-reviews.appspot.com/77817/diff/1/35#newcode164 Line 164: protected void callback(String event, Object callbackData) { Could this be final? http://gwt-code-reviews.appspot.com/77817/diff/1/35#newcode174 Line 174: protected TreeLogger getConsoleLogger() { Could this be final? http://gwt-code-reviews.appspot.com/77817/diff/1/35#newcode188 Line 188: protected boolean hasCallback(String event) { Could this be final? http://gwt-code-reviews.appspot.com/77817/diff/1/30 File dev/core/src/com/google/gwt/dev/HostedModeBase.java (right): http://gwt-code-reviews.appspot.com/77817/diff/1/30#newcode69 Line 69: public class UiBrowserWidgetHostImpl extends BrowserWidgetHostImpl { Add some javadoc here. http://gwt-code-reviews.appspot.com/77817/diff/1/36 File dev/core/src/com/google/gwt/dev/SwingUI.java (right): http://gwt-code-reviews.appspot.com/77817/diff/1/36#newcode107 Line 107: static ImageIcon loadImageIcon(String name, boolean prependPackage) { Document the return behavior. http://gwt-code-reviews.appspot.com/77817/diff/1/36#newcode229 Line 229: tab.disconnect(); Is this thread-safe? http://gwt-code-reviews.appspot.com/77817/diff/1/36#newcode247 Line 247: return ++sessionCounter; Is this thread-safe? http://gwt-code-reviews.appspot.com/77817/diff/1/19 File dev/oophm/src/com/google/gwt/dev/shell/BrowserChannel.java (right): http://gwt-code-reviews.appspot.com/77817/diff/1/19#newcode1332 Line 1332: stream.flush(); spurious indent? http://gwt-code-reviews.appspot.com/77817/diff/1/4 File user/src/com/google/gwt/junit/BatchingStrategy.java (right): http://gwt-code-reviews.appspot.com/77817/diff/1/4#newcode51 Line 51: // TODO(jat): merge problem? Not sure what this refers to. http://gwt-code-reviews.appspot.com/77817/diff/1/6 File user/src/com/google/gwt/junit/JUnitShell.java (right): http://gwt-code-reviews.appspot.com/77817/diff/1/6#newcode2 Line 2: * Copyright 2008 Google Inc. Might be good to have someone else review this file; I'm not very familiar with this part of the code. http://gwt-code-reviews.appspot.com/77817 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
