LGTM, just a few style nits.

http://gwt-code-reviews.appspot.com/437801/diff/8001/9003
File /dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java
(right):

http://gwt-code-reviews.appspot.com/437801/diff/8001/9003#newcode33
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:33:
protected static class LogManagerWithExposedConstructor extends
LogManager {
Checkstyle wants this to have Javadoc since it's protected.  Or you
could just make it default access.

http://gwt-code-reviews.appspot.com/437801/diff/8001/9003#newcode42
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:42: return
new LogManagerWithExposedConstructor();
Nice... I totally forgot about this trick. :)

http://gwt-code-reviews.appspot.com/437801/diff/8001/9003#newcode52
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:52:
serverLogManager = new LogManagerWithExposedConstructor();
Would it do any good to log the error to the newly-created logger?  Or
does it just go into the bit bucket if there are no handlers set up yet?

http://gwt-code-reviews.appspot.com/437801/diff/8001/9003#newcode73
/dev/core/src/com/google/gwt/dev/shell/DevModeLogManager.java:73: public
Enumeration getLoggerNames() {
Return Enumeration<String> instead of the unchecked.

http://gwt-code-reviews.appspot.com/437801/diff/8001/9004
File /dev/core/test/com/google/gwt/dev/DevModeBaseTest.java (right):

http://gwt-code-reviews.appspot.com/437801/diff/8001/9004#newcode23
/dev/core/test/com/google/gwt/dev/DevModeBaseTest.java:23: static final
String NEW_MANAGER = "com.google.gwt.dev.shell.DevModeLogManager";
Nit: wrong sort order for OLD/NEW. :)

http://gwt-code-reviews.appspot.com/437801/diff/8001/9005
File /dev/core/test/com/google/gwt/dev/shell/DevModeLogManagerTest.java
(right):

http://gwt-code-reviews.appspot.com/437801/diff/8001/9005#newcode46
/dev/core/test/com/google/gwt/dev/shell/DevModeLogManagerTest.java:46:
assertTrue(devManager.serverLogManager instanceof LogManager);
Eclipse warns me that the instanceof test cannot possibly be false.  Are
you just null-checking, or are you looking for a specific type?

http://gwt-code-reviews.appspot.com/437801/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to