Issue 240343002: Track event handlers on windows

Issue 240343002: Track event handlers on windows (Closed)

Created:
6 years, 8 months ago by Sami

Modified:
6 years, 7 months ago

Reviewers:
Rick Byers, abarth-chromium

CC:
blink-reviews, sof, eae+blinkwatch, dglazkov+blink, adamk+blink_chromium.org, Inactive, rwlbuis

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Track event handlers on windows This patch makes EventHandlerRegistry keep track of event handlers registered on the window object. TEST=fast/events/scroll-event-handler-count.html,fast/events/scroll-event-handler-reused-window.html BUG=359566 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173552

Patch Set 1 #

Patch Set 2 : Rebased. #

Patch Set 3 : Reworked against refactorings, WIP. #

Patch Set 4 : Improved test coverage and implemented remaining bits. #

Patch Set 5 : Fixed assertions not to go through the document. #

Total comments: 11

Patch Set 6 : Simplification. #

Patch Set 7 : Better description for window reuse test. #

Total comments: 1

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+128 lines, -23 lines)			Patch
A +	LayoutTests/fast/events/resources/conclude-test-in-parent.html	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	LayoutTests/fast/events/scroll-event-handler-count.html	View	1 2 3	8 chunks	+51 lines, -12 lines	0 comments	Download
M	LayoutTests/fast/events/scroll-event-handler-count-expected.txt	View	1 2 3	3 chunks	+16 lines, -3 lines	0 comments	Download
A	LayoutTests/fast/events/scroll-event-handler-reused-window.html	View	1 2 3 4 5 6	1 chunk	+25 lines, -0 lines	0 comments	Download
A +	LayoutTests/fast/events/scroll-event-handler-reused-window-expected.txt	View	3 4 5	1 chunk	+2 lines, -3 lines	0 comments	Download
M	Source/core/frame/DOMWindow.cpp	View	1 2 3 4 5	5 chunks	+11 lines, -0 lines	1 comment	Download
M	Source/core/frame/EventHandlerRegistry.cpp	View	1 2 3 4 5	4 chunks	+22 lines, -5 lines	0 comments	Download

Messages

Total messages: 15 (0 generated)

Expand Messages | Collapse Messages

Rick Byers

Seems reasonable overall. Just a few questions. https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/scroll-event-handler-reused-window.html File LayoutTests/fast/events/scroll-event-handler-reused-window.html (right): https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/scroll-event-handler-reused-window.html#newcode1 LayoutTests/fast/events/scroll-event-handler-reused-window.html:1: <!DOCTYPE html> ...

6 years, 7 months ago (2014-05-06 20:54:39 UTC) #2

Seems reasonable overall.  Just a few questions.

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
File LayoutTests/fast/events/scroll-event-handler-reused-window.html (right):

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
LayoutTests/fast/events/scroll-event-handler-reused-window.html:1: <!DOCTYPE
html>
missing expected output for this test

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
LayoutTests/fast/events/scroll-event-handler-reused-window.html:4:
description('Test tracking event handlers with a reused DOMWindow.');
can you elaborate on what you mean by "reused DOMWindow" here?

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:333:
m_frame->host()->eventHandlerRegistry().didMoveIntoFrameHost(*this);
Please add a comment describing why this is necessary.  Naively I don't see how
there can be any event handlers to attach to the registry at this point when the
window is just being created now.  Ah, I see now - it's to ensure the observer
is created.

I think separate windowCreated/windowDestroyed entrypoints into the registry
might be clearer.  Then you could ASSERT in in windowCreated that there are no
event handlers on the window (and maybe even assert in windowDestroyed that
there aren't any handlers left in the registry if we can get deterministic
cleanup).  WDYT?

Overall it seems like this might be a lot of extra bookkeeping in order to
robustly support the DOMWindowLifecycleObserver abstraction.  Since every
DOMWindow should always have exactly one FrameHost, perhaps this would get a lot
simpler (and more robust) if we just hard-coded calls into EventHandlerRegistry
in DOMWindow::*eventListener (as the touch event handlers are tracked today)?

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:529: void DOMWindow::willDetachFrameHost()
Does this only happen when a window is being destroyed?  Is there any way we can
verify that event handlers aren't added after this point?

Sami

