Emily - I replied to all your comments. I'll send out a new patch for review soon that addresses all of your issues.
- John http://gwt-code-reviews.appspot.com/805/diff/1/5 File user/src/com/google/gwt/event/shared/GwtEvent.java (right): http://gwt-code-reviews.appspot.com/805/diff/1/5#newcode139 Line 139: /** On 2008/12/08 23:06:37, ecc wrote: > Should this method be final? As we've regretted making onDetach not final. Also, > I think you meant rely Per our conversation, it doesn't need to be final. Users may override it to take action when an event is killed. http://gwt-code-reviews.appspot.com/805/diff/1/9 File user/src/com/google/gwt/user/client/Event.java (right): http://gwt-code-reviews.appspot.com/805/diff/1/9#newcode187 Line 187: protected void kill() { On 2008/12/08 23:06:37, ecc wrote: > can you document the method that requires access to kill here? fire() has been moved into the Event, so this is no longer needed. http://gwt-code-reviews.appspot.com/805/diff/1/9#newcode482 Line 482: NativePreviewEvent event = new NativePreviewEvent(nativeEvent); On 2008/12/08 23:06:37, ecc wrote: > I think we might want to recycle the native event here for efficiency. Agreed. Added to the latest version. http://gwt-code-reviews.appspot.com/805/diff/1/11 File user/src/com/google/gwt/user/client/ui/DialogBox.java (left): http://gwt-code-reviews.appspot.com/805/diff/1/11#oldcode231 Line 231: On 2008/12/08 23:06:37, ecc wrote: > As much as I'd really, really love to do this, I think backward compatibility > forces us to deprecate the method rather then removing it. I'm just removing the override. onEventPreview is still deprecated in the parent class. http://gwt-code-reviews.appspot.com/805/diff/1/11 File user/src/com/google/gwt/user/client/ui/DialogBox.java (right): http://gwt-code-reviews.appspot.com/805/diff/1/11#newcode392 Line 392: On 2008/12/08 23:06:37, ecc wrote: > do we want to expose this? Per our conversation, onPreviewNativeEvent will be exposed, but will only contain minimal code by default. The bulk of the code will be in a new method previewNativeEvent() http://gwt-code-reviews.appspot.com/805/diff/1/13 File user/src/com/google/gwt/user/client/ui/MenuBar.java (left): http://gwt-code-reviews.appspot.com/805/diff/1/13#oldcode966 Line 966: public boolean onEventPreview(Event event) { On 2008/12/08 23:06:37, ecc wrote: > See comments from DialogBox. We can remove the overide, and this is an anonymous inner class anyway. http://gwt-code-reviews.appspot.com/805/diff/1/12 File user/src/com/google/gwt/user/client/ui/PopupPanel.java (right): http://gwt-code-reviews.appspot.com/805/diff/1/12#newcode464 Line 464: */ On 2008/12/08 23:06:37, ecc wrote: > Where is the onPreviewNativeEvent for this one? Its protected, not public http://gwt-code-reviews.appspot.com/805/diff/1/3 File user/test/com/google/gwt/user/client/EventTest.java (right): http://gwt-code-reviews.appspot.com/805/diff/1/3#newcode26 Line 26: public class EventTest extends GWTTestCase { On 2008/12/08 23:06:37, ecc wrote: > Nice Test! > > I think the only thing you need to add is the interactions of code that mixes > the old and new styles. > For example: > Use adds a new style preview, then an old style preview. What order should they > be evaluated in? Added in new version of patch. http://gwt-code-reviews.appspot.com/805 --~--~---------~--~----~------------~-------~--~----~ http://groups.google.com/group/Google-Web-Toolkit-Contributors -~----------~----~----~----~------~----~------~--~---
