Delegate dragend to the correct frame's EventHandler.

Delegate dragend to the correct frame's EventHandler.

This is a re-land of https://crrev.com/32e50971f32bcd21be8540758efe9aac0ffafcd9
whose description is below.

Currently, most drag-and-drop events are correctly delegated to the
EventHandler of the frame that contains the events' target elements, in
EventHandler::updateDragAndDrop. However,
EventHandler::dragSourceEndedAt does not implement this delegation.  The
issue is visible to the Web in a subtle way -- the frame of the
EventHandler is used to populate the DragEvent's view, which is used to
compute the clientX and clientY attributes, which are user-visible.

This CL implements the delegation used by
MouseEventManager::dispatchDragSrcEvent, into
MouseEventManager::dispatchDragSrcEvent.

BUG=618770
Committed: https://crrev.com/fb74d203dc9797b1e12592cac7c86f1d2b923eab
Cr-Commit-Position: refs/heads/master@{#430432}

[pwnall, 2016-11-04 12:47:06]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 12:47:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2016-11-04 13:19:40]

pwnall@chromium.org changed reviewers:
+ dcheng@chromium.org

[pwnall, 2016-11-04 13:19:41]

Can you please take another look?

The ASAN trybots don't run layout tests [1, 2], but I ran this on my desktop.
The problematic test passes, and the tests under fast/dnd pass.

[1] http://crbug.com/361678
[2] http://crbug.com/526188

[commit-bot: I haz the power, 2016-11-04 13:51:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 13:51:53]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1984:
m_mouseEventManager->dragSourceEndedAt(event, operation);
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.

[Navid Zolghadr, 2016-11-04 15:29:05]

nzolghadr@chromium.org changed reviewers:
+ nzolghadr@chromium.org

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

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1970: // Asides from
routing the event to the correct frame, the hit test is also an
Can we have this whole block of code in MouseEventManager instead?

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

Thank you very much for the quick feedback!

Here are some thoughts below. I'll get back to reading the code and try to get a
better understanding of it.

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1970: // Asides from
routing the event to the correct frame, the hit test is also an
On 2016/11/04 15:29:06, Navid Zolghadr wrote:
> Can we have this whole block of code in MouseEventManager instead?

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...

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1984:
m_mouseEventManager->dragSourceEndedAt(event, operation);
On 2016/11/04 15:12:15, dcheng wrote:
> 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.

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]

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1970: // Asides from
routing the event to the correct frame, the hit test is also an
On 2016/11/04 18:24:29, pwnall wrote:
> On 2016/11/04 15:29:06, Navid Zolghadr wrote:
> > Can we have this whole block of code in MouseEventManager instead?
> 
> 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...

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.

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1984:
m_mouseEventManager->dragSourceEndedAt(event, operation);
On 2016/11/04 18:24:29, pwnall wrote:
> On 2016/11/04 15:12:15, dcheng wrote:
> > 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.
> 
> 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

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:38:32]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 22:39:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thank you very much for your explanations! PTAL?

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1970: // Asides from
routing the event to the correct frame, the hit test is also an
On 2016/11/04 18:33:22, Navid Zolghadr wrote:
> On 2016/11/04 18:24:29, pwnall wrote:
> > On 2016/11/04 15:29:06, Navid Zolghadr wrote:
> > > Can we have this whole block of code in MouseEventManager instead?
> > 
> > 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...
> 
> 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.

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.

https://codereview.chromium.org/2471843005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1984:
m_mouseEventManager->dragSourceEndedAt(event, operation);
On 2016/11/04 18:33:22, Navid Zolghadr wrote:
> On 2016/11/04 18:24:29, pwnall wrote:
> > On 2016/11/04 15:12:15, dcheng wrote:
> > > 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.
> > 
> > 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
> 
> 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.

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.

[commit-bot: I haz the power, 2016-11-05 00:05:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-05 00:05:57]

Dry run: This issue passed the CQ dry run.

[dcheng, 2016-11-07 22:15:54]

The srcdoc stuff is very clever... personally I'd prefer it if we wrote it out
inline somehow, since that makes it easier to understand the structure of tests
at a glance.

LGTM though

[pwnall, 2016-11-07 22:24:30]

The CQ bit was checked by pwnall@chromium.org

[commit-bot: I haz the power, 2016-11-07 22:25:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2016-11-07 22:28:28]

On 2016/11/07 22:15:54, dcheng wrote:
> The srcdoc stuff is very clever... personally I'd prefer it if we wrote it out
> inline somehow, since that makes it easier to understand the structure of
tests
> at a glance.

I agree, but I couldn't think of a better way. If you have one, I'd be happy to
update the test!

Here are the other approaches that I've considered:
* .html files in resources/ -- that seems strictly worse, as the files wouldn't
actually be shared between tests
* changing <iframe> to allow inline content -- asides from how hard that'd be to
spec, I think we still would not be able to inline <!DOCTYPE html>, as that
trips up the parser

[commit-bot: I haz the power, 2016-11-08 00:07:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-08 00:33:08]

Description was changed from

==========
Delegate dragend to the correct frame's EventHandler.

This is a re-land of https://crrev.com/32e50971f32bcd21be8540758efe9aac0ffafcd9
whose description is below.

Currently, most drag-and-drop events are correctly delegated to the
EventHandler of the frame that contains the events' target elements, in
EventHandler::updateDragAndDrop. However,
EventHandler::dragSourceEndedAt does not implement this delegation.  The
issue is visible to the Web in a subtle way -- the frame of the
EventHandler is used to populate the DragEvent's view, which is used to
compute the clientX and clientY attributes, which are user-visible.

This CL implements the delegation used by
MouseEventManager::dispatchDragSrcEvent, into
MouseEventManager::dispatchDragSrcEvent.

BUG=618770
==========

to

==========
Delegate dragend to the correct frame's EventHandler.

This is a re-land of https://crrev.com/32e50971f32bcd21be8540758efe9aac0ffafcd9
whose description is below.

Currently, most drag-and-drop events are correctly delegated to the
EventHandler of the frame that contains the events' target elements, in
EventHandler::updateDragAndDrop. However,
EventHandler::dragSourceEndedAt does not implement this delegation.  The
issue is visible to the Web in a subtle way -- the frame of the
EventHandler is used to populate the DragEvent's view, which is used to
compute the clientX and clientY attributes, which are user-visible.

This CL implements the delegation used by
MouseEventManager::dispatchDragSrcEvent, into
MouseEventManager::dispatchDragSrcEvent.

BUG=618770

Committed: https://crrev.com/fb74d203dc9797b1e12592cac7c86f1d2b923eab
Cr-Commit-Position: refs/heads/master@{#430432}
==========

[commit-bot: I haz the power, 2016-11-08 00:33:09]

Patchset 3 (id:??) landed as
https://crrev.com/fb74d203dc9797b1e12592cac7c86f1d2b923eab
Cr-Commit-Position: refs/heads/master@{#430432}