Track scroll event handlers in nested documents

Track scroll event handlers in nested documents
    
This patch moves the EventHandlerRegistry from the Document 
object to the Page so that it tracks handlers in all 
documents on the page. This makes it possible to do global 
queries for event handlers.
    
TEST=fast/events/scroll-event-handler-count.html
BUG=359566, 367297, 369082
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173101

[abarth-chromium, 2014-04-22 16:18:22]

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:23: suspendIfNeeded();
As a general pattern, we don't call suspendIfNeeded() inside constructors
because it can call back into virtual functions on |this| that might be
overridden in subclasses.  I understand that won't happen for this class because
it's FINAL, but it's a pattern we avoid generally in the code base to simplify
our lives.  Instead, we use a static |create| function that calls
suspendIfNeeded() after we're done constructing the object.

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:51: {
What's the point of having a separate DocumentObserver class?  Instead,
EventHandlerRegistry could inherit from DocumentLifecycleObserver and listen for
documentWasDetached.

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:163: }
Can we implement this algorithm iteratively instead of recursively?

[Sami, 2014-04-22 18:05:17]

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:23: suspendIfNeeded();
On 2014/04/22 16:18:23, abarth wrote:
> As a general pattern, we don't call suspendIfNeeded() inside constructors
> because it can call back into virtual functions on |this| that might be
> overridden in subclasses.  I understand that won't happen for this class
because
> it's FINAL, but it's a pattern we avoid generally in the code base to simplify
> our lives.  Instead, we use a static |create| function that calls
> suspendIfNeeded() after we're done constructing the object.

Okay, it sounds like I can move the call to suspendIfNeeded to the registry's
constructor instead.

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:51: {
On 2014/04/22 16:18:23, abarth wrote:
> What's the point of having a separate DocumentObserver class?  Instead,
> EventHandlerRegistry could inherit from DocumentLifecycleObserver and listen
for
> documentWasDetached.

First, it better isolates the implementation from the clients of
EventHandlerRegistry; an "EventHandlerRegistry is-a DocumentLifecycleObserver"
relation does not make any sense for them. Second, in another patch I'm planning
to add a window observer as a separated nested class. I think having code for
observing documents and DOM windows in separate classes makes things clearer.

https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:163: }
On 2014/04/22 16:18:23, abarth wrote:
> Can we implement this algorithm iteratively instead of recursively?

Sure, done. I'm having trouble deciding which way is better, but PTAL.

[abarth-chromium, 2014-04-22 23:39:45]

On 2014/04/22 18:05:17, Sami wrote:
>
https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
> Source/core/dom/EventHandlerRegistry.cpp:51: {
> On 2014/04/22 16:18:23, abarth wrote:
> > What's the point of having a separate DocumentObserver class?  Instead,
> > EventHandlerRegistry could inherit from DocumentLifecycleObserver and listen
> for
> > documentWasDetached.
> 
> First, it better isolates the implementation from the clients of
> EventHandlerRegistry; an "EventHandlerRegistry is-a DocumentLifecycleObserver"
> relation does not make any sense for them.

We use this pattern in many places in the code base.  You're right that we could
use private inheritance to keep the interface strict, but so far it hasn't
really been a problem to use public inheritance.

> Second, in another patch I'm planning
> to add a window observer as a separated nested class. I think having code for
> observing documents and DOM windows in separate classes makes things clearer.

I'd prefer to worry about that in the future when we're actually observing
DOMWindows.  Each CL should stand on its own as something sensible for the code
base.

>
https://codereview.chromium.org/237963014/diff/80001/Source/core/dom/EventHan...
> Source/core/dom/EventHandlerRegistry.cpp:163: }
> On 2014/04/22 16:18:23, abarth wrote:
> > Can we implement this algorithm iteratively instead of recursively?
> 
> Sure, done. I'm having trouble deciding which way is better, but PTAL.

We usually use recursion when walking down trees and iteration when walking up
trees, but I don't think we're all that consistent on that topic.

[abarth-chromium, 2014-04-22 23:40:25]

https://codereview.chromium.org/237963014/diff/100001/Source/core/dom/EventHa...
File Source/core/dom/EventHandlerRegistry.h (right):

https://codereview.chromium.org/237963014/diff/100001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.h:82: class DocumentObserver: public
ActiveDOMObject {
I still don't understand why we're using ActiveDOMObject rather than
DocumentLifecycleObserver.  This class isn't a DOM object.

[Sami, 2014-04-23 10:02:34]

Thanks again for your comments. I've reworked this to hopefully follow
established practices more closely. PTAL.

https://codereview.chromium.org/237963014/diff/100001/Source/core/dom/EventHa...
File Source/core/dom/EventHandlerRegistry.h (right):

https://codereview.chromium.org/237963014/diff/100001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.h:82: class DocumentObserver: public
ActiveDOMObject {
On 2014/04/22 23:40:26, abarth wrote:
> I still don't understand why we're using ActiveDOMObject rather than
> DocumentLifecycleObserver.  This class isn't a DOM object.

Sorry if that wasn't clear. The reason is that when the
DocumentLifecycleObserver::documentWasDetached() notification comes in, the
document no longer has an association with its previous parent, so it's too late
to unregister any handlers.

Originally I was going to add an earlier notification to the observer, but ended
up using ActiveDOMObject based on your suggestion here:
https://codereview.chromium.org/206603002/#msg19

[abarth-chromium, 2014-04-23 15:23:28]

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:30: , m_document(document)
We should be able to remove m_document now.  ActiveDOMObject already keeps a
pointer to the document.

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:48: registry->suspendIfNeeded();
Please add a static create function that does this work and make the constructor
private.

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:131: }
Is it possible for an active document to be contained in an inactive document? 
That seems impossible.

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:135:
registry->updateEventHandlerTargets(op, handlerClass, target);
So, each registry knows about all the event listeners on all descendant
documents?  Why do we need to keep this information redundantly at every level
of the tree?  It seems like each event listener should be represented exactly
once (e.g., either associated with its document or maybe with the top-level
document).

[Sami, 2014-04-23 16:44:09]

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:30: , m_document(document)
On 2014/04/23 15:23:28, abarth wrote:
> We should be able to remove m_document now.  ActiveDOMObject already keeps a
> pointer to the document.

Ah, good idea, done.

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:48: registry->suspendIfNeeded();
On 2014/04/23 15:23:28, abarth wrote:
> Please add a static create function that does this work and make the
constructor
> private.

Done.

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:131: }
On 2014/04/23 15:23:28, abarth wrote:
> Is it possible for an active document to be contained in an inactive document?

> That seems impossible.

Right, we can just check this at the leaf.

