Send got/lostpointercapture immediately if possible

third_party/WebKit/Source/core/input/PointerEventManager.cpp

Index: third_party/WebKit/Source/core/input/PointerEventManager.cpp
diff --git a/third_party/WebKit/Source/core/input/PointerEventManager.cpp b/third_party/WebKit/Source/core/input/PointerEventManager.cpp
index c3cb5b770a9521f9937ea289373426c9127a9dcf..b5ce0949f095f7dacb8d7f2c893df88dd3169161 100644
--- a/third_party/WebKit/Source/core/input/PointerEventManager.cpp
+++ b/third_party/WebKit/Source/core/input/PointerEventManager.cpp
@@ -5,6 +5,7 @@
 #include "core/input/PointerEventManager.h"
 
 #include "core/dom/ElementTraversal.h"
+#include "core/dom/ExecutionContextTask.h"
 #include "core/dom/shadow/FlatTreeTraversal.h"
 #include "core/events/MouseEvent.h"
 #include "core/frame/FrameView.h"
@@ -612,6 +613,25 @@ void PointerEventManager::clear()
     m_pendingPointerCaptureTarget.clear();
 }
 
+bool PointerEventManager::getPointerCaptureState(int pointerId,
+    EventTarget** pointerCaptureTarget,
+    EventTarget** pendingPointerCaptureTarget)
+{
+    EventTarget* pct = m_pointerCaptureTarget.contains(pointerId)

[Rick Byers, 2016/07/15 17:35:32]

nit: HashMap::find is the efficient way to do contains/get at the same time.

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+        ? m_pointerCaptureTarget.get(pointerId)
+        : nullptr;
+    EventTarget* ppct = m_pendingPointerCaptureTarget.contains(pointerId)
+        ? m_pendingPointerCaptureTarget.get(pointerId)
+        : nullptr;
+
+    if (pointerCaptureTarget)
+        *pointerCaptureTarget = pct;
+    if (pendingPointerCaptureTarget)
+        *pendingPointerCaptureTarget = ppct;
+
+    return pct != ppct;
+}
+
 void PointerEventManager::processCaptureAndPositionOfPointerEvent(
     PointerEvent* pointerEvent,
     EventTarget* hitTestTarget,
@@ -636,23 +656,54 @@ void PointerEventManager::processCaptureAndPositionOfPointerEvent(
     }
 }
 
+void PointerEventManager::immediateProcessPendingPointerCapture(int pointerId)

[mustaq, 2016/07/15 19:35:43]

