Communicate whether passive event listeners exist to cc.

Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f206a40dcff89bf50517b1b4e109c25c277bad43
Cr-Commit-Position: refs/heads/master@{#373915}

[dtapuska, 2016-01-12 16:30:27]

Description was changed from

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
==========

to

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dtapuska, 2016-01-12 19:28:05]

Description was changed from

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dtapuska, 2016-01-12 19:28:05]

dtapuska@chromium.org changed reviewers:
+ aelias@chromium.org, rbyers@chromium.org

[aelias_OOO_until_Jul13, 2016-01-12 20:03:12]

Unless I missed something, it looks like the new touch events options field is
dead code as of this patch.  Could you go further and write the code that
actually does something with it (either as a separate dependent patch or
included in this one would be fine by me)?  It seems like it wouldn't be that
much extra code at this point and would help clarify if this is the best way to
plumb it.

https://codereview.chromium.org/1577263004/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1577263004/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:616:
layer->SetWheelEventProperties(wheel_event_properties_);
I think we also need a SetTouchEventProperties call here too, or the property
will never make it past the pending tree?

[dtapuska, 2016-01-12 21:25:49]

Adding a dependent change is going to be a few days while I work out the IPCs.

https://codereview.chromium.org/1577263004/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1577263004/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:616:
layer->SetWheelEventProperties(wheel_event_properties_);
On 2016/01/12 20:03:11, aelias wrote:
> I think we also need a SetTouchEventProperties call here too, or the property
> will never make it past the pending tree?

Done.

[aelias_OOO_until_Jul13, 2016-01-26 04:08:24]

Seems OK, I'm just wondering if per-layer tracking might actually be overkill
micro-optimization for passive listeners and we could just as well have a global
flag on the document?  I expect most users of passive listeners will prefer to
put them on the whole document in any case.  Rick, any opinion on this -- I know
you experienced a lot of pain introducing the per-layer tracking (that said it
might be a sunk cost at this point), any preference?

[Rick Byers, 2016-01-26 15:17:07]

On 2016/01/26 04:08:24, aelias wrote:
> Seems OK, I'm just wondering if per-layer tracking might actually be overkill
> micro-optimization for passive listeners and we could just as well have a
global
> flag on the document?  I expect most users of passive listeners will prefer to
> put them on the whole document in any case.  Rick, any opinion on this -- I
know
> you experienced a lot of pain introducing the per-layer tracking (that said it
> might be a sunk cost at this point), any preference?

The big pain I had was tracking rects-per-layer.  Compiling and maintaining the
rect-list / region is a huge hassle.  But I thought a single bit per layer
should be pretty simple.

That said, there's not a ton of benefit to tracking "has passive listeners" vs.
"has no listeners" separately per layer.  Often the listeners are on the root
layer, and sending events over to blink unnecessarily isn't a huge deal (just
some CPU work to construct the EventPath etc, can be non-trivial in some cases
but those should probably be optimized anyway).

The real benefit is in tracking "has active listeners" vs. "has passive or no
listeners".  Ultimately I want to do this per DisplayAtom to get (at least) the
granularity of the current rect-tracking code.  But in the interim, tracking
per-layer seems like the right tradeoff (i.e. we shouldn't jump through hoops to
lower the unit of granularity for the compositor, but we should take advantage
of the granularity in which the compositor operates).

But in the short term we have CC touch hit rects as a work-around for touch, and
we probably don't care enough about wheel to make a major investment there.  So
if there's a bunch of extra complexity required now to track per-layer instead
of globally, and that'll all be replaced once we have impl-side display-atom
hit-testing, then I'm fine keeping the temporary state as simple as possible and
rely only on global tracking.

But if it's not much more work to track per-layer, that's closer to the ultimate
design we want and will give us some benefits, so I'd prefer it.  It probably
wouldn't be a ton of work to update the usage of EventHandlerRegistry to report
handler state per-layer to CC, and would fix at least some wheel cases today.

[aelias_OOO_until_Jul13, 2016-01-26 19:16:33]