https://codereview.chromium.org/237963014/diff/120001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:135:
registry->updateEventHandlerTargets(op, handlerClass, target);
On 2014/04/23 15:23:28, abarth wrote:
> So, each registry knows about all the event listeners on all descendant
> documents?  Why do we need to keep this information redundantly at every level
> of the tree?  It seems like each event listener should be represented exactly
> once (e.g., either associated with its document or maybe with the top-level
> document).

No, each registry knows two things: 1) the set of handlers directly in its
associated document, and 2) for any direct descendant document whether there are
handlers anywhere in that subtree or not. That is, the full set of handlers is
not redundantly propagated up the tree. Note that this up-propagation also only
happens if it gives some new information to the parent (i.e., the first handler
was added or the last handler was removed in this subtree).

I don't think we want to register all handlers just to the top-level registry,
because that set will quickly grow very large.

[abarth-chromium, 2014-04-23 17:25:28]

On 2014/04/23 16:44:09, Sami wrote:
> I don't think we want to register all handlers just to the top-level registry,
> because that set will quickly grow very large.

What bad thing happens when the set grows large?

[abarth-chromium, 2014-04-23 17:25:53]

https://codereview.chromium.org/237963014/diff/140001/Source/core/dom/EventHa...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/140001/Source/core/dom/EventHa...
Source/core/dom/EventHandlerRegistry.cpp:156: target = document;
I see, I missed this line.

[Sami, 2014-04-23 17:54:03]

On 2014/04/23 17:25:28, abarth wrote:
> On 2014/04/23 16:44:09, Sami wrote:
> > I don't think we want to register all handlers just to the top-level
registry,
> > because that set will quickly grow very large.
> 
> What bad thing happens when the set grows large?

ScrollingCoordinator uses this data to tell the compositor about which areas of
the page contain touch event handlers. The bigger that set becomes, the more
time it will take to compute bounds for the handlers, and more critically, the
longer it will take for the compositor to do raycasting against this set as a
response to input events.

