Delegate dragend to the correct frame's EventHandler.

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights
  * reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
@@ skipping 1949 matching lines @@
   return m_keyboardEventManager->keyEvent(initialKeyEvent);
 }
 
 void EventHandler::defaultKeyboardEventHandler(KeyboardEvent* event) {
   m_keyboardEventManager->defaultKeyboardEventHandler(
       event, m_mouseEventManager->mousePressNode());
 }
 
 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.

 }
 
 void EventHandler::updateDragStateAfterEditDragIfNeeded(
     Element* rootEditableElement) {
   // If inserting the dragged contents removed the drag source, we still want to
   // fire dragend at the root editble element.
   if (m_mouseEventManager->dragState().m_dragSrc &&
       !m_mouseEventManager->dragState().m_dragSrc->isConnected())
     m_mouseEventManager->dragState().m_dragSrc = rootEditableElement;
 }
@@ skipping 105 matching lines @@
 }
 
 FrameHost* EventHandler::frameHost() const {
   if (!m_frame->page())
     return nullptr;
 
   return &m_frame->page()->frameHost();
 }
 
 }  // namespace blink