http://gwt-code-reviews.appspot.com/341803/diff/1/3 File samples/hello/src/com/google/gwt/sample/hello/Hello.gwt.xml (right):
http://gwt-code-reviews.appspot.com/341803/diff/1/3#newcode26 samples/hello/src/com/google/gwt/sample/hello/Hello.gwt.xml:26: <entry-point class="com.google.gwt.sample.hello.client.Hello"/> can you fix the whitespace here while you're at it? http://gwt-code-reviews.appspot.com/341803/diff/1/5 File samples/hello/war/Hello.html (right): http://gwt-code-reviews.appspot.com/341803/diff/1/5#newcode23 samples/hello/war/Hello.html:23: My name is George!! Test code? http://gwt-code-reviews.appspot.com/341803/diff/1/9 File user/src/com/google/gwt/logging/Logging.gwt.xml (right): http://gwt-code-reviews.appspot.com/341803/diff/1/9#newcode89 user/src/com/google/gwt/logging/Logging.gwt.xml:89: <when-type-is class="java.util.logging.LogManager.RootLogger" /> Why would you GWT.create(java.util.logging.LogManager.RootLogger.class) only to have the class unconditionally replaced with com.google.gwt.logging.client.RootLogger)? 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#newcode38 user/src/com/google/gwt/logging/client/ConsoleLogHandler.java:38: $wnd.console.log(message); I wonder if it would be more appropriate to call: window.console.log(message) See my comment on the FirebugLogHandler 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#newcode46 user/src/com/google/gwt/logging/client/FirebugLogHandler.java:46: $wnd.console.debug(message); I wonder if it would be more appropriate to call: window.console.debug(message) window.console.info(message) .. Practically I'm not sure that it actually matters, even in the default case with the IFRAME linker. But I was recently caught by http://code.google.com/p/fbug/issues/detail?id=2914 While that issue was fixed, I ended up working around the issue anyway: http://code.google.com/p/gwt-log/source/diff?spec=svn461&r=461&format=side&path=/trunk/Log/src/com/allen_sauer/gwt/log/client/FirebugLogger.java 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#newcode46 user/src/com/google/gwt/logging/client/HasWidgetsLogHandler.java:46: widgetContainer.add(new HTML(msg)); Need to prevent arbitrary HTML injection in case of in case the developer calls: - setFormatter(new MyCustomHtmlLogFormatterWhichDoesNotEscapeHtml()) - setFormatter(new TextLogFormatter()) Could go with: If (getFormatter() instanceof HtmlLogFormatter) { widgerContainer.add(new HTML(msg); } else { widgetContainer.add(new Label(msg); } And then rewrite HtmlLogFormatter so that only escaped HTML is passed into a protected method which subclasses can override, e.g. some along the lines of: public final String format(LogRecord record) { String htmlEscapedMessage = .... return getHtmlPrefix(record) + escape(htmlEscapedMessage) + getHtmlSuffix(record); } protected String getHtmlPrefix(Record record) { } }protected String getHtmlSuffix(Record record) { } 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#newcode17 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:17: date.setTime(event.getMillis()); Why not: Date date = new Date(event.getMillis()) http://gwt-code-reviews.appspot.com/341803/diff/1/16#newcode23 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:23: title += " thrown: " + event.getThrown(); This won't print a stack trace http://gwt-code-reviews.appspot.com/341803/diff/1/16#newcode34 user/src/com/google/gwt/logging/client/HtmlLogFormatter.java:34: // TODO(unnurg): There must be a cleaner way to do this... There's always the JavaScript builtin escape() function 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#newcode13 user/src/com/google/gwt/logging/client/LogConfiguration.java:13: * Configures client side logging using the CGI params and gwt.xml settings. s/CGI/query/g 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#newcode36 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:36: super(throwable); Simply wrapping Throwable will cause a lot of [de]serialization code to be generated. Should just send a StackTraceElement[] over the wire instead. http://gwt-code-reviews.appspot.com/341803/diff/1/20#newcode90 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:90: //Log.finest("Remote logging message acknowledged", CATEGORY); Remove this line before commit? http://gwt-code-reviews.appspot.com/341803/diff/1/20#newcode94 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:94: //private static final String CATEGORY = "gwt.logging.RemoteLogHandler"; remove comment http://gwt-code-reviews.appspot.com/341803/diff/1/20#newcode135 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:135: public void setCallBack(AsyncCallback<Void> callback) { Is there a strong use case for this already? If not, I suggest leaving it out. Less is more. http://gwt-code-reviews.appspot.com/341803/diff/1/20#newcode153 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:153: // TODO Auto-generated method stub change comment http://gwt-code-reviews.appspot.com/341803/diff/1/20#newcode159 user/src/com/google/gwt/logging/client/RemoteLogHandler.java:159: // TODO Auto-generated method stub change comment 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#newcode17 user/src/com/google/gwt/logging/client/TextLogFormatter.java:17: date.setTime(event.getMillis()); Why not: Date date = new Date(event.getMillis()) http://gwt-code-reviews.appspot.com/341803/diff/1/22#newcode23 user/src/com/google/gwt/logging/client/TextLogFormatter.java:23: accum.append("\n thrown: " + event.getThrown()); This won't print a stack trace 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#newcode98 user/src/com/google/gwt/logging/impl/LoggerImplRegular.java:98: public void log(Level level, String msg) { Can we just call: log(level, msg, null) http://gwt-code-reviews.appspot.com/341803/diff/1/28#newcode102 user/src/com/google/gwt/logging/impl/LoggerImplRegular.java:102: log(record); } closing paren on next line 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#newcode12 user/src/com/google/gwt/logging/impl/LoggerWithExposedConstructor.java:12: public class LoggerWithExposedConstructor extends Logger { This class is the same as the static inner class in LevelImplRegular ? 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#newcode24 user/src/com/google/gwt/logging/server/RemoteLoggingService.java:24: System.err.println("Failed to log message due to " + e.toString()); Suggest dropping e.toString(): System.err.println("Failed to log message due to: "); http://gwt-code-reviews.appspot.com/341803/diff/1/37 File user/super/com/google/gwt/emul/java/util/logging/Logger.java (right): http://gwt-code-reviews.appspot.com/341803/diff/1/37#newcode46 user/super/com/google/gwt/emul/java/util/logging/Logger.java:46: impl.addHandler(handler); A nice optimization (to avoid carrying around a bunch of NullHandlers at runtime) might be to make this method a no-op if handler instanceof NullHandler http://gwt-code-reviews.appspot.com/341803/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