Since the compositor can deal with false positives (i.e., the data said there
was a handler at (x, y) when there really wasn't any) but not false negatives,
we can speed this up by grouping multiple handlers into a single set of bounds.
The enclosing document seems like a natural choice for grouping, so that's what
we're doing here.

To avoid making this too pessimistic, we're still computing exact handler bounds
for the root document. Some folks in input-dev are looking into whether we
should do some further approximations to make this even faster, but in any case
I'd just like to keep the current behavior for now.

Some further details here:
http://www.chromium.org/developers/design-documents/compositor-hit-testing

[abarth-chromium, 2014-04-23 20:10:27]

It makes sense to summarize the information for use by the compositor, but I
don't understand why the frame hierarchy is the right place to do the
summarization.  Why not gather all the data at the root of the tree and the
decide how best to summarize it?

[Sami, 2014-04-24 11:00:41]

On 2014/04/23 20:10:27, abarth wrote:
> It makes sense to summarize the information for use by the compositor, but I
> don't understand why the frame hierarchy is the right place to do the
> summarization.  Why not gather all the data at the root of the tree and the
> decide how best to summarize it?

The point of doing this in the frame hierarchy is to throw away unneeded data at
the source instead of having an expensive optimization step later that prunes
the tree. If there was a reason to think ScrollingCoordinator could do a better
job by having access to the global set of handlers, then I would agree that
having a full root registry would make sense. Lacking that reason, I don't think
we should spend the CPU cycles to maintaining data that no-one needs.

The downside is that this makes the handler registry less generic than
otherwise, but again, I think the efficiency gains make it a worthwhile
trade-off.

Rick, do you have thoughts on whether pruning based on the frame hierarchy is a
good model going forward? If I understood right, you were considering coalescing
even more of the handler data?

[abarth-chromium, 2014-04-24 15:03:21]

There's nothing special about frame boundaries.  The approach in this CL just
coalesces at arbitrary points in the tree, which makes this code more
complicated than it would be otherwise.

[Rick Byers, 2014-04-24 17:02:39]

On 2014/04/24 11:00:41, Sami wrote:
> On 2014/04/23 20:10:27, abarth wrote:
> > It makes sense to summarize the information for use by the compositor, but I
> > don't understand why the frame hierarchy is the right place to do the
> > summarization.  Why not gather all the data at the root of the tree and the
> > decide how best to summarize it?
> 
> The point of doing this in the frame hierarchy is to throw away unneeded data
at
> the source instead of having an expensive optimization step later that prunes
> the tree. If there was a reason to think ScrollingCoordinator could do a
better
> job by having access to the global set of handlers, then I would agree that
> having a full root registry would make sense. Lacking that reason, I don't
think
> we should spend the CPU cycles to maintaining data that no-one needs.
> 
> The downside is that this makes the handler registry less generic than
> otherwise, but again, I think the efficiency gains make it a worthwhile
> trade-off.
> 
> Rick, do you have thoughts on whether pruning based on the frame hierarchy is
a
> good model going forward? If I understood right, you were considering
coalescing
> even more of the handler data?

On 2014/04/24 15:03:21, abarth wrote:
> There's nothing special about frame boundaries.  The approach in this CL just
> coalesces at arbitrary points in the tree, which makes this code more
> complicated than it would be otherwise.

I suspect the reason touch hit rects are tracked per document is just an
accident of history (When Levi first wrote the tracking I believe it didn't work
for iframes at all, and he later added it on).  For touch rect tracking a global
registry would be just as good (we essentially turn it into that anyway:
accumulateDocumentTouchEventTargetRects recrusively walks documents building up
a single data structure mapping RenderLayers to vectors of rects).

The one thing I'm unsure of here is how this all should play with out-of-process
iframes.  When a page is split across multiple processes, we almost certainly
want one registry per process instead of a global one for the page.  Some major
surgery will probably be required for that anyway, so I don't think we need to
worry about it too much now.

Sami, if you switch to a global registry then I'm happy to take over the patch
that moves touch handler tracking to your registry and update the hit-rect
computation code at the same time if you prefer.  I _think_ it should be pretty
straight forward to change it to use a global registry.  All the messy work of
walking the RenderLayer tree across frames (and the LayerChildFrameMap
optimization) still stays the same, we probably just need to update
accumulateDocumentTouchEventTargetRects to remove the recursion when it hits
document nodes.

[Rick Byers, 2014-04-24 17:08:13]

On 2014/04/24 17:02:39, Rick Byers wrote:
> On 2014/04/24 11:00:41, Sami wrote:
> > On 2014/04/23 20:10:27, abarth wrote:
> > > It makes sense to summarize the information for use by the compositor, but
I
> > > don't understand why the frame hierarchy is the right place to do the
> > > summarization.  Why not gather all the data at the root of the tree and
the
> > > decide how best to summarize it?
> > 
> > The point of doing this in the frame hierarchy is to throw away unneeded
data
> at
> > the source instead of having an expensive optimization step later that
prunes
> > the tree. If there was a reason to think ScrollingCoordinator could do a
> better
> > job by having access to the global set of handlers, then I would agree that
> > having a full root registry would make sense. Lacking that reason, I don't
> think
> > we should spend the CPU cycles to maintaining data that no-one needs.
> > 
> > The downside is that this makes the handler registry less generic than
> > otherwise, but again, I think the efficiency gains make it a worthwhile
> > trade-off.
> > 
> > Rick, do you have thoughts on whether pruning based on the frame hierarchy
is
> a
> > good model going forward? If I understood right, you were considering
> coalescing
> > even more of the handler data?
> 
> On 2014/04/24 15:03:21, abarth wrote:
> > There's nothing special about frame boundaries.  The approach in this CL
just
> > coalesces at arbitrary points in the tree, which makes this code more
> > complicated than it would be otherwise.
> 
> I suspect the reason touch hit rects are tracked per document is just an
> accident of history (When Levi first wrote the tracking I believe it didn't
work
> for iframes at all, and he later added it on).  For touch rect tracking a
global
> registry would be just as good (we essentially turn it into that anyway:
> accumulateDocumentTouchEventTargetRects recrusively walks documents building
up
> a single data structure mapping RenderLayers to vectors of rects).
> 
> The one thing I'm unsure of here is how this all should play with
out-of-process
> iframes.  When a page is split across multiple processes, we almost certainly
> want one registry per process instead of a global one for the page.  Some
major
> surgery will probably be required for that anyway, so I don't think we need to
> worry about it too much now.
> 
> Sami, if you switch to a global registry then I'm happy to take over the patch
> that moves touch handler tracking to your registry and update the hit-rect
> computation code at the same time if you prefer.  I _think_ it should be
pretty
> straight forward to change it to use a global registry.  All the messy work of
> walking the RenderLayer tree across frames (and the LayerChildFrameMap
> optimization) still stays the same, we probably just need to update
> accumulateDocumentTouchEventTargetRects to remove the recursion when it hits
> document nodes.

I was just looking to see if there's likely to be any tricky lifetime issues
(eg. clearing handlers properly at Document detach time).  It looks to me like
that it relatively sane now (I fixed a bunch of issues here and simplified it to
be more predictable), so I don't expect any problems moving to a global
registry.  I agree it'll make the code simpler.

[Sami, 2014-04-24 17:19:30]

Thanks Rick. Since I didn't write the original code this is replacing, I ran out
of anecdata for defending it and went ahead and changed this to using a global
registry. It's mostly straightforward, except for the detaching case where we
need to individually remove all targets under the detached subtree. I also think
the API is a little funny now since
EventHandlerRegistry::from(document)->hasEventHandlers() will tell you about all
handlers in the frame hierarchy instead of just under that one document. Maybe
that should move to Page or something.

Maybe I misread accumulateDocumentTouchEventTargetRects but isn't it already
terminating the iteration at document nodes?

[abarth-chromium, 2014-04-24 17:25:35]

On 2014/04/24 17:19:30, Sami wrote:
> EventHandlerRegistry::from(document)->hasEventHandlers() will tell you about
all
> handlers in the frame hierarchy instead of just under that one document. Maybe
> that should move to Page or something.

Yeah, we should probably hang it off Page.

[Rick Byers, 2014-04-24 17:34:59]

On 2014/04/24 17:19:30, Sami wrote:
> Thanks Rick. Since I didn't write the original code this is replacing, I ran
out
> of anecdata for defending it and went ahead and changed this to using a global
> registry. It's mostly straightforward, except for the detaching case where we
> need to individually remove all targets under the detached subtree.

The existing touch handler tracking code (which may be broken of course) does
this implicitly via the removeAllEventListenersRecursively call in ~Document. 
It walks the whole DOM tree for the document, and invokes didClearEventListeners
on each Node.

Is there some reason we need to clear at ::detach time instead of leaving them
around to be cleaned up at document destruction time like they are today? 
Naively if the event handlers are still active, then they should be in the
registry (eg. maybe a detach / reattach scenario where handlers are preserved?).

>  I also think
> the API is a little funny now since
> EventHandlerRegistry::from(document)->hasEventHandlers() will tell you about
all
> handlers in the frame hierarchy instead of just under that one document. Maybe
> that should move to Page or something.

Yes, if it's going to be global then I don't think it should be a
DocumentSupplement anymore.  Presumably hanging off of Page like
ScrollingCoordinator is the right way to go (again, not sure about site
isolation here).

> Maybe I misread accumulateDocumentTouchEventTargetRects but isn't it already
> terminating the iteration at document nodes?

See the recursive call:
  if (target->isDocumentNode())
    accumulateDocumentTouchEventTargetRects(rects, toDocument(target));

So it's doing a full walk of the page.

I had originally hoped that we'd be able to do some optimizations where changes
in one doc wouldn't require recomputing rects from all docs, but that seems
unlikely now (there will always be non-composited iframes, so there's always
some chance that changes in one doc will affect the rects computed for it's
parent).

[Sami, 2014-04-24 17:46:56]

On 2014/04/24 17:34:59, Rick Byers wrote:
> Yes, if it's going to be global then I don't think it should be a
> DocumentSupplement anymore.  Presumably hanging off of Page like
> ScrollingCoordinator is the right way to go (again, not sure about site
> isolation here).

Right. We still need a per-document supplement to keep track of the individual
handlers though.
 
> See the recursive call:
>   if (target->isDocumentNode())
>     accumulateDocumentTouchEventTargetRects(rects, toDocument(target));
> 
> So it's doing a full walk of the page.

Ah, I mistook the first part of that function as an early-out, but it only fires
if one of the targets is the document itself, not just any document.

[Rick Byers, 2014-04-24 17:54:42]

On 2014/04/24 17:46:56, Sami wrote:
> On 2014/04/24 17:34:59, Rick Byers wrote:
> > Yes, if it's going to be global then I don't think it should be a
> > DocumentSupplement anymore.  Presumably hanging off of Page like
> > ScrollingCoordinator is the right way to go (again, not sure about site
> > isolation here).
> 
> Right. We still need a per-document supplement to keep track of the individual
> handlers though.

Why?  Can't this also be in a page-global object?  Other than handling
Document::detach, what's fundamentally coupled to a Document instance here? 

> > See the recursive call:
> >   if (target->isDocumentNode())
> >     accumulateDocumentTouchEventTargetRects(rects, toDocument(target));
> > 
> > So it's doing a full walk of the page.
> 
> Ah, I mistook the first part of that function as an early-out, but it only
fires
> if one of the targets is the document itself, not just any document.

Yeah that's an optimization if the current document has a handler (can simply
mark the RenderView and skip any descendants) - very handy short-circuit in
practice (a good chunk of mobile sites fall into this bucket - makes touch rect
tracking trivially cheap since it won't provide any value anyway).

[Sami, 2014-04-24 18:06:03]

On 2014/04/24 17:54:42, Rick Byers wrote:
> On 2014/04/24 17:46:56, Sami wrote:
> > On 2014/04/24 17:34:59, Rick Byers wrote:
> > > Yes, if it's going to be global then I don't think it should be a
> > > DocumentSupplement anymore.  Presumably hanging off of Page like
> > > ScrollingCoordinator is the right way to go (again, not sure about site
> > > isolation here).
> > 
> > Right. We still need a per-document supplement to keep track of the
individual
> > handlers though.
> 
> Why?  Can't this also be in a page-global object?  Other than handling
> Document::detach, what's fundamentally coupled to a Document instance here? 

Yes, detach was the case I was thinking of. Now that I think about it more, is
the call to Document::didClearTouchEventHandlers() in Document::detach only
there because handlers in children get collapsed to the parent document? If so,
then there's no need to do anything special in detach.

[Rick Byers, 2014-04-24 18:09:43]

On 2014/04/24 18:06:03, Sami wrote:
> On 2014/04/24 17:54:42, Rick Byers wrote:
> > On 2014/04/24 17:46:56, Sami wrote:
> > > On 2014/04/24 17:34:59, Rick Byers wrote:
> > > > Yes, if it's going to be global then I don't think it should be a
> > > > DocumentSupplement anymore.  Presumably hanging off of Page like
> > > > ScrollingCoordinator is the right way to go (again, not sure about site
> > > > isolation here).
> > > 
> > > Right. We still need a per-document supplement to keep track of the
> individual
> > > handlers though.
> > 
> > Why?  Can't this also be in a page-global object?  Other than handling
> > Document::detach, what's fundamentally coupled to a Document instance here? 
> 
> Yes, detach was the case I was thinking of. Now that I think about it more, is
> the call to Document::didClearTouchEventHandlers() in Document::detach only
> there because handlers in children get collapsed to the parent document? If
so,
> then there's no need to do anything special in detach.

Yeah I think it's there only because the parent shouldn't be pointing at
documents that aren't necessarily it's children (eg. the document could be
re-attached elsewhere on the page).  If there's no pointers between Documents
like this then I don't think we need to do anything at detach time.

[Sami, 2014-04-24 18:39:59]

On 2014/04/24 18:09:43, Rick Byers wrote:
> Yeah I think it's there only because the parent shouldn't be pointing at
> documents that aren't necessarily it's children (eg. the document could be
> re-attached elsewhere on the page).  If there's no pointers between Documents
> like this then I don't think we need to do anything at detach time.

Got it.

I've moved the registry to the page now and it became a lot simpler. PTAL.

[abarth-chromium, 2014-04-24 21:17:56]

LGTM assuming Rick is happy.  Thanks for being willing try a few approaches.

[Rick Byers, 2014-04-24 21:30:11]

Yeah I like this simplification - thanks for suggesting it Adam!

This LGTM to me.  I have a couple questions, but land away if you believe they
don't represent a problem.

https://codereview.chromium.org/237963014/diff/200001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/237963014/diff/200001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:1947: if (document().page())
In what cases does a Document not have a Page?  It seems like whatever those
cases are we could have a problem getting caught up...

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.cpp (left):

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:92: // Note that we can't assert that
|target| is in this document because
nit: remove/update comment (no "this document" anymore)

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:129: if (oldRegistry == this)
Do you know in what scenarios this won't be true?  It seems surprising to me
that there would be scenarios where event handlers can move across Pages.

[Sami, 2014-04-25 19:51:15]

On 2014/04/24 21:17:56, abarth wrote:
> LGTM assuming Rick is happy.  Thanks for being willing try a few approaches.

Thanks for being patient along the way :)

