changeset 11347:1fb5d32ea90b

8092352: Skip dispatch if there are no handlers/filters Reviewed-by: kcr, arapte
author fastegal
date Tue, 01 Oct 2019 05:11:17 -0700
parents d9abc7fbc1fc
children 6744f504e684
files modules/javafx.base/src/main/java/com/sun/javafx/event/CompositeEventHandler.java modules/javafx.base/src/main/java/com/sun/javafx/event/EventHandlerManager.java modules/javafx.base/src/test/java/test/com/sun/javafx/event/CompositeEventHandlerTest.java modules/javafx.base/src/test/java/test/com/sun/javafx/event/EventHandlerManagerTest.java
diffstat 4 files changed, 310 insertions(+), 14 deletions(-) [+]
line wrap: on
line diff
--- a/modules/javafx.base/src/main/java/com/sun/javafx/event/CompositeEventHandler.java	Thu Sep 26 12:58:49 2019 -0700
+++ b/modules/javafx.base/src/main/java/com/sun/javafx/event/CompositeEventHandler.java	Tue Oct 01 05:11:17 2019 -0700
@@ -101,6 +101,15 @@
         }
     }
 
+    public boolean hasFilter() {
+        return find(true);
+    }
+
+    public boolean hasHandler() {
+        if (getEventHandler() != null) return true;
+        return find(false);
+    }
+
     /* Used for testing. */
     boolean containsHandler(final EventHandler<? super T> eventHandler) {
         return find(eventHandler, false) != null;
@@ -184,6 +193,19 @@
         return null;
     }
 
