http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/activity/shared/Activity.java File user/src/com/google/gwt/activity/shared/Activity.java (right):
http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/activity/shared/Activity.java#newcode19 user/src/com/google/gwt/activity/shared/Activity.java:19: import com.google.gwt.user.client.ui.IsWidget; This looks like an unused import (only used in JavaDoc). Might be better to inline the full path in the JavaDoc to avoid the warning. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/Event.gwt.xml File user/src/com/google/gwt/event/Event.gwt.xml (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/Event.gwt.xml#newcode2 user/src/com/google/gwt/event/Event.gwt.xml:2: <inherits name="com.google.gwt.event.EventBase" /> Remove tabs from all lines or revert. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/EventBase.gwt.xml File user/src/com/google/gwt/event/EventBase.gwt.xml (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/gwt/event/EventBase.gwt.xml#newcode5 user/src/com/google/gwt/event/EventBase.gwt.xml:5: <source path="shared" /> remove tabs from all lines. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/Event.gwt.xml File user/src/com/google/web/bindery/event/Event.gwt.xml (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/Event.gwt.xml#newcode17 user/src/com/google/web/bindery/event/Event.gwt.xml:17: <source path="client"/> The client path is not actually present. Can we remove it? Is it here for future support? http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/Event.gwt.xml#newcode18 user/src/com/google/web/bindery/event/Event.gwt.xml:18: <source path="shared"/> extra spaces or tabs http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/Event.java File user/src/com/google/web/bindery/event/shared/Event.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/Event.java#newcode2 user/src/com/google/web/bindery/event/shared/Event.java:2: * Copyright 2009 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/EventBus.java File user/src/com/google/web/bindery/event/shared/EventBus.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/EventBus.java#newcode2 user/src/com/google/web/bindery/event/shared/EventBus.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java File user/src/com/google/web/bindery/event/shared/HandlerRegistration.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode2 user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:2: * Copyright 2008 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode20 user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:20: * {@link EventBus#addHandler}, used to deregister. Close parathesis from (e.g....): (e.g. via {@linkEventBus#addHandler}) http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode22 user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:22: * A tip: to make a handler deregister itself, the following works: Phrasing. How about the following: NOTE: The following code creates a handler that deregisters itself the first time it handles an event. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/HandlerRegistration.java#newcode35 user/src/com/google/web/bindery/event/shared/HandlerRegistration.java:35: * Deregisters a handler. The API should call out how to handle multiple calls or calls to handlers that are already removed. Deregisters the handler associated with this registration object if the handler is still attached to the event source. If the handler is no longer attached to the event source, this is a no-op. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java File user/src/com/google/web/bindery/event/shared/ResettableEventBus.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java#newcode2 user/src/com/google/web/bindery/event/shared/ResettableEventBus.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/ResettableEventBus.java#newcode27 user/src/com/google/web/bindery/event/shared/ResettableEventBus.java:27: public class ResettableEventBus extends EventBus { Make sure you incorporate http://gwt-code-reviews.appspot.com/1388804/ before submitting. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java File user/src/com/google/web/bindery/event/shared/SimpleEventBus.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode2 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode37 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:37: private interface Command { interfaces go above fields. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode63 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:63: * protected because it is a bad idea to rely upon the order of It isn't package protected, but see next comment. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode69 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:69: protected SimpleEventBus(boolean fireInReverseOrder) { I suggest that you remove this method and add a package protected setFireInReverseOrder method. Then, you can call it from c.g.g.event.shared.SimpleEventBus using the violator pattern and keep this API clean. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode96 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:96: } You can move the type and handler checks to doAdd(). Slight reduction in code size and protects against future uses of doAdd(). The source==null check will have to stay in this method. http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode116 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:116: } Ditto. Move event==null check into doFire, keep the source==null check in this method. Also, there are a lot of if(null) checks. You could add a assertNotNull(Object toCheck, String message) http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/SimpleEventBus.java#newcode124 user/src/com/google/web/bindery/event/shared/SimpleEventBus.java:124: protected <H> void doRemove(Event.Type<H> type, Object source, H handler) { I don't like adding deprecated APIs. Can you use the violator pattern to keep the deprecated methods out of here? http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java File user/src/com/google/web/bindery/event/shared/UmbrellaException.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java#newcode2 user/src/com/google/web/bindery/event/shared/UmbrellaException.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/UmbrellaException.java#newcode22 user/src/com/google/web/bindery/event/shared/UmbrellaException.java:22: * {@link Throwable}s together. Typically thrown after loop, with all of the thrown after loop => thrown after a loop http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java File user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode2 user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode77 user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:77: counts.put(type, count - 1); maybe just getCount(type) - 1 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode85 user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:85: counts.put(type, count + 1); getCount(type) + 1 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/gwt/event/shared/SimpleEventBusTest.java File user/test/com/google/gwt/event/shared/SimpleEventBusTest.java (left): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/gwt/event/shared/SimpleEventBusTest.java#oldcode35 user/test/com/google/gwt/event/shared/SimpleEventBusTest.java:35: public void testAddAndRemoveHandlers() { Why remove all of these tests? http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/EventBusTestBase.java File user/test/com/google/web/bindery/event/shared/EventBusTestBase.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/EventBusTestBase.java#newcode2 user/test/com/google/web/bindery/event/shared/EventBusTestBase.java:2: * Copyright 2008 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java File user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java#newcode2 user/test/com/google/web/bindery/event/shared/ResettableEventBusTest.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java File user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java (right): http://gwt-code-reviews.appspot.com/1394803/diff/1/user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java#newcode2 user/test/com/google/web/bindery/event/shared/SimpleEventBusTest.java:2: * Copyright 2010 Google Inc. 2011 http://gwt-code-reviews.appspot.com/1394803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