https://codereview.chromium.org/237963014/diff/200001/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/237963014/diff/200001/Source/core/dom/Node.cp...
Source/core/dom/Node.cpp:1947: if (document().page())
On 2014/04/24 21:30:12, Rick Byers wrote:
> In what cases does a Document not have a Page?  It seems like whatever those
> cases are we could have a problem getting caught up...

After a document is detached it will no longer have a page. However, it looks
like we remove all event handlers from a document before it is detached through
this call chain:

FrameLoader::detachFromParent
=> FrameLoader::closeURL
=> Document::dispatchUnloadEvents
=> Document::removeAllEventListenersRecursively

The only exception is for the transitional empty document
(https://bugs.webkit.org/show_bug.cgi?id=28716), but that only concerns window
handlers which we aren't tracking yet.

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.cpp (left):

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:92: // Note that we can't assert that
|target| is in this document because
On 2014/04/24 21:30:12, Rick Byers wrote:
> nit: remove/update comment (no "this document" anymore)

Good find, done.

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:129: if (oldRegistry == this)
On 2014/04/24 21:30:12, Rick Byers wrote:
> Do you know in what scenarios this won't be true?  It seems surprising to me
> that there would be scenarios where event handlers can move across Pages.

No, I don't have evidence whether this can actually happen. I'm mainly being
defensive here because I didn't see anything preventing a node from getting
detached from a document in one page and getting attached to a document in
another page. If that can't happen for a fact I'll just replace this with an
assert.

[Rick Byers, 2014-04-25 20:13:52]

On 2014/04/25 19:51:15, Sami wrote:
> On 2014/04/24 21:17:56, abarth wrote:
> > LGTM assuming Rick is happy.  Thanks for being willing try a few approaches.
> 
> Thanks for being patient along the way :)
> 
> https://codereview.chromium.org/237963014/diff/200001/Source/core/dom/Node.cpp
> File Source/core/dom/Node.cpp (right):
> 
>
https://codereview.chromium.org/237963014/diff/200001/Source/core/dom/Node.cp...
> Source/core/dom/Node.cpp:1947: if (document().page())
> On 2014/04/24 21:30:12, Rick Byers wrote:
> > In what cases does a Document not have a Page?  It seems like whatever those
> > cases are we could have a problem getting caught up...
> 
> After a document is detached it will no longer have a page. However, it looks
> like we remove all event handlers from a document before it is detached
through
> this call chain:
> 
> FrameLoader::detachFromParent
> => FrameLoader::closeURL
> => Document::dispatchUnloadEvents
> => Document::removeAllEventListenersRecursively
> 
> The only exception is for the transitional empty document
> (https://bugs.webkit.org/show_bug.cgi?id=28716), but that only concerns window
> handlers which we aren't tracking yet.

If we update event handlers at all when the Document doesn't have a page, and
the Document returns to having a page then that page's EventHandlerRegistry will
be incorrect.  Should we add ASSERTs here to at least catch cases in tests/debug
where handlers are being added/removed when there is no Page (and so no registry
to update)?

>
https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
> File Source/core/page/EventHandlerRegistry.cpp (left):
> 
>
https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
> Source/core/page/EventHandlerRegistry.cpp:92: // Note that we can't assert
that
> |target| is in this document because
> On 2014/04/24 21:30:12, Rick Byers wrote:
> > nit: remove/update comment (no "this document" anymore)
> 
> Good find, done.
> 
>
https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
> File Source/core/page/EventHandlerRegistry.cpp (right):
> 
>
https://codereview.chromium.org/237963014/diff/200001/Source/core/page/EventH...
> Source/core/page/EventHandlerRegistry.cpp:129: if (oldRegistry == this)
> On 2014/04/24 21:30:12, Rick Byers wrote:
> > Do you know in what scenarios this won't be true?  It seems surprising to me
> > that there would be scenarios where event handlers can move across Pages.
> 
> No, I don't have evidence whether this can actually happen. I'm mainly being
> defensive here because I didn't see anything preventing a node from getting
> detached from a document in one page and getting attached to a document in
> another page. If that can't happen for a fact I'll just replace this with an
> assert.

I don't know either (perhaps Adam does).  Pre-rendering is a weird case, but I
don't know exactly what the Page looks like.  You should probably manually
verify that a handler added during pre-rendering shows up in the registry after
the page has loaded (we've had bugs like that before - eg. see
https://code.google.com/p/chromium/issues/detail?id=332961).

Anyway I'm OK with this code as is, but can you at least add a comment saying
it's unclear whether this code is necessary?  That'll make it easier for someone
to remove if they find they don't need it.  Also there's obviously no test for
this code, ideally if we were going to keep it we'd test it.  You could add an
ASSERT and run the layout tests to see if any of them trigger it, but again I'm
OK landing it like this and iterating at some later date (eg. I'd rather get
best-effort sync-scroll in for M36 <grin>).  Maybe just add a comment that
references a new bug tracking this for now?

[Sami, 2014-04-25 21:08:15]

On 2014/04/25 20:13:52, Rick Byers wrote:
> If we update event handlers at all when the Document doesn't have a page, and
> the Document returns to having a page then that page's EventHandlerRegistry
will
> be incorrect.  Should we add ASSERTs here to at least catch cases in
tests/debug
> where handlers are being added/removed when there is no Page (and so no
registry
> to update)?

Good idea, I added a couple of asserts that I think make sense.
 
> I don't know either (perhaps Adam does).  Pre-rendering is a weird case, but I
> don't know exactly what the Page looks like.  You should probably manually
> verify that a handler added during pre-rendering shows up in the registry
after
> the page has loaded (we've had bugs like that before - eg. see
> https://code.google.com/p/chromium/issues/detail?id=332961).

Not sure about pre-rendering either, but I can do some testing. Based on quick
grepping though it doesn't seem like a document can be moved to a different
page; Document::page() goes through LocalFrame => Frame => FrameHost => Page,
and as far as I can tell the only change you can make to those relationships is
to detach them.
 
> Anyway I'm OK with this code as is, but can you at least add a comment saying
> it's unclear whether this code is necessary?  That'll make it easier for
someone
> to remove if they find they don't need it.  Also there's obviously no test for
> this code, ideally if we were going to keep it we'd test it.  You could add an
> ASSERT and run the layout tests to see if any of them trigger it, but again
I'm
> OK landing it like this and iterating at some later date (eg. I'd rather get
> best-effort sync-scroll in for M36 <grin>).  Maybe just add a comment that
> references a new bug tracking this for now?

I've added a comment and a bug. Agreed about needing a test, but I'm not sure if
it can be done in a layout test since they mainly run in the context of a single
Page. Maybe window.open is one way...

[Rick Byers, 2014-04-26 02:27:54]

Ok, LGTM - thanks!

[Sami, 2014-04-28 10:43:02]

I did some testing with link prefetching (http://jsbin.com/yecebogo/4/quiet) and
I didn't see any inconsistencies; the handlers are added during prefetching and
are properly present when the prefetched page becomes active. The older bug Rick
mentioned earlier was a problem on how prefetches were handled on the browser
side, which is outside the scope of EventHandlerRegistry.

Thanks for the reviews, let me try to land this now.

[Sami, 2014-04-28 10:43:09]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-04-28 10:43:40]

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

[commit-bot: I haz the power, 2014-04-28 12:01:03]

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

[commit-bot: I haz the power, 2014-04-28 12:01:04]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Sami, 2014-04-28 20:52:44]

Looks like Rick's suggested assertions found some cases where event handlers are
modified in a non-attached document. Mainly this seems to happen with innerHTML,
XMLHttpRequest responses, DOMParser and
document.implementation.createHTMLDocument().

I've fixed this by splitting didMoveFromOtherDocument() into didMoveIntoPage()
and didMoveOutOfPage() so that we can handle both when a node comes in from a
detached document or goes out to one. I also found a few cases where nodes were
moving in different documents under the same page and thus resolved the TODO we
added earlier.

[Rick Byers, 2014-04-28 21:18:07]

Interesting, I was worried about that.  Well I guess that's one argument for
tracking per-Document then.  I still think per-Page is better if we can get it
right without too much complexity/overhead.

Can you try to come up with a way to explicitly test the EventHandlerRegistry in
a scenario where a node is moving from one Page to another (or least modified
while detached then attached)? 

BTW, I reviewed just the diff to PS14 ("rebase") - thanks for uploading the
rebase separately to make that possible.

https://codereview.chromium.org/237963014/diff/300001/LayoutTests/fast/events...
File LayoutTests/fast/events/scroll-event-handler-count.html (right):

https://codereview.chromium.org/237963014/diff/300001/LayoutTests/fast/events...
LayoutTests/fast/events/scroll-event-handler-count.html:79: debug('Moving a
scroll event listener between documents should keep it active');
The test below doesn't seem to be testing what this comment says.  It just
removes the node, never inserts it into a new document.

https://codereview.chromium.org/237963014/diff/300001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:150: for (unsigned count =
node->getEventListeners(eventTypes[i]).size(); count > 0; --count)
Why call this in a loop for RemoveAll?  Ideally we'd be able to set the ref
count directly rather than looping for the Add case too, but that might not be
worth complexity.

https://codereview.chromium.org/237963014/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:154: for (Node* child =
node->firstChild(); child; child = child->nextSibling())
I don't think you want to do this recursively.  It looks to me like
Node::didMoveToNewDocument is called for each node that moved (see iteration in
TreeScopeAdopter::moveTreeToNewScope) so this will count each node other than
the root multiple times and be very expensive.  Ideally we'd have a test that
verifies this.

[Sami, 2014-04-29 14:21:28]

On 2014/04/28 21:18:07, Rick Byers wrote:
> Interesting, I was worried about that.  Well I guess that's one argument for
> tracking per-Document then.  I still think per-Page is better if we can get it
> right without too much complexity/overhead.

Right. I think we still get away with having this per-Page 

> Can you try to come up with a way to explicitly test the EventHandlerRegistry
in
> a scenario where a node is moving from one Page to another (or least modified
> while detached then attached)? 

Yes, I've got tests for three cases now: null page => non-null page, non-null
page => null-page, and between two documents on the same page.

I'l also do a local experiment where I make the registry track all types of
event handlers and see if there are inconsistencies.

https://codereview.chromium.org/237963014/diff/300001/LayoutTests/fast/events...
File LayoutTests/fast/events/scroll-event-handler-count.html (right):

https://codereview.chromium.org/237963014/diff/300001/LayoutTests/fast/events...
LayoutTests/fast/events/scroll-event-handler-count.html:79: debug('Moving a
scroll event listener between documents should keep it active');
On 2014/04/28 21:18:07, Rick Byers wrote:
> The test below doesn't seem to be testing what this comment says.  It just
> removes the node, never inserts it into a new document.

Sorry, this the description was a little unclear -- I've improved it. We
actually are moving the handler from one document to another. It is first
created inside |doc| which is a detached document (doesn't have a Page), and
then moved to the main document with the appendChild call.

As an aside, I think it's a little odd that removeChild doesn't decrease the
handler count, but this seems to be the expected behavior. It's also checked by
fast/events/touch/touch-event-handler-count.html. I'm not sure how to fix that
even if we wanted to since Node::detached doesn't seem to get called in that
case.

https://codereview.chromium.org/237963014/diff/300001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/237963014/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:150: for (unsigned count =
node->getEventListeners(eventTypes[i]).size(); count > 0; --count)
On 2014/04/28 21:18:07, Rick Byers wrote:
> Why call this in a loop for RemoveAll?  Ideally we'd be able to set the ref
> count directly rather than looping for the Add case too, but that might not be
> worth complexity.

Right, I wasn't sure if a fast path RemoveAll was worth the hassle, but I've now
added it and it isn't too bad.

Looks like HashCountedSet doesn't support bulk additions, so doing the same for
additions would just be moving the loop further down. I'm inclined to keep the
loop here since this is the only case we need it for.

https://codereview.chromium.org/237963014/diff/300001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.cpp:154: for (Node* child =
node->firstChild(); child; child = child->nextSibling())
On 2014/04/28 21:18:07, Rick Byers wrote:
> I don't think you want to do this recursively.  It looks to me like
> Node::didMoveToNewDocument is called for each node that moved (see iteration
in
> TreeScopeAdopter::moveTreeToNewScope) so this will count each node other than
> the root multiple times and be very expensive.  Ideally we'd have a test that
> verifies this.

Oh, thanks for pointing this out! Definitely don't want to be doing recursive
recursion :P I checked that the recursion was needed while writing the patch,
but looks like I got it somehow wrong. I've now fixed this and added a test.

[Sami, 2014-04-29 15:40:46]

On further meditation I think we may have to go back to the per-Document
registry. I found 419 layout tests where a node gets detached from a page before
unregistering its handlers, leaving a stale pointer in the registry. I still
need to check what the root causes are, but in any case this feels too brittle.

Maybe a compromise would be to require the client to walk the frame hierarchy to
find all handlers. I'll sketch out some options.

[Rick Byers, 2014-04-29 15:41:11]

Thanks Sami, this is looking great.  I just want to make sure we've got the
detach/reattach case covered completely then I think we're good to go.

https://codereview.chromium.org/237963014/diff/340001/LayoutTests/fast/events...
File LayoutTests/fast/events/scroll-event-handler-count.html (right):

https://codereview.chromium.org/237963014/diff/340001/LayoutTests/fast/events...
LayoutTests/fast/events/scroll-event-handler-count.html:97: 
I agree this is a little surprising.  What if the div is moved back into this
document at this point, do we double-count the handlers then?  That would be
really bad I think.  What if we move the div into a different document, does
this document get updated at that point?  If both of those cases are handled
alright (without double-counting) then let's just add tests for that.

[Sami, 2014-04-29 15:43:25]

Heh, jinx. Let me see why the handlers aren't getting unregistered and report
back.

[Rick Byers, 2014-04-29 17:57:15]

On 2014/04/29 15:43:25, Sami wrote:
> Heh, jinx. Let me see why the handlers aren't getting unregistered and report
> back.

Interesting, thanks.  Keep in mind we need to avoid changing semantics pages are
likely to rely on (but we should feel free to fix bugs we think won't break web
compat).  Eg. if pages can add a handler to a node, then remove a subtree of the
DOM containing that node and later re-add it at another place in the document
while preserving the event handler registration, then we can't break that.

This change is really turning into an interesting exploration of event handler
behavior, thanks for being so diligent - I'm at least glad to be learning
<grin>.

[Sami, 2014-04-29 18:37:42]

I think I've now figured out how we end up with stale event handler targets in
the registry:

1. Some event handler is added to an element.
2. The element is removed from the document, but a JS-reference is kept to it.
3. By the time the reference goes away (e.g., when the test completes) and gc
runs, the element no longer knows about the page it was on, so it cannot
unregister.

We could fix this by un/registering handlers as elements are de/attached, but
both of those methods are inline which suggests we shouldn't do anything
expensive there. The only other option I can think of is moving the registry
back to document scope. 

On 2014/04/29 17:57:15, Rick Byers wrote:
> Interesting, thanks.  Keep in mind we need to avoid changing semantics pages
are
> likely to rely on (but we should feel free to fix bugs we think won't break
web
> compat).  Eg. if pages can add a handler to a node, then remove a subtree of
the
> DOM containing that node and later re-add it at another place in the document
> while preserving the event handler registration, then we can't break that.

Yes, that type of behavior is totally fine and will remain working. The
particular weirdness I was referring to was when you add a handler to a node and
the remove that node from the document; at that point the node will still be in
the touch event target set. I guess that's why ScrollingCoordinator guards all
targets it considers with inDocument().

> This change is really turning into an interesting exploration of event handler
> behavior, thanks for being so diligent - I'm at least glad to be learning
> <grin>.

"Refactor some blink code", they said. "It'll be fun", they said :) Seriously
though, I'm sure the future me will appreciate all these new unit tests and
asserts documenting what should happen.