Thanks Rick, all comments addressed. https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/scroll-event-handler-reused-window.html File LayoutTests/fast/events/scroll-event-handler-reused-window.html (right): https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/scroll-event-handler-reused-window.html#newcode1 LayoutTests/fast/events/scroll-event-handler-reused-window.html:1: <!DOCTYPE html> On 2014/05/06 ...

6 years, 7 months ago (2014-05-07 10:36:40 UTC) #3

Thanks Rick, all comments addressed.

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
File LayoutTests/fast/events/scroll-event-handler-reused-window.html (right):

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
LayoutTests/fast/events/scroll-event-handler-reused-window.html:1: <!DOCTYPE
html>
On 2014/05/06 20:54:39, Rick Byers wrote:
> missing expected output for this test

Oops, added.

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
LayoutTests/fast/events/scroll-event-handler-reused-window.html:4:
description('Test tracking event handlers with a reused DOMWindow.');
On 2014/05/06 20:54:39, Rick Byers wrote:
> can you elaborate on what you mean by "reused DOMWindow" here?

Sure, I've added a comment here:

// |window.open| returns a DOMWindow synchronously without waiting for the
// network load to finish. Once the load completes, we transition the existing
// window object to the document instead of creating a new one like we normally
// would. The purpose of this test is to ensure event handlers added on the
// window are still present after the window is reused with the new document.
// See DocumentLoader.cpp for details.

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:333:
m_frame->host()->eventHandlerRegistry().didMoveIntoFrameHost(*this);
On 2014/05/06 20:54:39, Rick Byers wrote:
> Please add a comment describing why this is necessary.  Naively I don't see
how
> there can be any event handlers to attach to the registry at this point when
the
> window is just being created now.  Ah, I see now - it's to ensure the observer
> is created.
> 
> I think separate windowCreated/windowDestroyed entrypoints into the registry
> might be clearer.  Then you could ASSERT in in windowCreated that there are no
> event handlers on the window (and maybe even assert in windowDestroyed that
> there aren't any handlers left in the registry if we can get deterministic
> cleanup).  WDYT?
> 
> Overall it seems like this might be a lot of extra bookkeeping in order to
> robustly support the DOMWindowLifecycleObserver abstraction.  Since every
> DOMWindow should always have exactly one FrameHost, perhaps this would get a
lot
> simpler (and more robust) if we just hard-coded calls into
EventHandlerRegistry
> in DOMWindow::*eventListener (as the touch event handlers are tracked today)?

Yeah, I was thinking along the same lines -- it would be much simpler just to
add the few hooks that we need to DOMWindow. I've now done that and I think the
result is much cleaner. WDYT?

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:529: void DOMWindow::willDetachFrameHost()
On 2014/05/06 20:54:39, Rick Byers wrote:
> Does this only happen when a window is being destroyed? Is there any way we 
> can verify that event handlers aren't added after this point?

I don't think we need to worry about that. If handlers are attached after
FrameHost goes away, they shouldn't appear in the registry anyway. There's also
no need to import such handlers later, since as far as I can tell DOMWindow
can't attach to a different FrameHost.

The critical thing is to make sure we unregister all existing handlers before
the FrameHost goes away. The asserts in EventHandlerRegistry::checkConsistency()
check for that.

Rick Byers

lgtm https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/scroll-event-handler-reused-window.html File LayoutTests/fast/events/scroll-event-handler-reused-window.html (right): https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/scroll-event-handler-reused-window.html#newcode4 LayoutTests/fast/events/scroll-event-handler-reused-window.html:4: description('Test tracking event handlers with a reused DOMWindow.'); ...

6 years, 7 months ago (2014-05-07 13:28:20 UTC) #4

lgtm

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
File LayoutTests/fast/events/scroll-event-handler-reused-window.html (right):

https://codereview.chromium.org/240343002/diff/80001/LayoutTests/fast/events/...
LayoutTests/fast/events/scroll-event-handler-reused-window.html:4:
description('Test tracking event handlers with a reused DOMWindow.');
On 2014/05/07 10:36:41, Sami wrote:
> On 2014/05/06 20:54:39, Rick Byers wrote:
> > can you elaborate on what you mean by "reused DOMWindow" here?
> 
> Sure, I've added a comment here:
> 
> // |window.open| returns a DOMWindow synchronously without waiting for the
> // network load to finish. Once the load completes, we transition the existing
> // window object to the document instead of creating a new one like we
normally
> // would. The purpose of this test is to ensure event handlers added on the
> // window are still present after the window is reused with the new document.
> // See DocumentLoader.cpp for details.