Nit: s/immediate/immediately/

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+{
+    EventTarget* pointerCaptureTarget;
+    EventTarget* pendingPointerCaptureTarget;
+    const bool isCaptureChanged = getPointerCaptureState(pointerId,
+        &pointerCaptureTarget, &pendingPointerCaptureTarget);
+
+    if (!isCaptureChanged)
+        return;
+
+    if (pointerCaptureTarget) {
+        // Re-target lostpointercapture to the document when the element is

[Rick Byers, 2016/07/15 17:35:32]

is this behavior specified somewhere?  It seems reasonable to me, but if it's
not spec'd then please file a GitHub issue and reference it from a TODO here
(but obviously don't block on it).

[Navid Zolghadr, 2016/07/15 20:41:01]

Yes. It is here the second paragraph.
https://w3c.github.io/pointerevents/#implicit-release-of-pointer-capture

+        // no longer participating in the tree.
+        EventTarget* target = pointerCaptureTarget;
+        if (target->toNode()
+            && !target->toNode()->inShadowIncludingDocument()) {
+            target = target->toNode()->ownerDocument();
+        }
+        dispatchPointerEvent(target,
+            m_pointerEventFactory.createPointerCaptureEvent(
+                pointerId, EventTypeNames::lostpointercapture));
+    }
+    dispatchPointerEvent(pendingPointerCaptureTarget,

[Rick Byers, 2016/07/15 17:35:32]

Don't you need to check for null here too - eg. for the
Element.releasePointerCapture cases where only a 'lost' event is fired without
any 'got'?

[mustaq, 2016/07/15 19:35:43]

Yes, pendingPointerCaptureTarget would be null after a release call.

[Navid Zolghadr, 2016/07/15 20:41:01]

Since there was a lot of those checks around every dispatchPointerEvent in this
file I just put them inside dispatchPointerEvent to avoid having it for every
one in one of the early changes. The only reason that I have it in the previous
case is because for the lostpointercapture case it wanted to get the parent not
and I wanted to make sure it is not null.

+        m_pointerEventFactory.createPointerCaptureEvent(
+            pointerId, EventTypeNames::gotpointercapture));
+
+    // Setting |m_pointerCaptureTarget| comes at the end to prevent the infinite
+    // loop if js calls set/releasepointercapture in got/lostpointercapture
+    // handlers.
+    if (pendingPointerCaptureTarget)
+        m_pointerCaptureTarget.set(pointerId, pendingPointerCaptureTarget);
+    else
+        m_pointerCaptureTarget.remove(pointerId);
+}
+
 bool PointerEventManager::processPendingPointerCapture(

[Rick Byers, 2016/07/15 17:35:31]

once we make the other changes here (eg. not firing the boundary events) it
should be easier to merge these two similar "process pending pointer capture"
codepaths, right?  Please add a TODO saying they should be merged.

[Navid Zolghadr, 2016/07/15 20:41:01]

That is correct. I added the TODO.

[mustaq, 2016/07/15 21:06:14]

"Not firing the boundary events": we still need to fire boundary
events here because of the consistency principles regarding entry vs
capturing states, right?
https://github.com/w3c/pointerevents/issues/61#issuecomment-218512170
(The github issue is about boundary events /during capturing/, not
/at the moment of capturing/.)

     PointerEvent* pointerEvent,
     EventTarget* hitTestTarget,
     const PlatformMouseEvent& mouseEvent, bool sendMouseEvent)
 {
     int pointerId = pointerEvent->pointerId();
-    EventTarget* pointerCaptureTarget =
-        m_pointerCaptureTarget.contains(pointerId)
-        ? m_pointerCaptureTarget.get(pointerId) : nullptr;
-    EventTarget* pendingPointerCaptureTarget =
-        m_pendingPointerCaptureTarget.contains(pointerId)
-        ? m_pendingPointerCaptureTarget.get(pointerId) : nullptr;
+    EventTarget* pointerCaptureTarget;
+    EventTarget* pendingPointerCaptureTarget;
+    const bool isCaptureChanged = getPointerCaptureState(pointerId,
+        &pointerCaptureTarget, &pendingPointerCaptureTarget);
     const EventTargetAttributes &nodeUnderPointerAtt =
         m_nodeUnderPointer.contains(pointerId)
         ? m_nodeUnderPointer.get(pointerId) : EventTargetAttributes();
-    const bool isCaptureChanged =
-        pointerCaptureTarget != pendingPointerCaptureTarget;
 
     if (isCaptureChanged) {
         if ((hitTestTarget != nodeUnderPointerAtt.target
@@ -678,7 +729,7 @@ bool PointerEventManager::processPendingPointerCapture(
             }
             dispatchPointerEvent(target,
                 m_pointerEventFactory.createPointerCaptureEvent(
-                pointerEvent, EventTypeNames::lostpointercapture));
+                    pointerId, EventTypeNames::lostpointercapture));
         }
     }
 
@@ -694,7 +745,7 @@ bool PointerEventManager::processPendingPointerCapture(
     if (isCaptureChanged) {
         dispatchPointerEvent(pendingPointerCaptureTarget,
             m_pointerEventFactory.createPointerCaptureEvent(
-            pointerEvent, EventTypeNames::gotpointercapture));
+                pointerId, EventTypeNames::gotpointercapture));
         if ((pendingPointerCaptureTarget == hitTestTarget
             || !pendingPointerCaptureTarget)
             && (nodeUnderPointerAtt.target != hitTestTarget
@@ -751,8 +802,20 @@ void PointerEventManager::elementRemoved(EventTarget* target)
 void PointerEventManager::setPointerCapture(int pointerId, EventTarget* target)

[mustaq, 2016/07/15 19:35:43]

It's not clear to me what happens in case of a janky JS handler where
setPointerCapture call is after the jank: what guarantees here that the posted
task goes ahead of the other PE handler calls triggered by user activities
during the jank?

[Navid Zolghadr, 2016/07/15 20:41:01]

Nothing I guess. Actually before we fire every pointerevent we make sure to run
that processPendingPointerCapture anyway through
processCaptureAndPositionOfPointerEvent. So we never miss sending
got/lostpointercapture before the next pointerevent.

 {
     UseCounter::count(m_frame->document(), UseCounter::PointerEventSetCapture);
-    if (m_pointerEventFactory.isActiveButtonsState(pointerId))
+    if (m_pointerEventFactory.isActiveButtonsState(pointerId)) {
+        bool wasCaptureStationary = !getPointerCaptureState(pointerId, nullptr, nullptr);
         m_pendingPointerCaptureTarget.set(pointerId, target);
+        // Only queue the processing pending pointer capture if the pending

[Rick Byers, 2016/07/15 17:35:32]

nit: Add a comment referencing the github issue where we've debated the
semantics (since it may still change).

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+        // capture target and capture target were the same to prevent infinite
+        // got/lostpointercapture event sequence.
+        if (wasCaptureStationary) {
+            m_frame->document()->getExecutionContext()->postTask(

[Rick Byers, 2016/07/15 17:35:31]

Looking at this more, I think you probably should be using the
document()->postTask - if for nothing more than to be consistent with the wide
variety of existing uses (they can all be migrated at once).

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+                BLINK_FROM_HERE,
+                createSameThreadTask(
+                    &PointerEventManager::immediateProcessPendingPointerCapture,
+                    WTF::unretained(this), pointerId));
+        }
+    }
 }
 
 void PointerEventManager::releasePointerCapture(int pointerId, EventTarget* target)
@@ -770,7 +833,18 @@ void PointerEventManager::releasePointerCapture(int pointerId, EventTarget* targ
 
 void PointerEventManager::releasePointerCapture(int pointerId)
 {
+    bool wasCaptureStationary = !getPointerCaptureState(pointerId, nullptr, nullptr);
     m_pendingPointerCaptureTarget.remove(pointerId);
+    // Only queue the processing pending pointer capture if the pending

[Rick Byers, 2016/07/15 17:35:32]

please avoid copy/paste - share this code

[Navid Zolghadr, 2016/07/15 20:41:01]

Done.

+    // capture target and capture target were the same to prevent infinite
+    // got/lostpointercapture event sequence.
+    if (wasCaptureStationary) {
+        m_frame->document()->getExecutionContext()->postTask(
+            BLINK_FROM_HERE,
+            createSameThreadTask(
+                &PointerEventManager::immediateProcessPendingPointerCapture,
+                WTF::unretained(this), pointerId));
+    }
 }
 
 bool PointerEventManager::isActive(const int pointerId) const