[Rick Byers, 2014-04-29 19:08:21]

On 2014/04/29 18:37:42, Sami wrote:
> I think I've now figured out how we end up with stale event handler targets in
> the registry:
> 
> 1. Some event handler is added to an element.
> 2. The element is removed from the document, but a JS-reference is kept to it.
> 3. By the time the reference goes away (e.g., when the test completes) and gc
> runs, the element no longer knows about the page it was on, so it cannot
> unregister.

Right.  And this case works fine with a Document-coupled registry because every
node is guaranteed to always have an ownerDocument (even when it's not currently
in the DOM tree of that document), and we know reliably when it's ownerDocument
changes.  But Document's aren't guaranteed to always have a Page (in particular,
they loose their Frame when they become detached, and also Frame's can loose
their FrameHost/Page when the frame is detached).

> We could fix this by un/registering handlers as elements are de/attached, but
> both of those methods are inline which suggests we shouldn't do anything
> expensive there. The only other option I can think of is moving the registry
> back to document scope. 

If we can find all cases where a Document can loose it's connection to a Page
(and if it never gets a connection to a new Page) then we can unregister all
handlers in a Document when that occurs.  The two places I see now are
Page::documentDetached and Frame::willDetachFromFrameHost.  Perhaps if we remove
them all there that's good enough?  I agree we don't want to search for handlers
to remove on Node attach/detach time but that shouldn't matter (like we agreed
below, a detached node should retain it's handlers).

