Author: [EMAIL PROTECTED]
Date: Wed Nov 12 21:30:25 2008
New Revision: 4045
Modified:
branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java
branches/1_6_clean_events/user/test/com/google/gwt/event/shared/HandlerManagerTest.java
Log:
Ensure concurrent mods happen in the right order. Required undoing
optimization in JsRegistry that would do adds immediately, as immediately
is the wrong time if a remove came first
Modified:
branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java
==============================================================================
---
branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java
(original)
+++
branches/1_6_clean_events/user/src/com/google/gwt/event/shared/HandlerManager.java
Wed Nov 12 21:30:25 2008
@@ -18,6 +18,7 @@
import com.google.gwt.core.client.GWT;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.event.shared.GwtEvent.Type;
+import com.google.gwt.user.client.Command;
import java.util.ArrayList;
import java.util.HashMap;
@@ -70,7 +71,12 @@
public <H> void removeHandler(GwtEvent.Type<H> eventKey, H handler) {
ArrayList<H> l = get(eventKey);
if (l != null) {
- l.remove(handler);
+ boolean result = l.remove(handler);
+ if (!result) {
+ // The client code has a similar assert. Relying on asserts in
+ // server code is dicey
+ throw new IllegalArgumentException("Tried to remove unknown
handler");
+ }
}
}
@@ -100,7 +106,7 @@
}
public final <H extends EventHandler> void addHandler(
- HandlerManager manager, Type<H> type, H myHandler) {
+ Type<H> type, H myHandler) {
// The base is the equivalent to a c pointer into the flattened
handler
// data structure.
@@ -112,12 +118,6 @@
// flattened data structure, store the handlers in an external list
// instead.
if ((count == EXPECTED_HANDLERS) & flattened) {
- // As long as we are only adding to the end of a handler list,
should
- // not need to queue.
- if (manager.firingDepth > 0) {
- manager.enqueueAdd(type, myHandler);
- return;
- }
unflatten(base);
flattened = false;
}
@@ -172,15 +172,14 @@
EventHandler handler) {
int base = eventKey.hashCode();
- // Removing a handler is unusual, so smaller code is preferable then
+ // Removing a handler is unusual, so smaller code is preferable to
// handling both flat and dangling list of pointers.
if (isFlattened(base)) {
unflatten(base);
}
boolean result = removeHelper(base, handler);
// Hiding this behind an assertion as we'd rather not force the
compiler
- // to
- // have to include all handler.toString() instances.
+ // to have to include all handler.toString() instances.
assert result : handler + " did not exist";
}
@@ -248,10 +247,6 @@
this[base + 1][index] = handler;
}-*/;
- private native void setHandlerList(int base, JavaScriptObject
handlerList) /*-{
- this[base + 1] = handlerList;
- }-*/;
-
private native void unflatten(int base) /*-{
var handlerList = {};
var count = this[base];
@@ -287,13 +282,10 @@
// source of the event.
private final Object source;
-
- // pending queue of removes that were called during event firing.
- private List<Object> removalQueue;
-
- // pending queue of adds that were called during event firing.
- private ArrayList<Object> addQueue;
-
+
+ // Add and remove operations received during dispatch.
+ private List<Command> deferredDeltas;
+
/**
* Creates a handler manager with the given source.
*
@@ -319,24 +311,10 @@
*/
public <H extends EventHandler> HandlerRegistration addHandler(
GwtEvent.Type<H> type, final H handler) {
-
- /*
- * We used to keep the enqueue / dequeue entirely inside
HandlerManager.
- * However, we found a 30% speed improvement in handler registration if
- * JsHandlerRegistry is allowed to make its own decision about
queuing. Thus
- * the funny code path here.
- *
- * No parallel optimization was made for removing handlers, as that
rarely
- * happens anyway, and is not a significant contributer to startup
time.
- */
- if (useJs) {
- javaScriptRegistry.addHandler(this, type, handler);
+ if (firingDepth > 0) {
+ enqueueAdd(type, handler);
} else {
- if (firingDepth > 0) {
- enqueueAdd(type, handler);
- } else {
- javaRegistry.addHandler(type, handler);
- }
+ doAdd(type, handler);
}
return new DefaultHandlerRegistration(this, type, handler);
}
@@ -388,7 +366,7 @@
public <H extends EventHandler> H getHandler(GwtEvent.Type<H> type, int
index) {
assert index < getHandlerCount(type) : "handlers for " +
type.getClass()
+ " have size: " + getHandlerCount(type)
- + " so do not have a handler at index:" + index;
+ + " so do not have a handler at index: " + index;
if (useJs) {
return javaScriptRegistry.getHandler(type, index);
} else {
@@ -443,6 +421,22 @@
}
}
+ private void defer(Command command) {
+ if (deferredDeltas == null) {
+ deferredDeltas = new ArrayList<Command>();
+ }
+ deferredDeltas.add(command);
+ }
+
+ private <H extends EventHandler> void doAdd(GwtEvent.Type<H> type,
+ final H handler) {
+ if (useJs) {
+ javaScriptRegistry.addHandler(type, handler);
+ } else {
+ javaRegistry.addHandler(type, handler);
+ }
+ }
+
private <H extends EventHandler> void doRemove(GwtEvent.Type<H> type,
final H handler) {
if (useJs) {
@@ -452,40 +446,34 @@
}
}
- private <H extends EventHandler> void enqueueAdd(GwtEvent.Type<H> type,
+ private <H extends EventHandler> void enqueueAdd(final GwtEvent.Type<H>
type,
final H handler) {
- if (addQueue == null) {
- addQueue = new ArrayList<Object>();
- addQueue.add(type);
- addQueue.add(handler);
- }
+ defer(new Command() {
+ public void execute() {
+ doAdd(type, handler);
+ }
+ });
}
- private <H extends EventHandler> void enqueueRemove(GwtEvent.Type<H>
type,
- final H handler) {
- if (removalQueue == null) {
- removalQueue = new ArrayList<Object>();
- removalQueue.add(type);
- removalQueue.add(handler);
- }
+ private <H extends EventHandler> void enqueueRemove(
+ final GwtEvent.Type<H> type, final H handler) {
+ defer(new Command() {
+ public void execute() {
+ doRemove(type, handler);
+ }
+ });
}
@SuppressWarnings("unchecked")
private void handleQueuedAddsAndRemoves() {
- // Do Adds first in case someone does a quick add/remove
- if (addQueue != null) {
- for (int i = 0; i < addQueue.size(); i += 2) {
- addHandler((GwtEvent.Type) addQueue.get(i),
- (EventHandler) addQueue.get(i + 1));
- }
- addQueue = null;
- }
- if (removalQueue != null) {
- for (int i = 0; i < removalQueue.size(); i += 2) {
- doRemove((GwtEvent.Type) removalQueue.get(i),
- (EventHandler) removalQueue.get(i + 1));
+ if (deferredDeltas != null) {
+ try {
+ for (Command c : deferredDeltas) {
+ c.execute();
+ }
+ } finally {
+ deferredDeltas = null;
}
- removalQueue = null;
}
}
}
Modified:
branches/1_6_clean_events/user/test/com/google/gwt/event/shared/HandlerManagerTest.java
==============================================================================
---
branches/1_6_clean_events/user/test/com/google/gwt/event/shared/HandlerManagerTest.java
(original)
+++
branches/1_6_clean_events/user/test/com/google/gwt/event/shared/HandlerManagerTest.java
Wed Nov 12 21:30:25 2008
@@ -16,6 +16,7 @@
package com.google.gwt.event.shared;
+import com.google.gwt.core.client.GWT;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.DomEvent;
import com.google.gwt.event.dom.client.MouseDownEvent;
@@ -25,7 +26,6 @@
/**
* Handler manager test.
*/
[EMAIL PROTECTED]("deprecation")
public class HandlerManagerTest extends HandlerTestBase {
public void testAddHandlers() {
@@ -34,6 +34,7 @@
addHandlers(manager);
}
+ @SuppressWarnings("deprecation")
private void addHandlers(HandlerManager manager) {
manager.addHandler(MouseDownEvent.getType(), mouse1);
manager.addHandler(MouseDownEvent.getType(), mouse2);
@@ -64,6 +65,7 @@
assertNotFired(click1, click2);
}
+ @SuppressWarnings("deprecation")
public void testRemoveHandlers() {
HandlerManager manager = new HandlerManager("bogus source");
addHandlers(manager);
@@ -172,18 +174,46 @@
manager.addHandler(MouseDownEvent.getType(), mouse1);
manager.addHandler(MouseDownEvent.getType(), mouse2);
manager.addHandler(MouseDownEvent.getType(), mouse3);
- manager.fireEvent(new MouseDownEvent() {
- });
+ manager.fireEvent(new MouseDownEvent() { });
assertFired(one, mouse1, mouse2, mouse3);
assertNotFired(two);
reset();
- manager.fireEvent(new MouseDownEvent() {
- });
+ manager.fireEvent(new MouseDownEvent() { });
assertFired(one, mouse1, mouse2, mouse3);
assertNotFired(two);
}
+
+ @SuppressWarnings("deprecation")
+ public void testConcurrentAddAfterRemoveIsNotClobbered() {
+ final HandlerManager manager = new HandlerManager("bogus source");
+
+ MouseDownHandler one = new MouseDownHandler() {
+ public void onMouseDown(MouseDownEvent event) {
+ manager.removeHandler(MouseDownEvent.getType(), mouse1);
+ manager.addHandler(MouseDownEvent.getType(), mouse1);
+ add(this);
+ }
+ };
+ manager.addHandler(MouseDownEvent.getType(), one);
+
+ if (!GWT.isScript()) {
+ try {
+ manager.fireEvent(new MouseDownEvent() { });
+ fail("Should have thrown on remove");
+ } catch (AssertionError e) { /* pass */ }
+ return;
+ }
+
+ // Web mode, no asserts, so remove will quietly succeed.
+ manager.fireEvent(new MouseDownEvent() { });
+ assertFired(one);
+ reset();
+ manager.fireEvent(new MouseDownEvent() { });
+ assertFired(one, mouse1);
+ }
+ @SuppressWarnings("deprecation")
public void testMultiFiring() {
HandlerManager manager = new HandlerManager("source1");
--~--~---------~--~----~------------~-------~--~----~
http://groups.google.com/group/Google-Web-Toolkit-Contributors
-~----------~----~----~----~------~----~------~--~---
