Migrate wheel events to EventHandlerRegistry

Migrate wheel events to EventHandlerRegistry

Remove the specialized code for tracking wheel events in favor of the
generic EventHandlerRegistry. Covered by existing tests.

See https://codereview.chromium.org/206603002/ for full review history.

BUG=359566, 332793
TEST=fast/events/wheelevent-handler-count.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175485

[Rick Byers, 2014-04-11 18:30:49]

I quickly skimmed this to verify it was what I already reviewed.  LGTM (but
please add a link to the earlier review).

[Sami, 2014-05-30 17:59:39]

Hey Rick, this is now rewritten against what we landed. Mind taking another
look?

[Rick Byers, 2014-05-31 03:16:40]

LGTM, thanks for doing this!  Touch events next I hope?

https://codereview.chromium.org/225823007/diff/80001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/225823007/diff/80001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:510: // TODO(skyostil): No-one is using this
information, so remove this code.
Seems a little silly to update this code if you're just about to remove it
(doesn't look like there's much involved in removing it - just a handful of
deletions I think).  But I'm fine with this if you prefer to keep it separate
and submit another CL soon to rip it out.

https://codereview.chromium.org/225823007/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225823007/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1479:
page()->frameHost().eventHandlerRegistry().didAddExternalEventHandler(EventTypeNames::wheel);
I know you're just updating the existing code, but this seems fishy.  I wonder
why it's necessary.  Seems unfortunate to add the external event handler
mechanism just for this (but there was something else we needed it for right?). 
Anyway it's probably not necessary to dig into it now.

[Sami, 2014-06-02 10:00:21]

Thanks Rick. Touch events are indeed next on my list.

Adam, could you check the changes under Source/web please?

https://codereview.chromium.org/225823007/diff/80001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/225823007/diff/80001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:510: // TODO(skyostil): No-one is using this
information, so remove this code.
On 2014/05/31 03:16:40, Rick Byers wrote:
> Seems a little silly to update this code if you're just about to remove it
> (doesn't look like there's much involved in removing it - just a handful of
> deletions I think).  But I'm fine with this if you prefer to keep it separate
> and submit another CL soon to rip it out.

Right, I just wanted to limit the amount of noise in this patch -- I think this
plumbs all the way to Java on Android. I'll put together another patch to get
rid of it.

https://codereview.chromium.org/225823007/diff/80001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225823007/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1479:
page()->frameHost().eventHandlerRegistry().didAddExternalEventHandler(EventTypeNames::wheel);
On 2014/05/31 03:16:40, Rick Byers wrote:
> I know you're just updating the existing code, but this seems fishy.  I wonder
> why it's necessary.  Seems unfortunate to add the external event handler
> mechanism just for this (but there was something else we needed it for
right?). 
> Anyway it's probably not necessary to dig into it now.

