Delegate dragend to the correct frame's EventHandler.

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

Index: third_party/WebKit/Source/core/input/EventHandler.cpp
diff --git a/third_party/WebKit/Source/core/input/EventHandler.cpp b/third_party/WebKit/Source/core/input/EventHandler.cpp
index a40a5bc065dff8cd721b95a119fcf4bc36e0dad3..ebf0f6bf20e7194bf30fd532563885dad1344a7f 100644
--- a/third_party/WebKit/Source/core/input/EventHandler.cpp
+++ b/third_party/WebKit/Source/core/input/EventHandler.cpp
@@ -1967,6 +1967,20 @@ void EventHandler::defaultKeyboardEventHandler(KeyboardEvent* event) {
 
 void EventHandler::dragSourceEndedAt(const PlatformMouseEvent& event,
                                      DragOperation operation) {
+  // Asides from routing the event to the correct frame, the hit test is also an

[Navid Zolghadr, 2016/11/04 15:29:06]

Can we have this whole block of code in MouseEventManager instead?

[pwnall, 2016/11/04 18:24:29]

This kind of code also exists in EventHandler::updateDragAndDrop [1], so I think
it belongs here. The duplication suggests refactoring opportunities, so I want
it to be as obvious as possible. FWIW, I do plan to refactor these areas of the
code, but I'd rather do that after I work through enough bugs to truly
understand how it works.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Eve...

[Navid Zolghadr, 2016/11/04 18:33:22]

MouseEventManager was intended to contain this and similar logic. I agree that
more refactoring can be done to move more logic and not everything yet fully
moved over there. But adding more stuff here might make it harder. If you would
like to do that refactoring later that is fine with me.

[pwnall, 2016/11/04 22:40:39]

Thank you very much for understanding!

This is a re-land of a CL that got reverted because of LSAN failures in layout
tests. We don't have coverage for those tests on the trybots, so I'd really
prefer to keep the re-landed CL as small as possible.

+  // opportunity for Layer to update the :hover and :active pseudoclasses.
+  HitTestRequest request(HitTestRequest::Release);
+  MouseEventWithHitTestResults mev =
+      EventHandlingUtil::performMouseEventHitTest(m_frame, request, event);
+
+  LocalFrame* targetFrame;
+  if (targetIsFrame(mev.innerNode(), targetFrame)) {
+    if (targetFrame)
+      targetFrame->eventHandler().dragSourceEndedAt(event, operation);
+  }
+
+  // This has to be called even if the dragend event is fired in another frame,
+  // in order to clean up the event manager's drag state.
   m_mouseEventManager->dragSourceEndedAt(event, operation);

[dcheng, 2016/11/04 15:12:15]

Hmm... that means this actually gets called twice on subframes, right? It just
happens to be idempotent but that feels a bit strange to me.

[pwnall, 2016/11/04 18:24:29]

I think that each LocalFrame (including subframes) has its own EventHandler that
has its own MouseEventManager, and I think that I need to clean up the state of
each MouseEventManager. (Admittedly, I don't understand all the code in
EventHandler / MouseEventManager well enough to know if all the
MouseEventManagers become aware of the drag-and-drop.) I think this call occurs
once per LocalFrame, because each EventHandler in the call stack is attached to
a different LocalFrame.

I'll get back to reading the code and try to understand it better. If you happen
to have some time to tell me where I'm wrong, I'd be grateful :D

[Navid Zolghadr, 2016/11/04 18:33:22]

that is correct. Each frame has its own EventHandler and each EventHandler has
some *Manager classes including MouseEventManager. Although there are some logic
in EventHandler that that belongs to page level and not iframe but we haven't
got to refactoring that out of iframe EventHandlers yet though.

[pwnall, 2016/11/04 22:40:39]

Thank you very much for explaining!

I put together a new test to check if the nested MouseEventManagers need to get
their state cleared, and it seems like that's not the case. I changed the code
so MouseEventManager::dragSourceEndedAt() is called exactly once in all the
cases, and LSAN seems to be happy with that on my machine.

 }