I'm personally OK with going back to per-Document tracking, but it still looks
possible that Page-level tracking may not be too hard to get right.  WDYT?

By the way, looking at this more now it seems the EventHandlerRegistry should
really be on FrameHost not Page (due to site isolation), although for now it'll
be wired up in essentially the same way.
 
> On 2014/04/29 17:57:15, Rick Byers wrote:
> > Interesting, thanks.  Keep in mind we need to avoid changing semantics pages
> are
> > likely to rely on (but we should feel free to fix bugs we think won't break
> web
> > compat).  Eg. if pages can add a handler to a node, then remove a subtree of
> the
> > DOM containing that node and later re-add it at another place in the
document
> > while preserving the event handler registration, then we can't break that.
> 
> Yes, that type of behavior is totally fine and will remain working. The
> particular weirdness I was referring to was when you add a handler to a node
and
> the remove that node from the document; at that point the node will still be
in
> the touch event target set. I guess that's why ScrollingCoordinator guards all
> targets it considers with inDocument().
> 
> > This change is really turning into an interesting exploration of event
handler
> > behavior, thanks for being so diligent - I'm at least glad to be learning
> > <grin>.
> 
> "Refactor some blink code", they said. "It'll be fun", they said :) Seriously
> though, I'm sure the future me will appreciate all these new unit tests and
> asserts documenting what should happen.