+    private boolean find(boolean isFilter) {
+        EventProcessorRecord<T> record = firstRecord;
+        while (record != null) {
+            if (record.isDisconnected()) {
+                remove(record);
+            } else if (isFilter == record.isFilter()) {
+                return true;
+            }
+            record = record.nextRecord;
+        }
+        return false;
+    }
+
     private static abstract class EventProcessorRecord<T extends Event> {
         private EventProcessorRecord<T> nextRecord;
         private EventProcessorRecord<T> prevRecord;
@@ -191,6 +213,8 @@
         public abstract boolean stores(EventHandler<? super T> eventProcessor,
                                        boolean isFilter);
 
+        public abstract boolean isFilter();
+
         public abstract void handleBubblingEvent(T event);
 
         public abstract void handleCapturingEvent(T event);
@@ -210,7 +234,12 @@
         @Override
         public boolean stores(final EventHandler<? super T> eventProcessor,
                               final boolean isFilter) {
-            return !isFilter && (this.eventHandler == eventProcessor);
+            return isFilter == isFilter() && (this.eventHandler == eventProcessor);
+        }
+
+        @Override
+        public boolean isFilter() {
+            return false;
         }
 
         @Override
@@ -240,7 +269,12 @@
         @Override
         public boolean stores(final EventHandler<? super T> eventProcessor,
                               final boolean isFilter) {
-            return !isFilter && (weakEventHandler == eventProcessor);
+            return isFilter == isFilter() && (weakEventHandler == eventProcessor);
+        }
+
+        @Override
+        public boolean isFilter() {
+            return false;
         }
 
         @Override
@@ -270,7 +304,12 @@
         @Override
         public boolean stores(final EventHandler<? super T> eventProcessor,
                               final boolean isFilter) {
-            return isFilter && (this.eventFilter == eventProcessor);
+            return isFilter == isFilter() && (this.eventFilter == eventProcessor);
+        }
+
+        @Override
+        public boolean isFilter() {
+            return true;
         }
 
         @Override
@@ -300,7 +339,12 @@
         @Override
         public boolean stores(final EventHandler<? super T> eventProcessor,
                               final boolean isFilter) {
-            return isFilter && (weakEventFilter == eventProcessor);
+            return isFilter == isFilter() && (weakEventFilter == eventProcessor);
+        }
+
+        @Override
+        public boolean isFilter() {
+            return true;
         }
 
         @Override
--- a/modules/javafx.base/src/main/java/com/sun/javafx/event/EventHandlerManager.java	Thu Sep 26 12:58:49 2019 -0700
+++ b/modules/javafx.base/src/main/java/com/sun/javafx/event/EventHandlerManager.java	Tue Oct 01 05:11:17 2019 -0700
@@ -216,9 +216,7 @@
         final CompositeEventHandler<? extends Event> compositeEventHandler =
                 eventHandlerMap.get(handlerType);
 
-        if (compositeEventHandler != null) {
-            // TODO: skip when no filters are registered in the
-            //       CompositeEventHandler (RT-23952)
+        if (compositeEventHandler != null && compositeEventHandler.hasFilter()) {
             event = fixEventSource(event, eventSource);
             compositeEventHandler.dispatchCapturingEvent(event);
         }
@@ -231,9 +229,7 @@
         final CompositeEventHandler<? extends Event> compositeEventHandler =
                 eventHandlerMap.get(handlerType);
 
-        if (compositeEventHandler != null) {
-            // TODO: skip when no handlers are registered in the
-            //       CompositeEventHandler (RT-23952)
+        if (compositeEventHandler != null && compositeEventHandler.hasHandler()) {
             event = fixEventSource(event, eventSource);
             compositeEventHandler.dispatchBubblingEvent(event);
         }
--- a/modules/javafx.base/src/test/java/test/com/sun/javafx/event/CompositeEventHandlerTest.java	Thu Sep 26 12:58:49 2019 -0700
+++ b/modules/javafx.base/src/test/java/test/com/sun/javafx/event/CompositeEventHandlerTest.java	Tue Oct 01 05:11:17 2019 -0700
@@ -27,6 +27,9 @@
 
 import com.sun.javafx.event.CompositeEventHandler;
 import com.sun.javafx.event.CompositeEventHandlerShim;
+
+import static org.junit.Assert.*;
+
 import test.com.sun.javafx.event.EventCountingHandler;
 import test.com.sun.javafx.event.EmptyEvent;
 import javafx.event.Event;
@@ -45,6 +48,132 @@
         compositeEventHandler = new CompositeEventHandler<Event>();
     }
 
+    /**
+     * test state report after add/clear weak filter
+     * Here we test that a garbage collected weak filter is actually
+     * removed from the chain.
+     */
+    @Test
+    public void testHasFilterWeakCleared() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        final WeakEventHandler<Event> weakEventHandler =
+                new WeakEventHandler<Event>(eventCountingHandler);
+
+        compositeEventHandler.addEventFilter(weakEventHandler);
+        assertFalse("must not have handler after adding filter", compositeEventHandler.hasHandler());
+        assertTrue("must have filter", compositeEventHandler.hasFilter());
+        WeakEventHandlerUtil.clear(weakEventHandler);
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+    }
+
+    /**
+     * test state report after add/clear weak handler
+     */
+    @Test
+    public void testHasHandlerAddWeakClear() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        final WeakEventHandler<Event> weakEventHandler =
+                new WeakEventHandler<Event>(eventCountingHandler);
+        compositeEventHandler.addEventHandler(weakEventHandler);
+        assertTrue("sanity: really added?", CompositeEventHandlerShim.containsHandler(
+                compositeEventHandler, weakEventHandler));
+        assertFalse("must not have filter after adding handler", compositeEventHandler.hasFilter());
+        assertTrue("must have handler", compositeEventHandler.hasHandler());
+        WeakEventHandlerUtil.clear(weakEventHandler);
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+    }
+
+    /**
+     * test state report after add/remove weak filter
+     * Here we test that the duplicated (against normal) implementation
+     * behaves as expected.
+     */
+    @Test
+    public void testHasFilterWeak() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        final WeakEventHandler<Event> weakEventHandler =
+                new WeakEventHandler<Event>(eventCountingHandler);
+
+        compositeEventHandler.addEventFilter(weakEventHandler);
+        assertFalse("must not have handler after adding filter", compositeEventHandler.hasHandler());
+        assertTrue("must have filter", compositeEventHandler.hasFilter());
+        compositeEventHandler.removeEventFilter(weakEventHandler);
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+    }
+
+    /**
+     * test state report after add/remove weak handler
+     */
+    @Test
+    public void testHasHandlerAddWeak() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        final WeakEventHandler<Event> weakEventHandler =
+                new WeakEventHandler<Event>(eventCountingHandler);
+        compositeEventHandler.addEventHandler(weakEventHandler);
+        assertTrue("sanity: really added?", CompositeEventHandlerShim.containsHandler(
+                compositeEventHandler, weakEventHandler));
+        assertFalse("must not have filter after adding handler", compositeEventHandler.hasFilter());
+        assertTrue("must have handler", compositeEventHandler.hasHandler());
+        compositeEventHandler.removeEventHandler(weakEventHandler);
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+    }
+
+    /**
+     * test state after add/remove filter
+     */
+    @Test
+    public void testHasFilter() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        compositeEventHandler.addEventFilter(eventCountingHandler);
+        assertFalse("must not have handler after adding filter", compositeEventHandler.hasHandler());
+        assertTrue("must have filter", compositeEventHandler.hasFilter());
+        compositeEventHandler.removeEventFilter(eventCountingHandler);
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+    }
+
+    /**
+     * test report after add/remove handler
+     */
+    @Test
+    public void testHasHandlerAdd() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        compositeEventHandler.addEventHandler(eventCountingHandler);
+        assertTrue("sanity: really added?", CompositeEventHandlerShim.containsHandler(
+                compositeEventHandler, eventCountingHandler));
+        assertFalse("must not have filter after adding handler", compositeEventHandler.hasFilter());
+        assertTrue("must have handler", compositeEventHandler.hasHandler());
+        compositeEventHandler.removeEventHandler(eventCountingHandler);
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+
+    }
+
+    /**
+     * test state after set/null singleton handler
+     */
+    @Test
+    public void testHasHandlerSingleton() {
+        final EventCountingHandler<Event> eventCountingHandler =
+                new EventCountingHandler<Event>();
+        compositeEventHandler.setEventHandler(eventCountingHandler);
+        assertFalse("must not have filter after set handler", compositeEventHandler.hasFilter());
+        assertTrue("must have handler", compositeEventHandler.hasHandler());
+        compositeEventHandler.setEventHandler(null);
+        assertFalse("must not have filter", compositeEventHandler.hasFilter());
+        assertFalse("must not have handler", compositeEventHandler.hasHandler());
+    }
+
     @Test
     public void weakEventHandlerTest() {
         final EventCountingHandler<Event> eventCountingHandler =
--- a/modules/javafx.base/src/test/java/test/com/sun/javafx/event/EventHandlerManagerTest.java	Thu Sep 26 12:58:49 2019 -0700
+++ b/modules/javafx.base/src/test/java/test/com/sun/javafx/event/EventHandlerManagerTest.java	Tue Oct 01 05:11:17 2019 -0700
@@ -25,16 +25,21 @@
 
 package test.com.sun.javafx.event;
 
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
 import com.sun.javafx.event.EventHandlerManager;
+
+import static org.junit.Assert.*;
+
 import javafx.event.Event;
 import javafx.event.EventDispatchChain;
 import javafx.event.EventDispatcher;
 import javafx.event.EventHandler;
 import javafx.event.EventType;
-
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
+import javafx.event.WeakEventHandler;
+import javafx.event.WeakEventHandlerUtil;
 
 public final class EventHandlerManagerTest {
     private EventHandlerManager eventHandlerManager;
@@ -44,6 +49,128 @@
         eventHandlerManager = new EventHandlerManager(this);
     }
 
+    /**
+     * JDK-8092352: Skip dispatch if there are no handlers/filters
+     * sanity test: freshly instantiated empty EventHandlerManager returns
+     * same instance of event
+     */
+    @Test
+    public void testEmptyHandler() {
+        assertDispatch(new ValueEvent(0), 0);
+    }
+
+    /**
+     * JDK-8092352: Skip dispatch if there are no handlers/filters
+     * Test cycle set/null singleton
+     */
+    @Test
+    public void testShouldNotCopyEventWithoutSingletonHandler() {
+        EventChangingHandler eventHandler = new EventChangingHandler(Operation.add(5));
+        // add handler
+        eventHandlerManager.setEventHandler(
+                ValueEvent.VALUE_A,
+                eventHandler);
+        ValueEvent sent = new ValueEvent(0);
+        assertDispatch(sent, 5);
+        // remove handler
+        eventHandlerManager.setEventHandler(ValueEvent.VALUE_A, null);
+        assertDispatch(sent, 0);
+    }
+
+    /**
+     * JDK-8092352: Skip dispatch if there are no handlers/filters
+     * Test cycle add/remove handler
+     */
+    @Test
+    public void testShouldNotCopyEventWithoutHandler() {
+        EventChangingHandler eventHandler = new EventChangingHandler(Operation.add(5));
+        // add handler
+        eventHandlerManager.addEventHandler(
+                ValueEvent.VALUE_A,
+                eventHandler);
+        ValueEvent sent = new ValueEvent(0);
+        assertDispatch(sent, 5);
+        // remove handler
+        eventHandlerManager.removeEventHandler(ValueEvent.VALUE_A, eventHandler);
+        assertDispatch(sent, 0);
+    }
+
+    /**
+     * JDK-8092352: Skip dispatch if there are no handlers/filters
+     * Test cycle add/remove filter
+     */
+    @Test
+    public void testShouldNotCopyEventWithoutFilter() {
+        EventChangingHandler eventFilter = new EventChangingHandler(Operation.add(5));
+        // add filter
+        eventHandlerManager.addEventFilter(
+                ValueEvent.VALUE_A,
+                eventFilter);
+        ValueEvent sent = new ValueEvent(0);
+        assertDispatch(sent, 5);
+        // remove filter
+        eventHandlerManager.removeEventFilter(ValueEvent.VALUE_A, eventFilter);
+        assertDispatch(sent, 0);
+    }
+
+    /**
+     * JDK-8092352: Skip dispatch if there are no handlers/filters
+     * Test cycle add/clear weak handler
+     */
+    @Test
+    public void testShouldNotCopyEventWeakHandlerCleared() {
+        EventChangingHandler eventHandler = new EventChangingHandler(Operation.add(5));
+        WeakEventHandler<ValueEvent> weakHandler = new WeakEventHandler<>(eventHandler);
+        // add weak handler
+        eventHandlerManager.addEventHandler(
+                ValueEvent.VALUE_A,
+                weakHandler);
+        ValueEvent sent = new ValueEvent(0);
+        assertDispatch(sent, 5);
+        // clear weak handler
+        WeakEventHandlerUtil.clear(weakHandler);
+        assertDispatch(sent, 0);
+    }
+
+    /**
+     * JDK-8092352: Skip dispatch if there are no handlers/filters
+     * Test cycle add/clear weak filter
+     */
+    @Test
+    public void testShouldNotCopyEventWeakFilterCleared() {
+        EventChangingHandler eventFilter = new EventChangingHandler(Operation.add(5));
+        WeakEventHandler<ValueEvent> weakFilter = new WeakEventHandler<>(eventFilter);
+        // add filter
+        eventHandlerManager.addEventFilter(
+                ValueEvent.VALUE_A,
+                weakFilter);
+
+        ValueEvent sent = new ValueEvent(0);
+        assertDispatch(sent, 5);
+        // clear weak filter
+        WeakEventHandlerUtil.clear(weakFilter);
+        assertDispatch(sent, 0);
+    }
+
+    /**
+     * Helper for JDK-8092352 testing: dispatches the given event and
+     * asserts its value and identity. If the given expected value is the
+     * same as the event's initial value, the received event is expected
+     * to be the same instance as the sent.
+     */
+    private void assertDispatch(ValueEvent sent, int expected) {
+        boolean same = sent.getValue() == expected;
+        ValueEvent received = (ValueEvent)
+                eventHandlerManager.dispatchEvent(sent, StubEventDispatchChain.EMPTY_CHAIN);
+        String message = "value must be " + (same ? "unchanged " : "changed ");
+        assertEquals(message, expected, received.getValue());
+        if (same) {
+            assertSame("received event", sent, received);
+        } else {
+            assertNotSame("received event", sent, received);
+        }
+    }
+
     @Test
     public void shouldForwardEventsToChain() {
         final EventDispatchChain eventDispatchChain =