Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
206603002:
Add EventHandlerRegistry (Closed)
|
Created:
6 years, 9 months ago by Sami Modified:
6 years, 8 months ago CC:
blink-reviews, kenneth.christiansen, jamesr, sof, eae+blinkwatch, blink-layers+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, Inactive, rwlbuis, Zeeshan Qureshi Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd EventHandlerRegistry
This patch introduces a generic EventHandlerRegistry document supplement
for keeping track for event handlers on EventTargets.
The EventHandlerRegistry provides a hash map of EventTargets for scroll
events; touch and wheel events will be added later. Currently the target
set contains all event handler targets in the host document. Future
patches will add handlers from child documents.
Initially the registry only tracks scroll handlers, but upcoming patches
will migrate the existing code paths for tracking touch and wheel events
to use this registry instead.
TEST=fast/events/scroll-event-handler-count.html
BUG=347366
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171885
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Tests. #
Total comments: 2
Patch Set 4 : Refactoring WIP. #Patch Set 5 : Refactoring WIP. #
Total comments: 2
Patch Set 6 : WIP snapshot. #Patch Set 7 : Logic is mostly there but still fails some tests. #Patch Set 8 : Tests now seem to pass. #
Total comments: 34
Patch Set 9 : Review comments. #Patch Set 10 : Add test for external handlers. #
Total comments: 2
Patch Set 11 : Rebased. #
Total comments: 11
Patch Set 12 : More comments. #
Total comments: 6
Patch Set 13 : Fix merge snafu. #Patch Set 14 : Rebased. #Patch Set 15 : Rebase harder. #
Total comments: 4
Patch Set 16 : Added comments. Replaced nullptr with 0 to fix build. #
Total comments: 12
Patch Set 17 : Review comments (except breaking up the patch). #Patch Set 18 : Just the registry. #
Total comments: 18
Patch Set 19 : Rebased. #Patch Set 20 : Adam's comments -- further splitting still TODO. #Patch Set 21 : Start with the bare minimum. #
Total comments: 13
Patch Set 22 : Adam's comments. #Patch Set 23 : One more unneeded include. #Messages
Total messages: 30 (0 generated)
Ian, wanna look this over? This is mostly identical to wheel event handler tracking except there's no need to tell RWH about scroll handlers. James, I left public/platform/WebLayer.h for you.
https://codereview.chromium.org/206603002/diff/40001/Source/core/dom/ScrollEv... File Source/core/dom/ScrollEventHandlerController.cpp (right): https://codereview.chromium.org/206603002/diff/40001/Source/core/dom/ScrollEv... Source/core/dom/ScrollEventHandlerController.cpp:77: } So much copy/pasted code. Can you extract a base class to share some of this code? https://codereview.chromium.org/206603002/diff/40001/Source/core/dom/ScrollEv... File Source/core/dom/ScrollEventHandlerController.h (right): https://codereview.chromium.org/206603002/diff/40001/Source/core/dom/ScrollEv... Source/core/dom/ScrollEventHandlerController.h:24: */ Please use a new-style copyright block: http://dev.chromium.org/blink/coding-style#TOC-License
Thanks for working on this Sami! I like the direction of the unification you're doing. Looks like you're still in the middle of refactoring so I won't do a detailed review yet, but here's a few things you may want to consider. https://codereview.chromium.org/206603002/diff/80001/Source/core/dom/EventHan... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/80001/Source/core/dom/EventHan... Source/core/dom/EventHandlerRegistry.h:18: class EventHandlerRegistry FINAL : public DocumentSupplement, public DOMWindowLifecycleObserver { I really like the fact that you're unifying this event handler tracking across different types of events! I think ideally there'd be no reason for this class to know details of specific event types at all (it could do completely generic event handler tracking). That's probably too much work right now (there are going to be some special cases and it's probably not worth the indirection/abstraction to remove all detail of them from this class). But it does seem like a good idea to work towards that wherever it can be done cleanly. For example, define a 'EventHandlerClass' enum (currently WheelEvent, ScrollEvent or TouchEvent). Then rather than have any 'fooEventHandlerCount' functions, you can have just 'eventHandlerCount' that takes the class as a parameter. At the moment touch event handlers are special in that we need to be able to enumerate all the nodes, but that probably doesn't need to be baked in (I could see wanting that for wheel handlers too, etc.). Perhaps each event class should just have a constant bit saying whether we want to take the time to track all the handler nodes (or just keep a dumb count). At some point I could see enabling this in debug for all handler classes since the touch-handler related ASSERTS have been super-valuable in finding edge cases where the handlers weren't being tracked properly. https://codereview.chromium.org/206603002/diff/80001/Source/core/page/scrolli... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/206603002/diff/80001/Source/core/page/scrolli... Source/core/page/scrolling/ScrollingCoordinator.cpp:637: void ScrollingCoordinator::recomputeScrollEventHandlerCountForFrameView(FrameView*) What you're doing here seems conceptually similar to the code in Document::didAdd/Remove/ClearTouchEventHandler that calls frameHost->chrome().client().needTouchEvents(true/false) whenever we transition between no handlers and some handlers. Can we generalize that somehow? One option is having the EventHandlerRegistry chain between documents the way the touch handler stuff does today (so any number of handlers in a child document count as one handler in the parent) and then expose an API for when it changes. Alternately, if it's simpler I'm not opposed to walking the frame tree to compute this when needed (should always be cheap), but then we should probably do the same for the touch handlers. One thing you may want to think about is the impact on out-of-process iframes / site-isolation. I haven't figured out yet how the touch handler tracking needs to change for site-isolation, but if you're refactoring all this then it probably makes sense to have some idea for how it'll eventually handle site isolation. I'm guessing having an outside process compute the document-wide view will fit much better with the site-isolation model.
Thanks for the great feedback Rick. I'm glad that you think the unification makes sense. I was a little hesitant first because of all the touch edge cases, but after taking a closer look it seems doable. I'll do another pass to make this more generic with the suggestions you had. I'm a little unsure how OoP iframes fits into all this, but like you said it's probably easier to retrofit that in once we have all the logic in a single place.
On 2014/03/24 15:03:53, Sami wrote: > Thanks for the great feedback Rick. I'm glad that you think the unification > makes sense. I was a little hesitant first because of all the touch edge cases, > but after taking a closer look it seems doable. I'll do another pass to make > this more generic with the suggestions you had. Yeah. When I first started looking at the touch handler code there were a number of special cases and confusing logic, but I've been able to make it increasingly generic (each time the ASSERTS leading me to finding additional special cases that weren't handled properly). I expect it shouldn't be too hard to treat the existence of scroll, wheel and touch handlers as all facets of the same general mechanism, but I expect you'll find some additional places where we weren't handling wheel event handlers properly (especially if you try to enforce that we only remove nodes we previously added, as for touch handlers). > I'm a little unsure how OoP iframes fits into all this, but like you said it's > probably easier to retrofit that in once we have all the logic in a single > place.
Rick, do you want to do a first pass over this since it looks like you were here before? Note that I had to start maintaining the target set for all event types since implementing the remove-all-handlers-for-a-target case was tricky otherwise. Thanks for your comments too, Adam; all addressed.
On 2014/03/26 20:08:39, Sami wrote: > Rick, do you want to do a first pass over this since it looks like you were here > before? Note that I had to start maintaining the target set for all event types > since implementing the remove-all-handlers-for-a-target case was tricky > otherwise. Sure, looking now. > Thanks for your comments too, Adam; all addressed.
This looks awesome Sami, thanks! My comments are now relatively minor. > Currently we are already tracking wheel and touch event handlers, but > the code paths for doing so are disjoint and subtly broken. If there were things that were broken then you should try to add some new test cases to cover those scenarios (I've been steadily advancing the touch handler test cases as I hit issues and the tests have been invaluable to me in previous refactorings, but it was still quite subtle/brittle). https://codereview.chromium.org/206603002/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/scroll-event-handler-count.html (right): https://codereview.chromium.org/206603002/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/scroll-event-handler-count.html:3: description("This test checks that we correctly update the scroll event handler count as event handlers are added and removed"); Note that I've added a bunch more testing to touch-event-handler-count.html from what was in the (apparently copied/pasted) wheel-event version. Now that the code is all unified, it seems silly to have 3 tests which are mostly (but not completely copy/paste of each other). Perhaps it's worth unifying into a single test that uses a mix of event types (verifying each type and each scenario but not every possible combination of type+scenario)? https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:94: // The node should either be in the document, or be the Document node of a child I like this assert, but again, but what if 'document' isn't the Document this EventHandlerRegistry is associated with (i.e. DocumentLifecycleObserver::lifecycleContext())? That could cause all sorts of confusion, right? https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:173: if (addedFirstHandler || removedLastHandler) { simpler just to check if(hadHandlers != haveHandlers) https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:176: EventHandlerRegistry::from(*parent)->updateEventHandlerInternal(op, *parent, handlerClass, &document); It's calls like this that make me think the Document parameter should be redundant - using "*parent" twice here. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:193: { this function should collapse to nothing if you can make the Document implicit based on this instead of explicit. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:287: void EventHandlerRegistry::updateAllEventHandlersForTarget(ChangeOperation op, EventTarget& target, EventHandlerRegistry& targetRegistry) const I find this function confusing. Perhaps there's really two different things being munged into one here. One function that supports RemoveAll (which as I point out below seems to be handled improperly here) and operates on only a single registry, and then a separate scenario (which could most naturally just be part of didMoveToNewDocument) for transferring out of one registry into another. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:294: for (unsigned count = targets->count(&target); count > 0; --count) This loop doesn't seem to make sense when the op is RemoveAll (as for the didRemoveAllEventListeners case). For the Remove case we could avoid the loop by changing the Remove to a RemoveAll, right? For Add I doubt it's worth complicating things with an 'AddMultiple' (and so perhaps both Add and Remove should just continue to use a loop - it should generally be small). https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:313: scrollingCoordinator->frameViewWheelEventHandlerCountChanged(frameView); This function and the below say they're supposed to be called whenever the count changes, but you're calling it only when the has-handlers bit changes. I suggest you rename these functions (basically to be the equivalent of needTouchEvents(bool), maybe hasWheelHandlers/hasScrollHandlers). https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:39: // the host Document as well as all child Documents. For efficiency each I find this wording a little contradictory. How about the following: "Returns the number of registered event handlers for the given class on the host Document. Child documents count as exacty 1 if they have any handlers of the class." https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:51: void didAddEventHandler(EventTarget&, const AtomicString&); document (perhaps just with a good variable name) what the string means (I'm guessing the event type?). https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:57: void didAddExternalEventHandler(Document&, const AtomicString&); each EventHandlerRegistry is coupled to a specific document, why do callers also have to supply a Document? What happens if the Document they supply (or the Document of an EventTarget supplied) isn't the one this EventHandlerRegistry is coupled to? Can we just enforce that it always matches, or possibly not require it at all (if it's unlikely that call sites would mix it up)? https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:60: // Inherited from DOMWindowLifecycleObserver It may be confusing to clients of this class whether they should be calling these methods, or the ones above. Can you keep an instance of a DOMWindowLifecycleObserver as a member instead of inheriting from it? Probably the same for DocumentLifecycleObserver. See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:94: void notifyDidAddFirstOrRemoveLastEventHandler(Document&, EventHandlerClass, bool hasActiveHandlers); this method name is a mouthfull. How about notifyHasHandlersChanged? https://codereview.chromium.org/206603002/diff/140001/Source/core/html/HTMLIn... File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/html/HTMLIn... Source/core/html/HTMLInputElement.cpp:172: EventHandlerRegistry::from(document())->didRemoveEventHandler(*this, EventTypeNames::touchstart); It's a bit of a lie here to pass just touchstart (eg. what if we make a future optmization to handle touchstart and touchmove separately?). It's not a big deal (I can live with this), but I wonder if it's worth considering exposing a version that takes just the event class... https://codereview.chromium.org/206603002/diff/140001/Source/core/page/scroll... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:651: unsigned count = EventHandlerRegistry::from(*frame->document())->eventHandlerCount(EventHandlerRegistry::ScrollEvent); why all this work with 'counts' when this function ever seems to do with them is check >0? It would be simpler and faster to just have a 'bool hasWheelHandlers' etc... https://codereview.chromium.org/206603002/diff/140001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:763: Node* node = target->toDOMWindow() ? target->toDOMWindow()->document() : target->toNode(); Would be simpler to just combine this with the "if(node->isDocumentNode())" check below (I.e. only get a Node object at all in the else case). https://codereview.chromium.org/206603002/diff/140001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (left): https://codereview.chromium.org/206603002/diff/140001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:1545: // calls to Document::did{Add|Remove|Clear}TouchEventHandler. Verifying that those calls are made update comment please
Thanks for the excellent in-depth review Rick. All points addressed. > If there were things that were broken then you should try to add some new test > cases to cover those scenarios (I've been steadily advancing the touch handler > test cases as I hit issues and the tests have been invaluable to me in previous > refactorings, but it was still quite subtle/brittle). I was mainly referring to the fact that the old touch handler tracking code covered much more cases than the wheel event one and was better tested too. Now that the implementation and tests are shared the coverage is much better. https://codereview.chromium.org/206603002/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/scroll-event-handler-count.html (right): https://codereview.chromium.org/206603002/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/scroll-event-handler-count.html:3: description("This test checks that we correctly update the scroll event handler count as event handlers are added and removed"); On 2014/03/27 16:43:31, Rick Byers wrote: > Note that I've added a bunch more testing to touch-event-handler-count.html from > what was in the (apparently copied/pasted) wheel-event version. Now that the > code is all unified, it seems silly to have 3 tests which are mostly (but not > completely copy/paste of each other). Perhaps it's worth unifying into a single > test that uses a mix of event types (verifying each type and each scenario but > not every possible combination of type+scenario)? Yes, definitely. No need to duplicate the same test three times over. I've now got a single event-handler-count.html that covers all the scenarios with variable event types. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:94: // The node should either be in the document, or be the Document node of a child On 2014/03/27 16:43:31, Rick Byers wrote: > I like this assert, but again, but what if 'document' isn't the Document this > EventHandlerRegistry is associated with (i.e. > DocumentLifecycleObserver::lifecycleContext())? That could cause all sorts of > confusion, right? Agreed. This is now cleared up too. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:173: if (addedFirstHandler || removedLastHandler) { On 2014/03/27 16:43:31, Rick Byers wrote: > simpler just to check if(hadHandlers != haveHandlers) D'oh, done. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:176: EventHandlerRegistry::from(*parent)->updateEventHandlerInternal(op, *parent, handlerClass, &document); On 2014/03/27 16:43:31, Rick Byers wrote: > It's calls like this that make me think the Document parameter should be > redundant - using "*parent" twice here. Done. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:193: { On 2014/03/27 16:43:31, Rick Byers wrote: > this function should collapse to nothing if you can make the Document implicit > based on this instead of explicit. Done. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:287: void EventHandlerRegistry::updateAllEventHandlersForTarget(ChangeOperation op, EventTarget& target, EventHandlerRegistry& targetRegistry) const On 2014/03/27 16:43:31, Rick Byers wrote: > I find this function confusing. Perhaps there's really two different things > being munged into one here. One function that supports RemoveAll (which as I > point out below seems to be handled improperly here) and operates on only a > single registry, and then a separate scenario (which could most naturally just > be part of didMoveToNewDocument) for transferring out of one registry into > another. Right, I tried a little too hard to reuse code and the result ended up being incorrect. I've now got two dedicated loops for the two scenarios. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:294: for (unsigned count = targets->count(&target); count > 0; --count) On 2014/03/27 16:43:31, Rick Byers wrote: > This loop doesn't seem to make sense when the op is RemoveAll (as for the > didRemoveAllEventListeners case). For the Remove case we could avoid the loop > by changing the Remove to a RemoveAll, right? For Add I doubt it's worth > complicating things with an 'AddMultiple' (and so perhaps both Add and Remove > should just continue to use a loop - it should generally be small). Done. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:313: scrollingCoordinator->frameViewWheelEventHandlerCountChanged(frameView); On 2014/03/27 16:43:31, Rick Byers wrote: > This function and the below say they're supposed to be called whenever the count > changes, but you're calling it only when the has-handlers bit changes. I > suggest you rename these functions (basically to be the equivalent of > needTouchEvents(bool), maybe hasWheelHandlers/hasScrollHandlers). Yes, especially now that ScrollingCoordinator no longer needs exact handler counts these names can be changed. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:39: // the host Document as well as all child Documents. For efficiency each On 2014/03/27 16:43:31, Rick Byers wrote: > I find this wording a little contradictory. How about the following: > "Returns the number of registered event handlers for the given class on the host > Document. Child documents count as exacty 1 if they have any handlers of the > class." Thanks for putting it into simpler words :) Note that since we're not tracking counts anymore this function is gone. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:51: void didAddEventHandler(EventTarget&, const AtomicString&); On 2014/03/27 16:43:31, Rick Byers wrote: > document (perhaps just with a good variable name) what the string means (I'm > guessing the event type?). Done. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:57: void didAddExternalEventHandler(Document&, const AtomicString&); On 2014/03/27 16:43:31, Rick Byers wrote: > each EventHandlerRegistry is coupled to a specific document, why do callers also > have to supply a Document? What happens if the Document they supply (or the > Document of an EventTarget supplied) isn't the one this EventHandlerRegistry is > coupled to? > > Can we just enforce that it always matches, or possibly not require it at all > (if it's unlikely that call sites would mix it up)? Excellent point. Even though the call sites might not get it wrong, it's having two potential documents is confusing and unnecessary. I've reworked this to not require clients to pass in documents anymore. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:60: // Inherited from DOMWindowLifecycleObserver On 2014/03/27 16:43:31, Rick Byers wrote: > It may be confusing to clients of this class whether they should be calling > these methods, or the ones above. Can you keep an instance of a > DOMWindowLifecycleObserver as a member instead of inheriting from it? Probably > the same for DocumentLifecycleObserver. See > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance Great suggestion, done. https://codereview.chromium.org/206603002/diff/140001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:94: void notifyDidAddFirstOrRemoveLastEventHandler(Document&, EventHandlerClass, bool hasActiveHandlers); On 2014/03/27 16:43:31, Rick Byers wrote: > this method name is a mouthfull. How about notifyHasHandlersChanged? Couldn't have put it better myself :) https://codereview.chromium.org/206603002/diff/140001/Source/core/html/HTMLIn... File Source/core/html/HTMLInputElement.cpp (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/html/HTMLIn... Source/core/html/HTMLInputElement.cpp:172: EventHandlerRegistry::from(document())->didRemoveEventHandler(*this, EventTypeNames::touchstart); On 2014/03/27 16:43:31, Rick Byers wrote: > It's a bit of a lie here to pass just touchstart (eg. what if we make a future > optmization to handle touchstart and touchmove separately?). It's not a big > deal (I can live with this), but I wonder if it's worth considering exposing a > version that takes just the event class... Done. https://codereview.chromium.org/206603002/diff/140001/Source/core/page/scroll... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/206603002/diff/140001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:651: unsigned count = EventHandlerRegistry::from(*frame->document())->eventHandlerCount(EventHandlerRegistry::ScrollEvent); On 2014/03/27 16:43:31, Rick Byers wrote: > why all this work with 'counts' when this function ever seems to do with them is > check >0? It would be simpler and faster to just have a 'bool hasWheelHandlers' > etc... Yeah, the more I looked at this the more obvious it became that the counts are completely unnecessary. ScrollingCoordinator now only deals with the global presence of handlers and the only counting going on is in tests (and the bit in LocalFrame which is going away). https://codereview.chromium.org/206603002/diff/140001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:763: Node* node = target->toDOMWindow() ? target->toDOMWindow()->document() : target->toNode(); On 2014/03/27 16:43:31, Rick Byers wrote: > Would be simpler to just combine this with the "if(node->isDocumentNode())" > check below (I.e. only get a Node object at all in the else case). Done. https://codereview.chromium.org/206603002/diff/140001/Source/web/tests/WebVie... File Source/web/tests/WebViewTest.cpp (left): https://codereview.chromium.org/206603002/diff/140001/Source/web/tests/WebVie... Source/web/tests/WebViewTest.cpp:1545: // calls to Document::did{Add|Remove|Clear}TouchEventHandler. Verifying that those calls are made On 2014/03/27 16:43:31, Rick Byers wrote: > update comment please Done.
Great work! A couple more things... https://codereview.chromium.org/206603002/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/event-handler-count.html (right): https://codereview.chromium.org/206603002/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/event-handler-count.html:154: debug("Test redundant addEventListener on a div."); heh, thanks for fixing my typo :-) https://codereview.chromium.org/206603002/diff/180001/Source/core/page/scroll... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/206603002/diff/180001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:760: if (DOMWindow* window = target->toDOMWindow()) { Sorry I should have pointed out earlier - this can never happen (it's handled in the special case a few lines up). So you should be able to remove it. https://codereview.chromium.org/206603002/diff/200001/LayoutTests/fast/events... File LayoutTests/fast/events/scroll-event-handler-count.html (right): https://codereview.chromium.org/206603002/diff/200001/LayoutTests/fast/events... LayoutTests/fast/events/scroll-event-handler-count.html:3: description("This test checks that we correctly update the scroll event handler count as event handlers are added and removed"); You meant to remove this file too, right? https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:55: void didMoveToNewDocument(EventTarget&, Document& oldDocument); Nit: A call like "doc1.didMoveToNewDocument(t, doc2)" reads to me like t moved from doc1 to doc2 (not from doc2 to doc1). Perhaps rename it to 'didMoveFromOtherDocument'? Alternately you could change the implementation to match the name (looks like it should be similar either way). Or I guess if there's really no strong association with one document over the other, then it could be a static method like didMoveToNewDocument(target, oldDoc, newDoc) https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:97: friend class DocumentObserver; I thought nested class members were friends automatically? It's not because you're just forward declaring the type that you need to explicitly make it a friend, is it? https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:105: unsigned externalHandlerCount; Now that you don't have handlerCount, it's tempting to consider simplifying this further. Eg. what if you just had a special singleton EventTarget instance that you used to represent 'External'. Then you could eliminate HandlerState entirely and just have an EventTargetSet. I think a bunch of your implementation would collapse too. It's up to you though - I don't know if it'll ultimately be easier to understand that way or not. https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:110: OwnPtr<DocumentObserver> m_documentObserver; Is there any value to the indirection via a pointer here (other than that it makes this header file a little smaller by pushing the class declaration to the cpp file)? It would be a more direct transformation of what you were doing before if you contained them by value. No big deal though (eg. the perf difference should be trivial) - your call...
Thanks again Rick! https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:55: void didMoveToNewDocument(EventTarget&, Document& oldDocument); On 2014/04/03 15:33:19, Rick Byers wrote: > Nit: A call like "doc1.didMoveToNewDocument(t, doc2)" reads to me like t moved > from doc1 to doc2 (not from doc2 to doc1). Perhaps rename it to > 'didMoveFromOtherDocument'? > > Alternately you could change the implementation to match the name (looks like it > should be similar either way). > > Or I guess if there's really no strong association with one document over the > other, then it could be a static method like didMoveToNewDocument(target, > oldDoc, newDoc) Agreed, it reads a little badly. "didMoveFromOtherDocument" sgtm. https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:97: friend class DocumentObserver; On 2014/04/03 15:33:19, Rick Byers wrote: > I thought nested class members were friends automatically? It's not because > you're just forward declaring the type that you need to explicitly make it a > friend, is it? I was thinking that was only the case for Java, but looks like it's true for C++ too since TR1. I'm not sure what language revision Chromium is targeting, but at least GCC and Clang seem happy even without this line so I've removed it. https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:105: unsigned externalHandlerCount; On 2014/04/03 15:33:19, Rick Byers wrote: > Now that you don't have handlerCount, it's tempting to consider simplifying this > further. Eg. what if you just had a special singleton EventTarget instance that > you used to represent 'External'. Then you could eliminate HandlerState > entirely and just have an EventTargetSet. I think a bunch of your > implementation would collapse too. It's up to you though - I don't know if > it'll ultimately be easier to understand that way or not. I see where you're going and it would simplify things somewhat. However, I'm worried that clients of this class would find it odd to deal with an EventTarget that's neither a Node nor a DOMWindow. That could lead to subtle bugs. Maybe the right solution is to make WebViewImpl an EventTarget and get rid of external handlers entirely. https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:110: OwnPtr<DocumentObserver> m_documentObserver; On 2014/04/03 15:33:19, Rick Byers wrote: > Is there any value to the indirection via a pointer here (other than that it > makes this header file a little smaller by pushing the class declaration to the > cpp file)? It would be a more direct transformation of what you were doing > before if you contained them by value. No big deal though (eg. the perf > difference should be trivial) - your call... I picked this way just to reduce the number of implementation details that leak to the main header (and then forgot to remove the observer header includes from there, oops). Since it turns out these observers aren't that complex in the end, I'll just move them back to the header file.
https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:105: unsigned externalHandlerCount; On 2014/04/04 19:53:57, Sami wrote: > On 2014/04/03 15:33:19, Rick Byers wrote: > > Now that you don't have handlerCount, it's tempting to consider simplifying > this > > further. Eg. what if you just had a special singleton EventTarget instance > that > > you used to represent 'External'. Then you could eliminate HandlerState > > entirely and just have an EventTargetSet. I think a bunch of your > > implementation would collapse too. It's up to you though - I don't know if > > it'll ultimately be easier to understand that way or not. > > I see where you're going and it would simplify things somewhat. However, I'm > worried that clients of this class would find it odd to deal with an EventTarget > that's neither a Node nor a DOMWindow. That could lead to subtle bugs. Maybe the > right solution is to make WebViewImpl an EventTarget and get rid of external > handlers entirely. Yeah I wouldn't want to expose this to clients either - I was thinking of it just as an implementation detail (and you could keep all the same public API surface area and never let this special EventTarget leak out), but I guess that would make the eventHandlerTargets implementation more complex. So maybe it's not a net-win afterall. I'm fine with it as is. https://codereview.chromium.org/206603002/diff/200001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:110: OwnPtr<DocumentObserver> m_documentObserver; On 2014/04/04 19:53:57, Sami wrote: > On 2014/04/03 15:33:19, Rick Byers wrote: > > Is there any value to the indirection via a pointer here (other than that it > > makes this header file a little smaller by pushing the class declaration to > the > > cpp file)? It would be a more direct transformation of what you were doing > > before if you contained them by value. No big deal though (eg. the perf > > difference should be trivial) - your call... > > I picked this way just to reduce the number of implementation details that leak > to the main header (and then forgot to remove the observer header includes from > there, oops). Since it turns out these observers aren't that complex in the end, > I'll just move them back to the header file. Ok. Did you want to update m_windowObserver too? https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... Source/core/dom/Document.cpp:5025: void Document::resetLastHandledUserGestureTimestamp() ditto https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... Source/core/dom/Document.h:954: double lastHandledUserGestureTimestamp() const { return m_lastHandledUserGestureTimestamp; } Looks like you added this back (after it was removed in r170826), bad merge? https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... Source/core/dom/Document.h:1316: double m_lastHandledUserGestureTimestamp; ditto
> Yeah I wouldn't want to expose this to clients either - I was thinking of it > just as an implementation detail (and you could keep all the same public API > surface area and never let this special EventTarget leak out), but I guess that > would make the eventHandlerTargets implementation more complex. So maybe it's > not a net-win afterall. I'm fine with it as is. I see. Feels like we should just see if WebViewImpl could be made to fit better with the other event targets somehow. Maybe by adding factoring out a new base class for EventTarget that isn't tied to DOM. > Ok. Did you want to update m_windowObserver too? That's still a pointer because when a document gets detached we remove the window observer. Otherwise the observer would keep counting handlers the document no longer had a connection to. The window handler related tests in event-handler-counts.html cover this case. Those tests were passing before because I had a bug where the window observer took the Document from the DOMWindow (instead of using its owning registry) and thus was effectively getting transferred over to the new registry even though it was owned by another one. Confusing :\ https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... Source/core/dom/Document.cpp:5025: void Document::resetLastHandledUserGestureTimestamp() On 2014/04/04 20:12:27, Rick Byers wrote: > ditto Done. https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Document.h File Source/core/dom/Document.h (right): https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... Source/core/dom/Document.h:954: double lastHandledUserGestureTimestamp() const { return m_lastHandledUserGestureTimestamp; } On 2014/04/04 20:12:27, Rick Byers wrote: > Looks like you added this back (after it was removed in r170826), bad merge? Oh, thanks for pointing this out. I was working remotely over a dodgy connection and somehow managed to mess this up. https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... Source/core/dom/Document.h:1316: double m_lastHandledUserGestureTimestamp; On 2014/04/04 20:12:27, Rick Byers wrote: > ditto Done.
LGTM! On 2014/04/07 14:56:48, Sami wrote: > > Yeah I wouldn't want to expose this to clients either - I was thinking of it > > just as an implementation detail (and you could keep all the same public API > > surface area and never let this special EventTarget leak out), but I guess > that > > would make the eventHandlerTargets implementation more complex. So maybe > it's > > not a net-win afterall. I'm fine with it as is. > > I see. Feels like we should just see if WebViewImpl could be made to fit better > with the other event targets somehow. Maybe by adding factoring out a new base > class for EventTarget that isn't tied to DOM. Yeah that's probably cleaner, but let's not block this CL on it. I'm not sure it's worth the conceptual cost of a new event target base. > > Ok. Did you want to update m_windowObserver too? > > That's still a pointer because when a document gets detached we remove the > window observer. Otherwise the observer would keep counting handlers the > document no longer had a connection to. The window handler related tests in > event-handler-counts.html cover this case. > > Those tests were passing before because I had a bug where the window observer > took the Document from the DOMWindow (instead of using its owning registry) and > thus was effectively getting transferred over to the new registry even though it > was owned by another one. Confusing :\ Ah, I see - thanks for the explanation! > https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... > File Source/core/dom/Document.cpp (right): > > https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... > Source/core/dom/Document.cpp:5025: void > Document::resetLastHandledUserGestureTimestamp() > On 2014/04/04 20:12:27, Rick Byers wrote: > > ditto > > Done. > > https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Document.h > File Source/core/dom/Document.h (right): > > https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... > Source/core/dom/Document.h:954: double lastHandledUserGestureTimestamp() const { > return m_lastHandledUserGestureTimestamp; } > On 2014/04/04 20:12:27, Rick Byers wrote: > > Looks like you added this back (after it was removed in r170826), bad merge? > > Oh, thanks for pointing this out. I was working remotely over a dodgy connection > and somehow managed to mess this up. > > https://codereview.chromium.org/206603002/diff/220001/Source/core/dom/Documen... > Source/core/dom/Document.h:1316: double m_lastHandledUserGestureTimestamp; > On 2014/04/04 20:12:27, Rick Byers wrote: > > ditto > > Done.
Thank you Rick. Ian and Adam, could I get your opinions too please?
ScrollingCoordinator lg2m, and although I really like the rest of the code, I'm really new to that area of blink, so I'll leave it to Adam to stamp. https://codereview.chromium.org/206603002/diff/280001/Source/core/frame/Local... File Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/206603002/diff/280001/Source/core/frame/Local... Source/core/frame/LocalFrame.cpp:471: Is this another call into the "black hole"? If so, maybe add a FIXME? https://codereview.chromium.org/206603002/diff/280001/Source/core/page/scroll... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/206603002/diff/280001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:645: if (WebLayer* scrollLayer = toWebLayer(m_page->mainFrame()->view()->layerForScrolling())) { It took a fair bit of reading in cc to figure out why it would still be correct to only apply the event handlers setting to the main scrolling layer when this needs to affect all scrolling on the page. Perhaps add a comment here with a brief explanation?
https://codereview.chromium.org/206603002/diff/280001/Source/core/frame/Local... File Source/core/frame/LocalFrame.cpp (right): https://codereview.chromium.org/206603002/diff/280001/Source/core/frame/Local... Source/core/frame/LocalFrame.cpp:471: On 2014/04/08 15:40:51, Ian Vollick wrote: > Is this another call into the "black hole"? If so, maybe add a FIXME? Indeed it is. Done. https://codereview.chromium.org/206603002/diff/280001/Source/core/page/scroll... File Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/206603002/diff/280001/Source/core/page/scroll... Source/core/page/scrolling/ScrollingCoordinator.cpp:645: if (WebLayer* scrollLayer = toWebLayer(m_page->mainFrame()->view()->layerForScrolling())) { On 2014/04/08 15:40:51, Ian Vollick wrote: > It took a fair bit of reading in cc to figure out why it would still be correct > to only apply the event handlers setting to the main scrolling layer when this > needs to affect all scrolling on the page. Perhaps add a comment here with a > brief explanation? Yes, that could use a clarification. Added a comment.
Adam, ping?
This CL is too large and complex to review accurately. Can we break it down into smaller pieces? For example, maybe we can move each of the event classes to this new system one at a time. https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2103: lifecycleNotifier().notifyWillDetachDocument(); Typically folks who want this notification implement the ActiveDOMObject interface and listen for stop() https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... File Source/core/dom/DocumentLifecycleNotifier.h (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... Source/core/dom/DocumentLifecycleNotifier.h:44: void notifyDocumentWasDisposed(); Can we sort these in the order they are called? https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:3: // found in the LICENSE file. blank line after this line pls https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:60: ASSERT(!document->domWindow()->hasEventListeners()); I don't think this ASSERT is valid. I think it's possible for a Document to re-use a DOMWindow that existed previously and might have event listeners.. https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:71: } Why are we delaying this processing until attachment? There's nothing special about that time period for events... https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:3: // found in the LICENSE file. Blank line after this line pls.
> This CL is too large and complex to review accurately. Can we break it down > into smaller pieces? For example, maybe we can move each of the event classes > to this new system one at a time. Yes, fair point. I'll rework this to only add the EventHandlerRegistry and do the other parts as follow-up. Responses inline. https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2103: lifecycleNotifier().notifyWillDetachDocument(); On 2014/04/10 21:25:58, abarth wrote: > Typically folks who want this notification implement the ActiveDOMObject > interface and listen for stop() Thanks for the hint. That looks perfect. https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... File Source/core/dom/DocumentLifecycleNotifier.h (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/Documen... Source/core/dom/DocumentLifecycleNotifier.h:44: void notifyDocumentWasDisposed(); On 2014/04/10 21:25:58, abarth wrote: > Can we sort these in the order they are called? Done (removed willDetach). https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:3: // found in the LICENSE file. On 2014/04/10 21:25:58, abarth wrote: > blank line after this line pls Done. https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:60: ASSERT(!document->domWindow()->hasEventListeners()); On 2014/04/10 21:25:58, abarth wrote: > I don't think this ASSERT is valid. I think it's possible for a Document to > re-use a DOMWindow that existed previously and might have event listeners.. Ok, I wasn't sure if that was the case so I added the ASSERT as a precaution. I'll add code to register any existing handlers on the window. https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:71: } On 2014/04/10 21:25:58, abarth wrote: > Why are we delaying this processing until attachment? There's nothing special > about that time period for events... This is something I cargo culted from DOMWindow::installNewDocument[1], but wasn't entirely convinced we needed it either. I did some spelunking and looks like the code originally came from https://bugs.webkit.org/show_bug.cgi?id=60779. Its purpose was to ensure ChromeClient was told the number of wheel handlers when a Frame's Document changed. Nowadays, if I understand correctly, Document::attach() will only called very soon after the document is created in DOMWindow::installNewDocument. This doesn't change the document's relationship to the frame, so it doesn't make sense to (re)send these notifications. In other words, the document is prepared to send the notifications as soon as it is constructed since it already knows about the frame. Therefore, we can just remove this code. Did I miss something? [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/300001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:3: // found in the LICENSE file. On 2014/04/10 21:25:58, abarth wrote: > Blank line after this line pls. Done.
This is now split as follows: 1. Refactor wheel event handler registration in ScrollingCoordinator: https://codereview.chromium.org/234913005 2. Add EventHandlerRegistry: https://codereview.chromium.org/206603002 (this issue) 3. Migrate wheel events to EventHandlerRegistry: https://codereview.chromium.org/225823007 4. Migrate touch events to EventHandlerRegistry: https://codereview.chromium.org/225903009 5. Unify event handler tracking tests: https://codereview.chromium.org/233653006 Each iteration builds and has tests coverage, so hopefully this will be easier to follow.
On 2014/04/11 18:18:26, Sami wrote: > Each iteration builds and has tests coverage, so hopefully this will be easier > to follow. Thanks!
This CL is still too big. :( Can we do one event class at a time? You're still doing both document observations and window observations all all types of events. It's too complex to review properly. I gave up after a while. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2096: lifecycleNotifier().notifyDocumentWasAttached(); What is it appropriate to do this work during Document::attach? Generally speaking, Document::attach is related to visual rendering of the document whereas keeping track of these events has to do with the object model of the document, which is independent of visual rendering. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:61: EventHandlerRegistry* registry = EventHandlerRegistry::from(*document); So, we're hanging the EventHandlerRegistry off the top-level document but none of the other documents? Why not hang the EventHandlerRegistry off the Page? That's generally where we put objects that exist per Page. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:72: } Do you have a test that demonstrates this code being necessary? Previously, you had an ASSERT here. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:73: registry->m_windowObserver = adoptPtr(new WindowObserver(*registry, *document->domWindow())); Why do we only observe the top-level DOMWindow? https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:82: EventHandlerRegistry* registry = EventHandlerRegistry::from(*document); On line 59, we skip creating the EventHandlerRegistry for everything except the top-level Document, but here you're accessing the EventHandlerRegistry on all the documents in the tree. Wouldn't this mean this code is needlessly creating the EventHandlerRegistry on all the child documents only to ignore them? https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:93: parentRegistry->didRemoveAllEventHandlers(*document); What happens if someone adds an event handler after the Document stops? https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:108: , m_windowObserver(adoptPtr(new WindowObserver(*this, *document.domWindow()))) Why is the DocumentObserver held by value but the WindowObserver held by pointer? https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:99: class DocumentObserver: public DocumentLifecycleObserver, public ActiveDOMObject { You shouldn't need to be both a DocumentLifecycleObserver and an ActiveDOMObject. That's redundant. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:102: ~DocumentObserver(); Presumably this needs to be virtual.
> This CL is still too big. :( > > Can we do one event class at a time? You're still doing both document > observations and window observations all all types of events. It's too complex > to review properly. I gave up after a while. I hear you. Thanks for your patience so far. I'll figure out a way to make this smaller by splitting by observers and/or event classes. Thanks to your comments I was already able to reduce a lot of complexity w.r.t window management. Still, please hold off on review until I have something smaller ready. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/Documen... File Source/core/dom/Document.cpp (right): https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/Documen... Source/core/dom/Document.cpp:2096: lifecycleNotifier().notifyDocumentWasAttached(); On 2014/04/11 19:08:01, abarth wrote: > What is it appropriate to do this work during Document::attach? Generally > speaking, Document::attach is related to visual rendering of the document > whereas keeping track of these events has to do with the object model of the > document, which is independent of visual rendering. Turns out I was a little confused about a document's relation to its window and thought there was a way to directly move a document from one window to another. Since this isn't the case (as far as I can tell), we can simplify this problem down to two transitions: 1. A new document is given a window, which may have existing handlers on it => register those handlers. 2. Document is detached => unregister all existing window handlers and stop tracking new ones. The first case can we can deal with when constructing the registry and the second case is served by ActiveDOMObject::stop(). https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:61: EventHandlerRegistry* registry = EventHandlerRegistry::from(*document); On 2014/04/11 19:08:01, abarth wrote: > So, we're hanging the EventHandlerRegistry off the top-level document but none > of the other documents? Why not hang the EventHandlerRegistry off the Page? > That's generally where we put objects that exist per Page. Sorry, this was my bug; actually EventHandlerRegistry needs to be created for all documents that have associated event handlers. The document->parentDocument() guard on line 59 was here to avoid sending (ultimately redundant) notifications about handlers to ScrollingCoordinator et al. I removed the notifications but accidentally left the guard here :\ https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:72: } On 2014/04/11 19:08:01, abarth wrote: > Do you have a test that demonstrates this code being necessary? Previously, you > had an ASSERT here. No, but I'll try to make one based on the comment here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:73: registry->m_windowObserver = adoptPtr(new WindowObserver(*registry, *document->domWindow())); On 2014/04/11 19:08:01, abarth wrote: > Why do we only observe the top-level DOMWindow? That's not intended -- fixed as explained under line 61. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:82: EventHandlerRegistry* registry = EventHandlerRegistry::from(*document); On 2014/04/11 19:08:01, abarth wrote: > On line 59, we skip creating the EventHandlerRegistry for everything except the > top-level Document, but here you're accessing the EventHandlerRegistry on all > the documents in the tree. Wouldn't this mean this code is needlessly creating > the EventHandlerRegistry on all the child documents only to ignore them? In fact we do need an EventHandlerRegistry on all documents that have associated event handlers. The presence or absence of handlers is communicated up the hierarchy to the root registry. It's possible we create unnecessary registries for documents that have handlers of types other than those we are interested in (scroll, wheel and touch), but my gut feeling is that the number of such documents is not high enough to warrant adding code to guard against it. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:93: parentRegistry->didRemoveAllEventHandlers(*document); On 2014/04/11 19:08:01, abarth wrote: > What happens if someone adds an event handler after the Document stops? Good point. I was assuming the document would be completely inert after it stopped. I'll make the registry stop accepting changes unless the document is active. Since I changed how we stop tracking windows (see below), this is covered by iframe tests in scroll-event-handler-count.html. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:108: , m_windowObserver(adoptPtr(new WindowObserver(*this, *document.domWindow()))) On 2014/04/11 19:08:01, abarth wrote: > Why is the DocumentObserver held by value but the WindowObserver held by > pointer? This was so that we could stop tracking handlers on the window once the document is stopped, but now it seems making the registry stop accepting all updates when stopped is a cleaner way to achieve the same. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:99: class DocumentObserver: public DocumentLifecycleObserver, public ActiveDOMObject { On 2014/04/11 19:08:01, abarth wrote: > You shouldn't need to be both a DocumentLifecycleObserver and an > ActiveDOMObject. That's redundant. Right, I don't need anything from DocumentLifecycleObserver anymore. https://codereview.chromium.org/206603002/diff/340001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:102: ~DocumentObserver(); On 2014/04/11 19:08:01, abarth wrote: > Presumably this needs to be virtual. Done.
I think I've now managed to prune this to a smaller version that still does something useful (testable). This code only tracks scroll handlers on a per-Document scope and has no interactions with wheel events, touch events or windows. It also omits reporting handlers to the root document, so ScrollingCoordinator will not know about scroll handlers in child documents. This is fine, because currently ScrollingCoordinator knows about none of them so this is an incremental improvement. Below is the full set of patches -- but let's focus on this one first. 1. Add EventHandlerRegistry (this patch) 2. Track handlers in nested documents (https://codereview.chromium.org/237963014) 3. Track event handlers on windows (Track event handlers on windows) 4. Migrate wheel events to EventHandlerRegistry (https://codereview.chromium.org/225823007/) 5. Migrate touch events to EventHandlerRegistry (https://codereview.chromium.org/225903009/) 6. Unify event handler tracking tests (https://codereview.chromium.org/233653006/)
LGTM w/ some minor comments. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:60: } I presume you've written this function this way because you're going to add to it in the future. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:77: #ifndef NDEBUG #if ASSERT_ENABLED https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:9: #include "core/dom/DocumentLifecycleObserver.h" These two includes don't appear to be used. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:12: #include "core/frame/DOMWindowLifecycleObserver.h" This include doesn't appear to be used. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:55: private: Please add a blank line before this line. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:2008: EventHandlerRegistry::from(document())->didRemoveAllEventHandlers(*this); It's kind of lame that we'll create the EventHandlerRegistry just to remove all event listeners. Maybe that's not worth worrying about. https://codereview.chromium.org/206603002/diff/400001/Source/web/FrameLoaderC... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/206603002/diff/400001/Source/web/FrameLoaderC... Source/web/FrameLoaderClientImpl.cpp:123: EventHandlerRegistry::from(*document); I don't think this is needed. Does your test fail if you remove this line?
Thanks, all comments addressed. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.cpp (right): https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:60: } On 2014/04/17 18:05:12, abarth wrote: > I presume you've written this function this way because you're going to add to > it in the future. That's right. It looks a little awkward now but upcoming patches will extend it. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.cpp:77: #ifndef NDEBUG On 2014/04/17 18:05:12, abarth wrote: > #if ASSERT_ENABLED Done. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... File Source/core/dom/EventHandlerRegistry.h (right): https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:9: #include "core/dom/DocumentLifecycleObserver.h" On 2014/04/17 18:05:12, abarth wrote: > These two includes don't appear to be used. Ah, thanks for pointing that out. Done. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/EventHa... Source/core/dom/EventHandlerRegistry.h:55: private: On 2014/04/17 18:05:12, abarth wrote: > Please add a blank line before this line. Done. https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/206603002/diff/400001/Source/core/dom/Node.cp... Source/core/dom/Node.cpp:2008: EventHandlerRegistry::from(document())->didRemoveAllEventHandlers(*this); On 2014/04/17 18:05:12, abarth wrote: > It's kind of lame that we'll create the EventHandlerRegistry just to remove all > event listeners. Maybe that's not worth worrying about. I think I can guard this with hasEventListeners() which makes that happen less frequently. https://codereview.chromium.org/206603002/diff/400001/Source/web/FrameLoaderC... File Source/web/FrameLoaderClientImpl.cpp (right): https://codereview.chromium.org/206603002/diff/400001/Source/web/FrameLoaderC... Source/web/FrameLoaderClientImpl.cpp:123: EventHandlerRegistry::from(*document); On 2014/04/17 18:05:12, abarth wrote: > I don't think this is needed. Does your test fail if you remove this line? The test passes fine without this. I only added it because WheelController was here too.
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/206603002/440001
Message was sent while issue was closed.
Change committed as 171885 |