[Sami, 2014-04-29 21:58:09]

On 2014/04/29 19:08:21, Rick Byers wrote:
> > We could fix this by un/registering handlers as elements are de/attached,
but
> > both of those methods are inline which suggests we shouldn't do anything
> > expensive there. The only other option I can think of is moving the registry
> > back to document scope. 
> 
> If we can find all cases where a Document can loose it's connection to a Page
> (and if it never gets a connection to a new Page) then we can unregister all
> handlers in a Document when that occurs.  The two places I see now are
> Page::documentDetached and Frame::willDetachFromFrameHost.  Perhaps if we
remove
> them all there that's good enough?  I agree we don't want to search for
handlers
> to remove on Node attach/detach time but that shouldn't matter (like we agreed
> below, a detached node should retain it's handlers).
>
> I'm personally OK with going back to per-Document tracking, but it still looks
> possible that Page-level tracking may not be too hard to get right.  WDYT?

Hmm, I think that might be sufficient after all. I was initially worried that
even if we catch all the current ways a document can be detached, someone can
always add new ones. However, I noticed we can increase test coverage by making
the registry track more event classes in debug builds. Whenever the registry is
modified, we can verify that all registered handlers still have a pointer to the
page.

This version handles document detachment by checking whether any of the
registered handlers are inside that document. Note that we can't do the reverse
of walking through the document's subtree because it will not contain nodes that
have been detached but still have handlers. (This is also why
Document::removeAllEventHandlersRecursively misses them.)

Also, for this version I only ended up adding a handful of event targets for the
debug build, because otherwise
fast/events/gc-freeze-with-attribute-listeners.html times out due to the
additional checking (it registers 10000 click handlers).

> By the way, looking at this more now it seems the EventHandlerRegistry should
> really be on FrameHost not Page (due to site isolation), although for now
it'll
> be wired up in essentially the same way.

Interesting. Do you think we should make the move now? FrameHost looks a little
bare so I'm not sure if now is the right time to start migrating stuff to it.

[Rick Byers, 2014-04-30 17:28:49]

Great, this seems like it should (hopefully) be adequate.  I love the extra
validation and test coverage.  All this lifetime stuff is the kind of extra
complexity I was saying I thought we might have to deal with by moving away from
Document-coupled registries, but I still _think_ it's simpler this way (but if
it gets much more complex I'd probably change my mind).

> > By the way, looking at this more now it seems the EventHandlerRegistry
should
> > really be on FrameHost not Page (due to site isolation), although for now
> it'll
> > be wired up in essentially the same way.
> 
> Interesting. Do you think we should make the move now? FrameHost looks a
little
> bare so I'm not sure if now is the right time to start migrating stuff to it.

