Refactor wheel event handler registration in ScrollingCoordinator

Refactor wheel event handler registration in ScrollingCoordinator

This patch simplifies the process of observing wheel event handlers in
the ScrollingCoordinator. We do not need to compute exact counts of
handlers but instead check whether there are any or not.

We also add an API for indicating the presence of scroll event handlers.
This will be used by the compositor to schedule scrolling differently when
handlers are consuming scroll updates.

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

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

[Rick Byers, 2014-04-11 18:29:51]

Other than splitting it out, there's no change here from what I've reviewed
already, right?  Please add a link in the CL description to the original review
to enable following the review history.  LGTM.

[abarth-chromium, 2014-04-11 18:53:38]

LGTM

https://codereview.chromium.org/234913005/diff/1/Source/core/dom/WheelControl...
File Source/core/dom/WheelController.cpp (right):

https://codereview.chromium.org/234913005/diff/1/Source/core/dom/WheelControl...
Source/core/dom/WheelController.cpp:72:
scrollingCoordinator->haveWheelEventHandlersChangedForPage();
didChangeWheelEventHandlers() ?

https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:661: if (frame->document())
Can frame->document() actually be zero here?  That seems unlikely...

https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:844: }
Why not have the callers of this function just call
updateHaveWheelEventHandlers() directly?  It seems unnecessary to have this
intermediate function.

https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:852: }
ditto

https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.h:75: void
haveScrollEventHandlersChangedForPage();
I'd skip these comments.

didChangeScrollEventHandlers?

We generally use Objective-C style names for these sorts of notifications.  The
"ForPage" bit is redundant because ScrollingCoordinator is already a Page-level
object.

[Rick Byers, 2014-04-11 19:01:18]

On 2014/04/11 18:53:38, abarth wrote:
> LGTM
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/dom/WheelControl...
> File Source/core/dom/WheelController.cpp (right):
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/dom/WheelControl...
> Source/core/dom/WheelController.cpp:72:
> scrollingCoordinator->haveWheelEventHandlersChangedForPage();
> didChangeWheelEventHandlers() ?
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
> File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:661: if
(frame->document())
> Can frame->document() actually be zero here?  That seems unlikely...
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:844: }
> Why not have the callers of this function just call
> updateHaveWheelEventHandlers() directly?  It seems unnecessary to have this
> intermediate function.
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
> Source/core/page/scrolling/ScrollingCoordinator.cpp:852: }
> ditto
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
> File Source/core/page/scrolling/ScrollingCoordinator.h (right):
> 
>
https://codereview.chromium.org/234913005/diff/1/Source/core/page/scrolling/S...
> Source/core/page/scrolling/ScrollingCoordinator.h:75: void
> haveScrollEventHandlersChangedForPage();
> I'd skip these comments.
> 
> didChangeScrollEventHandlers?

I'm the one that suggested 'haveScrollEventHandlersChanged'.  It's the 'are
there handlers or not' bit whose change we're reporting, not any change in the
handlers.  Perhaps 'didChangeHasScrollEventHandlers'
 
> We generally use Objective-C style names for these sorts of notifications. 
The
> "ForPage" bit is redundant because ScrollingCoordinator is already a
Page-level
> object.

[abarth-chromium, 2014-04-11 20:08:15]

On 2014/04/11 19:01:18, Rick Byers wrote:
> I'm the one that suggested 'haveScrollEventHandlersChanged'.  It's the 'are
> there handlers or not' bit whose change we're reporting, not any change in the
> handlers.  Perhaps 'didChangeHasScrollEventHandlers'

Yeah, that sounds fine.  Another option is to call updateHaveWheelEventHandlers
directly.

[Sami, 2014-04-14 17:57:13]

Thanks Rick and Adam. I went with exposing updateHaveWheelEventHandlers() since
that seemed cleanest.

[Sami, 2014-04-14 17:57:26]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-04-14 17:57:34]

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

[commit-bot: I haz the power, 2014-04-14 19:10:13]

Change committed as 171506