https://codereview.chromium.org/1577263004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/1577263004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:700: if
(WebLayer* scrollLayer =
toWebLayer(m_page->deprecatedLocalMainFrame()->view()->layerForScrolling())) {
Hmm, on the Blink side we only ever set it on the root scrolling layer.  So the
per-layer tracking and hit-testing stuff this patch has is actually equivalent
to a single root layer enum.  I see that was also the case with the wheel event
properties already.

When the slimming paint v2 change Rick mentioned happens, this code will be
fully deleted and replaced, so this layer-based tracking won't to ever be used
in a meaningful way.  So I'd indeed prefer this to be simplified down to a
global now.  Also, please replace the existing layer wheel event listener stuff
with a global.

[dtapuska, 2016-01-29 21:46:40]

dtapuska@chromium.org changed reviewers:
+ tdresser@chromium.org

[dtapuska, 2016-01-29 21:46:41]

PTAL; I've rebased this off the WebLayerTreeView changes.

+tdresser; as I incorporated his request to use an enum class instead of having
multiple methods for touch/wheel.

https://codereview.chromium.org/1577263004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/1577263004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:700: if
(WebLayer* scrollLayer =
toWebLayer(m_page->deprecatedLocalMainFrame()->view()->layerForScrolling())) {
On 2016/01/26 19:16:33, aelias wrote:
> Hmm, on the Blink side we only ever set it on the root scrolling layer.  So
the
> per-layer tracking and hit-testing stuff this patch has is actually equivalent
> to a single root layer enum.  I see that was also the case with the wheel
event
> properties already.
> 
> When the slimming paint v2 change Rick mentioned happens, this code will be
> fully deleted and replaced, so this layer-based tracking won't to ever be used
> in a meaningful way.  So I'd indeed prefer this to be simplified down to a
> global now.  Also, please replace the existing layer wheel event listener
stuff
> with a global.

Done.

[tdresser, 2016-02-01 14:49:13]

Let me know if you want a full review from me.

At a high level, the approach looks good here, and
cc/input/event_listener_properties.h LGTM.

[Rick Byers, 2016-02-02 21:44:46]

I've reviewed all of WebKit so far (plane is descending so may not get to the
rest before I loose WiFi - don't block on me for the non-WebKit bits though).

https://codereview.chromium.org/1577263004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.h (right):

https://codereview.chromium.org/1577263004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.h:33: WheelEvent,
nit rename this to WheelEventBlocking, and TouchEvent to TouchEventBlocking to
make it clear that Passive is not a subset of the total, and to ensure we're
actually updating the expectations of all the consumers (in many cases it's no
longer sufficient to just ask if there are any 'WheelEvent' listeners).

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:74: *result =
options.passive() ? TouchEventPassive : TouchEvent;
Pointer events never block scrolling, so their listeners should always be
considered passive.

We need a test for this somewhere (but no need to block this CL on any
non-trivial pointer events related stuff since that's still work in progress -
but do file a bug).

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:895: void
ChromeClientImpl::setHaveScrollEventHandlers(bool hasEventHandlers)
should these be folded into the above general functions now too?

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebEventListenerProperties.h (right):

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:12: enum class
WebEventListenerClass {
mention here that "touch" includes pointer event listeners too.
And I guess "MouseWheel" means DOM "wheel" and "mousewheel" events.

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:17: enum class
WebEventListenerProperties {
Please add a brief comment (in particular I find the word "properties" a little
non-obvious, but not worth renaming everything <grin>).  Maybe "Indicates the
variety of event listeners present for a given class of event"?

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:20: Passive =
0x2,
Do you really need to differentiate the 'Blocking|Passive' and 'Blocking' cases?
 If not, I think this would be simpler as a 3 element enum (can just do equality
or even < > comparison instead of bits).  Also folding in this 4th case may help
reduce the risk of certain types of bugs (we don't want the presense/absense of
passive listeners to be observable).

[aelias_OOO_until_Jul13, 2016-02-02 22:18:50]

lgtm

https://codereview.chromium.org/1577263004/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebEventListenerProperties.h (right):

https://codereview.chromium.org/1577263004/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebEventListenerProperties.h:1: // Copyright
2015 The Chromium Authors. All rights reserved.
nit: Copyright 2016

[dtapuska, 2016-02-03 16:30:58]

aelias@; can you take a quick look at the diff?

I changed cc's uint32_t -> EventListenerProperties enum so would prefer a second
pass.

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:74: *result =
options.passive() ? TouchEventPassive : TouchEvent;
On 2016/02/02 21:44:46, Rick Byers wrote:
> Pointer events never block scrolling, so their listeners should always be
> considered passive.
> 
> We need a test for this somewhere (but no need to block this CL on any
> non-trivial pointer events related stuff since that's still work in progress -
> but do file a bug).

Done.

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:895: void
ChromeClientImpl::setHaveScrollEventHandlers(bool hasEventHandlers)
On 2016/02/02 21:44:46, Rick Byers wrote:
> should these be folded into the above general functions now too?

I treated scroll event handlers as a separate one since it is always only
boolean in condition.

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebEventListenerProperties.h (right):

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:12: enum class
WebEventListenerClass {
On 2016/02/02 21:44:46, Rick Byers wrote:
> mention here that "touch" includes pointer event listeners too.
> And I guess "MouseWheel" means DOM "wheel" and "mousewheel" events.

Done.

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:17: enum class
WebEventListenerProperties {
On 2016/02/02 21:44:46, Rick Byers wrote:
> Please add a brief comment (in particular I find the word "properties" a
little
> non-obvious, but not worth renaming everything <grin>).  Maybe "Indicates the
> variety of event listeners present for a given class of event"?

Done.

https://codereview.chromium.org/1577263004/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:20: Passive =
0x2,
On 2016/02/02 21:44:46, Rick Byers wrote:
> Do you really need to differentiate the 'Blocking|Passive' and 'Blocking'
cases?
>  If not, I think this would be simpler as a 3 element enum (can just do
equality
> or even < > comparison instead of bits).  Also folding in this 4th case may
help
> reduce the risk of certain types of bugs (we don't want the presense/absense
of
> passive listeners to be observable).

Done.

[aelias_OOO_until_Jul13, 2016-02-03 20:01:58]

Still lgtm

[Rick Byers, 2016-02-04 22:18:57]

LGTM with a couple nits

https://codereview.chromium.org/1577263004/diff/160001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1577263004/diff/160001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:738: // on impl (despite the
scroll-blocks-on mode).
Looks like we missed updating the comment and test name when you removed
scroll-blocks-on.  I think the one above (L718) is probably also stale.  This
test probably should be renamed / reworked to be testing passive vs. active vs.
nothing wheel listener behavior.  But maybe you want to do that in your next CL
instead and just leave it confusing for now?

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/EventHandlerRegistry.cpp:73: // about touch
events for now.
nit: I'm confused by the "for now".  The compositor cares only about touch
listeners precisely because it's only touch listeners that can block gesture
scrolls.  So maybe reword as "since pointer events never block scrolling and so
the compositor only needs to know about the touch listeners"?

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLInputElement.cpp:179:
document().frameHost()->eventHandlerRegistry().didRemoveEventHandler(*this,
EventHandlerRegistry::TouchEventBlocking);
nit: Add a TODO saying to make this passive, referencing crbug.com/584438 (which
I just filed with details).

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2043: // a touchstart
handler that must be reported.
nit: add a TODO to remove this blocking touch handler - reference
http://crbug.com/318381

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebEventListenerProperties.h (right):

https://codereview.chromium.org/1577263004/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebEventListenerProperties.h:21: Blocking,
nit: add a comment clarifying that Blocking includes cases that also have
passive listeners.

[dtapuska, 2016-02-04 22:49:06]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-02-04 22:49:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, aelias@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1577263004/#ps180001
(title: "Fix nits")

[commit-bot: I haz the power, 2016-02-04 22:53:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577263004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577263004/180001

[commit-bot: I haz the power, 2016-02-04 23:12:49]

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

[commit-bot: I haz the power, 2016-02-04 23:12:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dtapuska, 2016-02-05 14:18:34]

dtapuska@chromium.org changed reviewers:
+ creis@chromium.org

[dtapuska, 2016-02-05 14:18:56]

On 2016/02/04 23:12:50, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

+creis for content/* (trivial changes)

[Charlie Reis, 2016-02-05 17:56:54]

content/ LGTM

[dtapuska, 2016-02-05 17:58:47]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-02-05 18:00:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1577263004/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1577263004/180001

[commit-bot: I haz the power, 2016-02-05 21:36:10]

Description was changed from

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-05 21:36:11]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-02-05 22:04:04]

Description was changed from

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Communicate whether passive event listeners exist to cc.

cc does nothing with these new fields being set other than plumbing them
through the pipeline to be later queried on the layer_impl.

Add an additional field whether touch event listeners exist on the layer
(which unfortunately is effectively implement as an entire document field
in the blink EventListenerRegistry; this can be improved). There still
exists TouchRegionRects that indicate the areas of blocking event
listeners; but whether a passive event listener exists will be indicated
in this field. It is intended that this model will be replicated
with property trees when it is ready.

BUG=489802
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f206a40dcff89bf50517b1b4e109c25c277bad43
Cr-Commit-Position: refs/heads/master@{#373915}
==========

[commit-bot: I haz the power, 2016-02-05 22:04:05]

Patchset 10 (id:??) landed as
https://crrev.com/f206a40dcff89bf50517b1b4e109c25c277bad43
Cr-Commit-Position: refs/heads/master@{#373915}