Looking very nice, just a few nitpicks.
http://gwt-code-reviews.appspot.com/341803/diff/1/10 File user/src/com/google/gwt/logging/client/BasicLoggingPopup.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/10#newcode1 user/src/com/google/gwt/logging/client/BasicLoggingPopup.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/10#newcode17 user/src/com/google/gwt/logging/client/BasicLoggingPopup.java:17: // TODO(unnurg): Make this popup not suck Can you be specific? http://gwt-code-reviews.appspot.com/341803/diff/1/11 File user/src/com/google/gwt/logging/client/ConsoleLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/11#newcode1 user/src/com/google/gwt/logging/client/ConsoleLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/11#newcode8 user/src/com/google/gwt/logging/client/ConsoleLogHandler.java:8: * A Handler that prints logs to the $wnd.console - this is used by by things "by by" http://gwt-code-reviews.appspot.com/341803/diff/1/12 File user/src/com/google/gwt/logging/client/DefaultLevel.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/12#newcode1 user/src/com/google/gwt/logging/client/DefaultLevel.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/12#newcode14 user/src/com/google/gwt/logging/client/DefaultLevel.java:14: @Override public Level getDefaultLevel() { return Level.ALL; } Not sure if our style allows single-line methods -- have you run Checkstyle? http://gwt-code-reviews.appspot.com/341803/diff/1/13 File user/src/com/google/gwt/logging/client/DevelopmentModeLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/13#newcode1 user/src/com/google/gwt/logging/client/DevelopmentModeLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/13#newcode36 user/src/com/google/gwt/logging/client/DevelopmentModeLogHandler.java:36: // Should we pass in the throwable here since GWT.log can handle it? Make this a // TODO? http://gwt-code-reviews.appspot.com/341803/diff/1/14 File user/src/com/google/gwt/logging/client/FirebugLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/14#newcode1 user/src/com/google/gwt/logging/client/FirebugLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/14#newcode8 user/src/com/google/gwt/logging/client/FirebugLogHandler.java:8: * A Handler that prints logs to $wind.console which is used by Firebug $wind -> $wnd http://gwt-code-reviews.appspot.com/341803/diff/1/15 File user/src/com/google/gwt/logging/client/HasWidgetsLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/15#newcode1 user/src/com/google/gwt/logging/client/HasWidgetsLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/16 File user/src/com/google/gwt/logging/client/HtmlLogFormatter.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/16#newcode1 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/17 File user/src/com/google/gwt/logging/client/LogConfiguration.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/17#newcode1 user/src/com/google/gwt/logging/client/LogConfiguration.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/17#newcode49 user/src/com/google/gwt/logging/client/LogConfiguration.java:49: // try to pull the log level from the CGI param Another 'CGI' http://gwt-code-reviews.appspot.com/341803/diff/1/17#newcode80 user/src/com/google/gwt/logging/client/LogConfiguration.java:80: String candidate = s.toUpperCase(); Canonicalizing by case can fail if the client is in a Turkish locale -- the lowercase 'i' gets converted to a dotted uppercase I, and the equality tests for "WARNING', 'INFO', etc. will fail. http://gwt-code-reviews.appspot.com/341803/diff/1/18 File user/src/com/google/gwt/logging/client/NullLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/18#newcode1 user/src/com/google/gwt/logging/client/NullLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/19 File user/src/com/google/gwt/logging/client/NullLoggingPopup.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/19#newcode1 user/src/com/google/gwt/logging/client/NullLoggingPopup.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/20 File user/src/com/google/gwt/logging/client/RemoteLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/20#newcode1 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/21 File user/src/com/google/gwt/logging/client/SystemLogHandler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/21#newcode1 user/src/com/google/gwt/logging/client/SystemLogHandler.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/21#newcode37 user/src/com/google/gwt/logging/client/SystemLogHandler.java:37: System.out.print(msg); I'm assuming the message contains its own newline, is that correct? http://gwt-code-reviews.appspot.com/341803/diff/1/22 File user/src/com/google/gwt/logging/client/TextLogFormatter.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/22#newcode1 user/src/com/google/gwt/logging/client/TextLogFormatter.java:1: package com.google.gwt.logging.client; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/22#newcode18 user/src/com/google/gwt/logging/client/TextLogFormatter.java:18: StringBuffer accum = new StringBuffer(event.getLevel().getName()); Typically StringBuilder is used nowadays rather than StringBuffer (although the difference isn't really significant in client code). http://gwt-code-reviews.appspot.com/341803/diff/1/22#newcode19 user/src/com/google/gwt/logging/client/TextLogFormatter.java:19: accum.append("-" + event.getLoggerName()); Separate appends rather than using + http://gwt-code-reviews.appspot.com/341803/diff/1/23 File user/src/com/google/gwt/logging/impl/LevelImpl.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/23#newcode1 user/src/com/google/gwt/logging/impl/LevelImpl.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/24 File user/src/com/google/gwt/logging/impl/LevelImplNull.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/24#newcode1 user/src/com/google/gwt/logging/impl/LevelImplNull.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/25 File user/src/com/google/gwt/logging/impl/LevelImplRegular.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/25#newcode1 user/src/com/google/gwt/logging/impl/LevelImplRegular.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/25#newcode12 user/src/com/google/gwt/logging/impl/LevelImplRegular.java:12: * need to work around the fact that the Impl class cannot access the protected Line length http://gwt-code-reviews.appspot.com/341803/diff/1/25#newcode40 user/src/com/google/gwt/logging/impl/LevelImplRegular.java:40: public int intValue() { Extra space after 'int' http://gwt-code-reviews.appspot.com/341803/diff/1/26 File user/src/com/google/gwt/logging/impl/LoggerImpl.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/26#newcode1 user/src/com/google/gwt/logging/impl/LoggerImpl.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/26#newcode19 user/src/com/google/gwt/logging/impl/LoggerImpl.java:19: public Handler[] getHandlers(); Need to document that this can return null if there are no handlers. http://gwt-code-reviews.appspot.com/341803/diff/1/27 File user/src/com/google/gwt/logging/impl/LoggerImplNull.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/27#newcode1 user/src/com/google/gwt/logging/impl/LoggerImplNull.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/27#newcode24 user/src/com/google/gwt/logging/impl/LoggerImplNull.java:24: public String getName() {return "";} Does our style allow single-line methods? http://gwt-code-reviews.appspot.com/341803/diff/1/28 File user/src/com/google/gwt/logging/impl/LoggerImplRegular.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/28#newcode1 user/src/com/google/gwt/logging/impl/LoggerImplRegular.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/29 File user/src/com/google/gwt/logging/impl/LoggerWithExposedConstructor.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/29#newcode1 user/src/com/google/gwt/logging/impl/LoggerWithExposedConstructor.java:1: package com.google.gwt.logging.impl; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/30 File user/src/com/google/gwt/logging/server/RemoteLoggingService.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/30#newcode1 user/src/com/google/gwt/logging/server/RemoteLoggingService.java:1: package com.google.gwt.logging.server; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/32 File user/super/com/google/gwt/emul/java/util/logging/Formatter.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/32#newcode1 user/super/com/google/gwt/emul/java/util/logging/Formatter.java:1: package java.util.logging; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/33 File user/super/com/google/gwt/emul/java/util/logging/Handler.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/33#newcode1 user/super/com/google/gwt/emul/java/util/logging/Handler.java:1: package java.util.logging; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/35 File user/super/com/google/gwt/emul/java/util/logging/LogManager.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/35#newcode1 user/super/com/google/gwt/emul/java/util/logging/LogManager.java:1: package java.util.logging; Copyright header http://gwt-code-reviews.appspot.com/341803/diff/1/35#newcode81 user/super/com/google/gwt/emul/java/util/logging/LogManager.java:81: public Logger getLogger(String name) { Use a map rather than a linear search? http://gwt-code-reviews.appspot.com/341803/diff/1/36 File user/super/com/google/gwt/emul/java/util/logging/LogRecord.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/36#newcode1 user/super/com/google/gwt/emul/java/util/logging/LogRecord.java:1: package java.util.logging; Copyright header Extra blank line http://gwt-code-reviews.appspot.com/341803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