Fascinating, thanks!

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
File Source/core/frame/DOMWindow.cpp (right):

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:333:
m_frame->host()->eventHandlerRegistry().didMoveIntoFrameHost(*this);
On 2014/05/07 10:36:41, Sami wrote:
> On 2014/05/06 20:54:39, Rick Byers wrote:
> > Please add a comment describing why this is necessary.  Naively I don't see
> how
> > there can be any event handlers to attach to the registry at this point when
> the
> > window is just being created now.  Ah, I see now - it's to ensure the
observer
> > is created.
> > 
> > I think separate windowCreated/windowDestroyed entrypoints into the registry
> > might be clearer.  Then you could ASSERT in in windowCreated that there are
no
> > event handlers on the window (and maybe even assert in windowDestroyed that
> > there aren't any handlers left in the registry if we can get deterministic
> > cleanup).  WDYT?
> > 
> > Overall it seems like this might be a lot of extra bookkeeping in order to
> > robustly support the DOMWindowLifecycleObserver abstraction.  Since every
> > DOMWindow should always have exactly one FrameHost, perhaps this would get a
> lot
> > simpler (and more robust) if we just hard-coded calls into
> EventHandlerRegistry
> > in DOMWindow::*eventListener (as the touch event handlers are tracked
today)?
> 
> Yeah, I was thinking along the same lines -- it would be much simpler just to
> add the few hooks that we need to DOMWindow. I've now done that and I think
the
> result is much cleaner. WDYT?

Yeah this seems a lot cleaner.  The observer abstraction is a nice idea, but in
this case I don't think it's worth the extra lifetime management complexity.

https://codereview.chromium.org/240343002/diff/80001/Source/core/frame/DOMWin...
Source/core/frame/DOMWindow.cpp:529: void DOMWindow::willDetachFrameHost()
On 2014/05/07 10:36:41, Sami wrote:
> On 2014/05/06 20:54:39, Rick Byers wrote:
> > Does this only happen when a window is being destroyed? Is there any way we 
> > can verify that event handlers aren't added after this point?
> 
> I don't think we need to worry about that. If handlers are attached after
> FrameHost goes away, they shouldn't appear in the registry anyway. There's
also
> no need to import such handlers later, since as far as I can tell DOMWindow
> can't attach to a different FrameHost.
> 
> The critical thing is to make sure we unregister all existing handlers before
> the FrameHost goes away. The asserts in
EventHandlerRegistry::checkConsistency()
> check for that.

Thanks, sounds reasonable.

abarth-chromium

https://codereview.chromium.org/240343002/diff/110001/Source/core/frame/DOMWindow.cpp File Source/core/frame/DOMWindow.cpp (right): https://codereview.chromium.org/240343002/diff/110001/Source/core/frame/DOMWindow.cpp#newcode1515 Source/core/frame/DOMWindow.cpp:1515: lifecycleNotifier().notifyAddEventListener(this, eventType); We can't listen for this notification via ...

6 years, 7 months ago (2014-05-07 17:14:01 UTC) #6

Sami

On 2014/05/07 17:14:01, abarth wrote: > We can't listen for this notification via this pathway? ...

6 years, 7 months ago (2014-05-07 17:16:30 UTC) #7

abarth-chromium

I see, the problem is that LifecycleObserver only lets you observe one thing at a ...

6 years, 7 months ago (2014-05-07 17:36:03 UTC) #8

abarth-chromium

Ok LGTM. Hopefully we can move the other special cases in these functions into EventRegistry ...

6 years, 7 months ago (2014-05-07 17:38:26 UTC) #9

Sami

On 2014/05/07 17:38:26, abarth wrote: > Ok LGTM. Hopefully we can move the other special ...

6 years, 7 months ago (2014-05-07 17:42:08 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/240343002/110001

6 years, 7 months ago (2014-05-07 17:42:51 UTC) #12

Rick Byers

On 2014/05/07 17:42:08, Sami wrote: > On 2014/05/07 17:38:26, abarth wrote: > > Ok LGTM. ...

6 years, 7 months ago (2014-05-07 17:45:25 UTC) #13

Sami

On 2014/05/07 17:45:25, Rick Byers wrote: > Hah hah, you mean: "some of us already ...

6 years, 7 months ago (2014-05-07 17:51:36 UTC) #14

Message was sent while issue was closed.

Change committed as 173552

Expand Messages | Collapse Messages