This is the only place where we need external event handlers :( Looks like this
is here to make sure popup windows are closed when you scroll:
https://chromiumcodereview.appspot.com/23677012. It feels like we should instead
close the popups as a response to a changing scroll offset -- but that could
have other side-effects.

[Rick Byers, 2014-06-02 18:39:44]

On 2014/06/02 10:00:21, Sami wrote:
> Thanks Rick. Touch events are indeed next on my list.
> 
> Adam, could you check the changes under Source/web please?

Adam is on paternity leave, +ojan for OWNERS stamp here instead
 
>
https://codereview.chromium.org/225823007/diff/80001/Source/core/frame/LocalF...
> File Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/225823007/diff/80001/Source/core/frame/LocalF...
> Source/core/frame/LocalFrame.cpp:510: // TODO(skyostil): No-one is using this
> information, so remove this code.
> On 2014/05/31 03:16:40, Rick Byers wrote:
> > Seems a little silly to update this code if you're just about to remove it
> > (doesn't look like there's much involved in removing it - just a handful of
> > deletions I think).  But I'm fine with this if you prefer to keep it
separate
> > and submit another CL soon to rip it out.
> 
> Right, I just wanted to limit the amount of noise in this patch -- I think
this
> plumbs all the way to Java on Android. I'll put together another patch to get
> rid of it.
> 
>
https://codereview.chromium.org/225823007/diff/80001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/225823007/diff/80001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:1479:
>
page()->frameHost().eventHandlerRegistry().didAddExternalEventHandler(EventTypeNames::wheel);
> On 2014/05/31 03:16:40, Rick Byers wrote:
> > I know you're just updating the existing code, but this seems fishy.  I
wonder
> > why it's necessary.  Seems unfortunate to add the external event handler
> > mechanism just for this (but there was something else we needed it for
> right?). 
> > Anyway it's probably not necessary to dig into it now.
> 
> This is the only place where we need external event handlers :( Looks like
this
> is here to make sure popup windows are closed when you scroll:
> https://chromiumcodereview.appspot.com/23677012. It feels like we should
instead
> close the popups as a response to a changing scroll offset -- but that could
> have other side-effects.

Hmm, do we really need this parallel tracking mechanism just for this
(questionable) case?  Why not just add a fake handler for the Document (which is
already an EventTarget) for this case?  As long as the lifetime semantics are
the same (eg. document destruction should clear this handler) then it should be
ok, right?  There's other places where we add a handler for a Node that isn't a
true event handler registration (eg. InputType and TouchAction handling of touch
events).

[ojan, 2014-06-02 21:23:03]

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Event...
File Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Event...
Source/core/frame/EventHandlerRegistry.cpp:209: // TODO(skyostil): This
notification is wired up to a black hole, so remove it.
Blink style is:
// FIXME: ...

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:517: if (targets) {
There's no official style rule, but we typically do the following:
if (const EventTargetSet* targets =
registry.eventHandlerTargets(EventHandlerRegistry::WheelEvent))

It scopes the variable to the if clause and since you're declaring the variable
here, there's no possibility of silly bugs by accidentally typing == or
something like that.

[ojan, 2014-06-02 21:23:13]

lgtm

[ojan, 2014-06-02 21:23:50]

lgtm, but please address the style nits before landing.

Sorry for the noise, trying out a new code review tool.

[Sami, 2014-06-03 15:57:44]

Thanks, all addressed.

> Hmm, do we really need this parallel tracking mechanism just for this
> (questionable) case?  Why not just add a fake handler for the Document (which
is
> already an EventTarget) for this case?  As long as the lifetime semantics are
> the same (eg. document destruction should clear this handler) then it should
be
> ok, right?  There's other places where we add a handler for a Node that isn't
a
> true event handler registration (eg. InputType and TouchAction handling of
touch
> events).

Thanks for the excellent suggestion. By adding registering the "handler" on the
Document we can avoid all this external handler nonsense.

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Event...
File Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Event...
Source/core/frame/EventHandlerRegistry.cpp:209: // TODO(skyostil): This
notification is wired up to a black hole, so remove it.
On 2014/06/02 21:23:04, ojan wrote:
> Blink style is:
> // FIXME: ...

Done.

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/225823007/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:517: if (targets) {
On 2014/06/02 21:23:04, ojan wrote:
> There's no official style rule, but we typically do the following:
> if (const EventTargetSet* targets =
> registry.eventHandlerTargets(EventHandlerRegistry::WheelEvent))
> 
> It scopes the variable to the if clause and since you're declaring the
variable
> here, there's no possibility of silly bugs by accidentally typing == or
> something like that.

Ah yes, that's a good idea. Done.

[Sami, 2014-06-03 16:17:21]

On 2014/06/03 15:57:44, Sami wrote:
> Thanks for the excellent suggestion. By adding registering the "handler" on
the
> Document we can avoid all this external handler nonsense.

Turns out this was slightly more subtle: we need to remove the fake handler
conditionally since it may have already been removed if the Document was
unloaded. For example, opening a pop-up and navigating back while it was still
open.

[Rick Byers, 2014-06-03 21:32:33]

Nice, thanks for getting rid of the external handler mechanism - this is
definitely simpler.  LGTM with one nit.

https://codereview.chromium.org/225823007/diff/140001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/225823007/diff/140001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1495: if
(registry->eventHandlerTargets(EventHandlerRegistry::WheelEvent)->contains(&document))
nit: add a comment saying why the 'if' is necessary (I found these sorts of
cleanup ordering dependencies to be a real mess when I did the last round of
cleanups on the touch handler tracking).

[Sami, 2014-06-04 10:11:04]

On 2014/06/03 21:32:33, Rick Byers wrote:
>
https://codereview.chromium.org/225823007/diff/140001/Source/web/WebViewImpl....
> Source/web/WebViewImpl.cpp:1495: if
>
(registry->eventHandlerTargets(EventHandlerRegistry::WheelEvent)->contains(&document))
> nit: add a comment saying why the 'if' is necessary (I found these sorts of
> cleanup ordering dependencies to be a real mess when I did the last round of
> cleanups on the touch handler tracking).

Yes, it would be nice to be able to understand this a year from now. Done.

[Sami, 2014-06-04 10:11:21]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-04 10:12:13]

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

[commit-bot: I haz the power, 2014-06-04 10:12:21]

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

[commit-bot: I haz the power, 2014-06-04 10:12:21]

Failed to apply patch for Source/core/page/scrolling/ScrollingCoordinator.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/page/scrolling/ScrollingCoordinator.cpp
  Hunk #2 FAILED at 663.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/page/scrolling/ScrollingCoordinator.cpp.rej

Patch:       Source/core/page/scrolling/ScrollingCoordinator.cpp
Index: Source/core/page/scrolling/ScrollingCoordinator.cpp
diff --git a/Source/core/page/scrolling/ScrollingCoordinator.cpp
b/Source/core/page/scrolling/ScrollingCoordinator.cpp
index
26291466a173e59bd53c3172d4fc6487e2d05445..7991b971acb740f3b7bd0121d325d9f901bd2eb9
100644
--- a/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -31,7 +31,6 @@
 #include "core/dom/Document.h"
 #include "core/dom/FullscreenElementStack.h"
 #include "core/dom/Node.h"
-#include "core/dom/WheelController.h"
 #include "core/frame/EventHandlerRegistry.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
@@ -664,13 +663,8 @@ void ScrollingCoordinator::updateHaveWheelEventHandlers()
         return;
 
     if (WebLayer* scrollLayer =
toWebLayer(m_page->mainFrame()->view()->layerForScrolling())) {
-        unsigned wheelEventHandlerCount = 0;
-
-        for (LocalFrame* frame = m_page->mainFrame(); frame; frame =
frame->tree().traverseNext()) {
-            wheelEventHandlerCount +=
WheelController::from(*frame->document())->wheelEventHandlerCount();
-        }
-
-        scrollLayer->setHaveWheelEventHandlers(wheelEventHandlerCount);
+        bool haveHandlers =
m_page->frameHost().eventHandlerRegistry().hasEventHandlers(EventHandlerRegistry::WheelEvent);
+        scrollLayer->setHaveWheelEventHandlers(haveHandlers);
     }
 }

[Paweł Hajdan Jr., 2014-06-04 12:11:27]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-06-04 12:11:55]

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

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

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

[commit-bot: I haz the power, 2014-06-04 12:12:07]

Failed to apply patch for Source/core/page/scrolling/ScrollingCoordinator.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/page/scrolling/ScrollingCoordinator.cpp
  Hunk #2 FAILED at 663.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/page/scrolling/ScrollingCoordinator.cpp.rej

Patch:       Source/core/page/scrolling/ScrollingCoordinator.cpp
Index: Source/core/page/scrolling/ScrollingCoordinator.cpp
diff --git a/Source/core/page/scrolling/ScrollingCoordinator.cpp
b/Source/core/page/scrolling/ScrollingCoordinator.cpp
index
26291466a173e59bd53c3172d4fc6487e2d05445..7991b971acb740f3b7bd0121d325d9f901bd2eb9
100644
--- a/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -31,7 +31,6 @@
 #include "core/dom/Document.h"
 #include "core/dom/FullscreenElementStack.h"
 #include "core/dom/Node.h"
-#include "core/dom/WheelController.h"
 #include "core/frame/EventHandlerRegistry.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
@@ -664,13 +663,8 @@ void ScrollingCoordinator::updateHaveWheelEventHandlers()
         return;
 
     if (WebLayer* scrollLayer =
toWebLayer(m_page->mainFrame()->view()->layerForScrolling())) {
-        unsigned wheelEventHandlerCount = 0;
-
-        for (LocalFrame* frame = m_page->mainFrame(); frame; frame =
frame->tree().traverseNext()) {
-            wheelEventHandlerCount +=
WheelController::from(*frame->document())->wheelEventHandlerCount();
-        }
-
-        scrollLayer->setHaveWheelEventHandlers(wheelEventHandlerCount);
+        bool haveHandlers =
m_page->frameHost().eventHandlerRegistry().hasEventHandlers(EventHandlerRegistry::WheelEvent);
+        scrollLayer->setHaveWheelEventHandlers(haveHandlers);
     }
 }

[Sami, 2014-06-04 12:45:22]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-04 12:46:29]

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

[commit-bot: I haz the power, 2014-06-04 14:18:29]

Change committed as 175485