Looks like the site isolation guys have put stuff in there asking that we don't
extend Page (they're actively moving stuff out), so I think we should put it on
FrameHost soonish (although the implementation will probably still rely on
Page).  But I'm OK doing that in a follow-up CL - this one has been hard enough
<grin>.

https://codereview.chromium.org/237963014/diff/360001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.h (right):

https://codereview.chromium.org/237963014/diff/360001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.h:50: void documentDetached(Document&);
Please add a comment saying that we require didMoveOutOfPage or documentDetached
to be called whenever the Page associated with a Node could change (or become
unset).  This is the main thing I think your extra consistency checks help
verify - if they fail at some point in the future, the code should be clear on
what it requires.

https://codereview.chromium.org/237963014/diff/400001/LayoutTests/fast/events...
File LayoutTests/fast/events/scroll-event-handler-count.html (right):

https://codereview.chromium.org/237963014/diff/400001/LayoutTests/fast/events...
LayoutTests/fast/events/scroll-event-handler-count.html:87:
div.appendChild(childDiv);
perhaps it's worth verifying here that the event handler count for 'doc' is 0
and add a comment saying why this is OK.  I guess even an unattached document
may have scroll events - we should probably have a comment in
EventHandlerRegistry.h saying it's purpose is only to tell you about event
handlers in documents that can be rendered  (or can receive input).

Also probably worth verifying that the handler count on document is 0 at this
point (i.e. we're not incorrectly assigning the handlers of unattached documents
to the current page or something).

[Rick Byers, 2014-04-30 17:30:17]

On 2014/04/30 17:28:49, Rick Byers wrote:
> Great, this seems like it should (hopefully) be adequate.  I love the extra
> validation and test coverage.  All this lifetime stuff is the kind of extra
> complexity I was saying I thought we might have to deal with by moving away
from
> Document-coupled registries, but I still _think_ it's simpler this way (but if
> it gets much more complex I'd probably change my mind).
> 
> > > By the way, looking at this more now it seems the EventHandlerRegistry
> should
> > > really be on FrameHost not Page (due to site isolation), although for now
> > it'll
> > > be wired up in essentially the same way.
> > 
> > Interesting. Do you think we should make the move now? FrameHost looks a
> little
> > bare so I'm not sure if now is the right time to start migrating stuff to
it.
> 
> Looks like the site isolation guys have put stuff in there asking that we
don't
> extend Page (they're actively moving stuff out), so I think we should put it
on
> FrameHost soonish (although the implementation will probably still rely on
> Page).  But I'm OK doing that in a follow-up CL - this one has been hard
enough
> <grin>.
> 
>
https://codereview.chromium.org/237963014/diff/360001/Source/core/page/EventH...
> File Source/core/page/EventHandlerRegistry.h (right):
> 
>
https://codereview.chromium.org/237963014/diff/360001/Source/core/page/EventH...
> Source/core/page/EventHandlerRegistry.h:50: void documentDetached(Document&);
> Please add a comment saying that we require didMoveOutOfPage or
documentDetached
> to be called whenever the Page associated with a Node could change (or become
> unset).  This is the main thing I think your extra consistency checks help
> verify - if they fail at some point in the future, the code should be clear on
> what it requires.
> 
>
https://codereview.chromium.org/237963014/diff/400001/LayoutTests/fast/events...
> File LayoutTests/fast/events/scroll-event-handler-count.html (right):
> 
>
https://codereview.chromium.org/237963014/diff/400001/LayoutTests/fast/events...
> LayoutTests/fast/events/scroll-event-handler-count.html:87:
> div.appendChild(childDiv);
> perhaps it's worth verifying here that the event handler count for 'doc' is 0
> and add a comment saying why this is OK.  I guess even an unattached document
> may have scroll events - we should probably have a comment in
> EventHandlerRegistry.h saying it's purpose is only to tell you about event
> handlers in documents that can be rendered  (or can receive input).
> 
> Also probably worth verifying that the handler count on document is 0 at this
> point (i.e. we're not incorrectly assigning the handlers of unattached
documents
> to the current page or something).

Oh and this LGTM modulo the minor comment/test tweaks.  Probably time for Adam
to take another look.

[abarth-chromium, 2014-04-30 19:17:43]

I haven't followed the latest discussion in detail, but the CL LGTM

[Sami, 2014-05-01 13:29:33]

Thanks both.

> Looks like the site isolation guys have put stuff in there asking that we
don't
> extend Page (they're actively moving stuff out), so I think we should put it
on
> FrameHost soonish (although the implementation will probably still rely on
> Page).  But I'm OK doing that in a follow-up CL - this one has been hard
enough
> <grin>.

Ok, I've opened https://code.google.com/p/chromium/issues/detail?id=369082 added
a TODO here. I'll upload another patch for the move once I figure out what it
takes.

https://codereview.chromium.org/237963014/diff/360001/Source/core/page/EventH...
File Source/core/page/EventHandlerRegistry.h (right):

https://codereview.chromium.org/237963014/diff/360001/Source/core/page/EventH...
Source/core/page/EventHandlerRegistry.h:50: void documentDetached(Document&);
On 2014/04/30 17:28:50, Rick Byers wrote:
> Please add a comment saying that we require didMoveOutOfPage or
documentDetached
> to be called whenever the Page associated with a Node could change (or become
> unset).  This is the main thing I think your extra consistency checks help
> verify - if they fail at some point in the future, the code should be clear on
> what it requires.

That's right, better be explicit about it. I've added a comment and a hint next
to the assertions.

https://codereview.chromium.org/237963014/diff/400001/LayoutTests/fast/events...
File LayoutTests/fast/events/scroll-event-handler-count.html (right):

https://codereview.chromium.org/237963014/diff/400001/LayoutTests/fast/events...
LayoutTests/fast/events/scroll-event-handler-count.html:87:
div.appendChild(childDiv);
On 2014/04/30 17:28:50, Rick Byers wrote:
> perhaps it's worth verifying here that the event handler count for 'doc' is 0
> and add a comment saying why this is OK.  I guess even an unattached document
> may have scroll events - we should probably have a comment in
> EventHandlerRegistry.h saying it's purpose is only to tell you about event
> handlers in documents that can be rendered  (or can receive input).
> 
> Also probably worth verifying that the handler count on document is 0 at this
> point (i.e. we're not incorrectly assigning the handlers of unattached
documents
> to the current page or something).

Yes, good ideas all around. |doc| will have zero handlers at this point by
virtue of not having a Page where to ask for handlers, but it's good to verify
that too. I've also updated the documentation in EventHandlerRegistry.h.

[Rick Byers, 2014-05-01 13:42:35]

Great, thanks!  Lets land this monster ;-)

https://codereview.chromium.org/237963014/diff/430001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/237963014/diff/430001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:1251: if (!document.page())
Good.  This is also important to prevent noise from ClusterFuzz (which can
randomly use your internal APIs trying to provoke a crash).

[Sami, 2014-05-01 14:01:19]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-01 14:01:26]

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

[commit-bot: I haz the power, 2014-05-01 14:50:15]

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

[commit-bot: I haz the power, 2014-05-01 14:50:16]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink

[Sami, 2014-05-01 17:42:11]

Hmm,
virtual/serviceworker/http/tests/serviceworker/serviceworkerglobalscope-scope.html
seems to be flaky. Saw the same crash without my patch too.

[Sami, 2014-05-01 17:42:22]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-01 17:42:41]

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

[Sami, 2014-05-01 17:44:16]

On 2014/05/01 17:42:11, Sami wrote:
> Hmm,
>
virtual/serviceworker/http/tests/serviceworker/serviceworkerglobalscope-scope.html
> seems to be flaky. Saw the same crash without my patch too.

crbug.com/366987

[commit-bot: I haz the power, 2014-05-01 18:44:27]

Change committed as 173101