http://gwt-code-reviews.appspot.com/880802/diff/12001/13003 File samples/logexample/src/com/google/gwt/sample/logexample/client/LogExample.java (right):
http://gwt-code-reviews.appspot.com/880802/diff/12001/13003#newcode55 samples/logexample/src/com/google/gwt/sample/logexample/client/LogExample.java:55: }); The remainer of the code should be executed in a (documented) DeferredCommand lest the UEH will not have taken effect. See http://code.google.com/p/gwt-dnd/wiki/GettingStarted and search for onModuleLoad2 as an example Should also note to the reader that any (static or non-static) initialization code in this class could throw an exception before the UEH is installed, which means the UEH will not be called in that case http://gwt-code-reviews.appspot.com/880802/diff/12001/13009 File user/src/com/google/gwt/logging/server/RemoteLoggingServiceImpl.java (right): http://gwt-code-reviews.appspot.com/880802/diff/12001/13009#newcode29 user/src/com/google/gwt/logging/server/RemoteLoggingServiceImpl.java:29: new StackTraceDeobfuscator(""); Document why we're passing in the empty string http://gwt-code-reviews.appspot.com/880802/diff/12001/13010 File user/src/com/google/gwt/logging/server/RemoteLoggingServiceUtil.java (right): http://gwt-code-reviews.appspot.com/880802/diff/12001/13010#newcode37 user/src/com/google/gwt/logging/server/RemoteLoggingServiceUtil.java:37: if (lr == null) { Why would this be null? Maybe document a possible reason? http://gwt-code-reviews.appspot.com/880802/diff/12001/13010#newcode52 user/src/com/google/gwt/logging/server/RemoteLoggingServiceUtil.java:52: return e.toString(); I'm not a fan of returning just the exception message and giving the recipient no way to access the stack trace. Can we do something more useful for the caller? http://gwt-code-reviews.appspot.com/880802/diff/12001/13011 File user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java (right): http://gwt-code-reviews.appspot.com/880802/diff/12001/13011#newcode68 user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java:68: public void setSymbolMapsDirectory(String dir) { Because symbolMapsDirectory = "", callers can be expected to pass in the empty string in order to get the default symbol maps directory. If that's an expected "feature" of the API, it should be documented so that even if the default symbol maps directory changes, anyone updating this class will know to make the corresponding changes here. http://gwt-code-reviews.appspot.com/880802/diff/12001/13011#newcode69 user/src/com/google/gwt/logging/server/StackTraceDeobfuscator.java:69: if (!dir.equals(symbolMapsDirectory)) { [See above comment] In fact, if that is to be a documented feature, then I'd expect an explicit test here against the empty string ("") and not symbolMapsDirectory, which just so happens to be the empty string. Alternate suggestion: Make passing in null be or use the no-arg ctor imply the default directory. http://gwt-code-reviews.appspot.com/880802/diff/12001/13015 File user/src/com/google/gwt/requestfactory/client/RequestFactoryLogHandler.java (right): http://gwt-code-reviews.appspot.com/880802/diff/12001/13015#newcode57 user/src/com/google/gwt/requestfactory/client/RequestFactoryLogHandler.java:57: * Level being used app-wide. This handler also takes string swhich it will string swhich -> strings, which http://gwt-code-reviews.appspot.com/880802/diff/12001/13015#newcode65 user/src/com/google/gwt/requestfactory/client/RequestFactoryLogHandler.java:65: super(ignoredLoggerSubstrings); Are substrings robust enough? Can you include explicit examples of the substrings that one would pass in and some common loggers which would be tested? http://gwt-code-reviews.appspot.com/880802/diff/12001/13016 File user/src/com/google/gwt/requestfactory/server/Logging.java (right): http://gwt-code-reviews.appspot.com/880802/diff/12001/13016#newcode38 user/src/com/google/gwt/requestfactory/server/Logging.java:38: RpcRequestBuilder.STRONG_NAME_HEADER); Should probably assert that the header is not null. Also, is there a corresponding test to assert that the header is present? http://gwt-code-reviews.appspot.com/880802/diff/12001/13017 File user/src/com/google/gwt/requestfactory/shared/LoggingRequest.java (right): http://gwt-code-reviews.appspot.com/880802/diff/12001/13017#newcode30 user/src/com/google/gwt/requestfactory/shared/LoggingRequest.java:30: RequestObject<String> logMessage(String serializedLogRecordString); Can you document what return values should be considered success vs. failure? What actual string values might one get back? http://gwt-code-reviews.appspot.com/880802/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
