Replace MouseWheel events with GestureScrollBegin in WebContentsObserver::DidGetUserInteraction

Replace MouseWheel events with GestureScrollBegin in WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612
Committed: https://crrev.com/5cc29a417bcb7b86ca8ebc4304eefa4d74a7e919
Cr-Commit-Position: refs/heads/master@{#383231}

[dominickn, 2016-02-29 04:24:23]

dominickn@chromium.org changed reviewers:
+ asanka@chromium.org, calamity@chromium.org, dtapuska@chromium.org,
tdresser@chromium.org, thestig@chromium.org

[dominickn, 2016-02-29 04:24:24]

It is intended that this will land once crbug.com/568183 is finished.

asanka: please review chrome/browser/download

calamity: please review chrome/browser/engagement

thestig: please review chrome/browser/external_protocol

isherman: please review histograms.xml

tdresser + dtapuska: please review content/

Thanks!

[tdresser, 2016-02-29 14:14:12]

content/ LGTM

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_request_limiter.cc:102: if (is_showing_prompt()
|| type == blink::WebInputEvent::GestureScrollBegin) {
This is a behavior change for touch scrolling - was that intended?
Should it be tested?

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/external_pro...
File chrome/browser/external_protocol/external_protocol_observer.cc (right):

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/external_pro...
chrome/browser/external_protocol/external_protocol_observer.cc:23: if (type !=
blink::WebInputEvent::GestureScrollBegin)
This also involves a behavior change for touch, doesn't it?

https://codereview.chromium.org/1748553002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1748553002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:324: // 2) the start of a scroll
event (blink::WebInputEvent::GestureScrollBegin);
the start of a scroll event -> the start of a scroll

https://codereview.chromium.org/1748553002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1748553002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:79550: <int value="3" label="Touch
gesture"/>
These labels are a bit confusing.

Is a touch scroll a "Touch gesture" or a "Scroll"?
Can we come up with labels that are less ambiguous?

[dtapuska, 2016-02-29 14:49:00]

On 2016/02/29 14:14:12, tdresser wrote:
> content/ LGTM
> 
>
https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
> File chrome/browser/download/download_request_limiter.cc (right):
> 
>
https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
> chrome/browser/download/download_request_limiter.cc:102: if
(is_showing_prompt()
> || type == blink::WebInputEvent::GestureScrollBegin) {
> This is a behavior change for touch scrolling - was that intended?
> Should it be tested?
> 
>
https://codereview.chromium.org/1748553002/diff/1/chrome/browser/external_pro...
> File chrome/browser/external_protocol/external_protocol_observer.cc (right):
> 
>
https://codereview.chromium.org/1748553002/diff/1/chrome/browser/external_pro...
> chrome/browser/external_protocol/external_protocol_observer.cc:23: if (type !=
> blink::WebInputEvent::GestureScrollBegin)
> This also involves a behavior change for touch, doesn't it?
> 
>
https://codereview.chromium.org/1748553002/diff/1/content/public/browser/web_...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/1748553002/diff/1/content/public/browser/web_...
> content/public/browser/web_contents_observer.h:324: // 2) the start of a
scroll
> event (blink::WebInputEvent::GestureScrollBegin);
> the start of a scroll event -> the start of a scroll
> 
>
https://codereview.chromium.org/1748553002/diff/1/tools/metrics/histograms/hi...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/1748553002/diff/1/tools/metrics/histograms/hi...
> tools/metrics/histograms/histograms.xml:79550: <int value="3" label="Touch
> gesture"/>
> These labels are a bit confusing.
> 
> Is a touch scroll a "Touch gesture" or a "Scroll"?
> Can we come up with labels that are less ambiguous?

So mouse wheel gestures is conditionally enabled right now. I am working on a
few regressions. From the looks of it this code moves solely over to gesture
scroll begin; so that means versions that don't have the wheel gesture enabled
won't get the site engagement for wheel events. Is that a concern or are you
planning on waiting to land this until wheel gestures is enabled everywhere?

[dominickn, 2016-02-29 22:52:02]

Thanks!

On 2016/02/29 14:49:00, dtapuska wrote:
> So mouse wheel gestures is conditionally enabled right now. I am working on a
> few regressions. From the looks of it this code moves solely over to gesture
> scroll begin; so that means versions that don't have the wheel gesture enabled
> won't get the site engagement for wheel events. Is that a concern or are you
> planning on waiting to land this until wheel gestures is enabled everywhere?

The plan is to land this once mouse wheel gestures have landed.

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_request_limiter.cc:102: if (is_showing_prompt()
|| type == blink::WebInputEvent::GestureScrollBegin) {
On 2016/02/29 14:14:11, tdresser wrote:
> This is a behavior change for touch scrolling - was that intended?
> Should it be tested?

I thought this wasn't a behaviour change? The intention here is that this client
ignores scrolls.

This method was previously ignoring touch scrolls because they weren't being
sent through DidGetUserInteraction. After this CL it will still ignore touch
scrolls, and will continue to not trigger on mouse scrolls. Is there some
behavioural change with when GestureScrollBegin is sent to the input router that
I'm missing?

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/external_pro...
File chrome/browser/external_protocol/external_protocol_observer.cc (right):

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/external_pro...
chrome/browser/external_protocol/external_protocol_observer.cc:23: if (type !=
blink::WebInputEvent::GestureScrollBegin)
On 2016/02/29 14:14:12, tdresser wrote:
> This also involves a behavior change for touch, doesn't it?

See comment for the download request limiter.

https://codereview.chromium.org/1748553002/diff/1/content/public/browser/web_...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1748553002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:324: // 2) the start of a scroll
event (blink::WebInputEvent::GestureScrollBegin);
On 2016/02/29 14:14:12, tdresser wrote:
> the start of a scroll event -> the start of a scroll

Done.

https://codereview.chromium.org/1748553002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1748553002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:79550: <int value="3" label="Touch
gesture"/>
On 2016/02/29 14:14:12, tdresser wrote:
> These labels are a bit confusing.
> 
> Is a touch scroll a "Touch gesture" or a "Scroll"?
> Can we come up with labels that are less ambiguous?

Done.

[dominickn, 2016-02-29 22:57:21]

Description was changed from

==========
Replace MouseWheel events with GestureScrollBegin in
WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612
==========

to

==========
Replace MouseWheel events with GestureScrollBegin in
WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612
==========

[dominickn, 2016-02-29 22:57:21]

dominickn@chromium.org changed reviewers:
+ jochen@chromium.org
- thestig@chromium.org

[dominickn, 2016-02-29 22:58:15]

jochen: PTAL at chrome/browser/external_protocol. Thanks!

[jochen (gone - plz use gerrit), 2016-03-01 14:38:27]

c/b/external_protocol lgtm

[asanka, 2016-03-01 14:54:10]

c/b/download lgtm

[tdresser, 2016-03-01 15:12:10]

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_request_limiter.cc:102: if (is_showing_prompt()
|| type == blink::WebInputEvent::GestureScrollBegin) {
On 2016/02/29 22:52:02, dominickn wrote:
> On 2016/02/29 14:14:11, tdresser wrote:
> > This is a behavior change for touch scrolling - was that intended?
> > Should it be tested?
> 
> I thought this wasn't a behaviour change? The intention here is that this
client
> ignores scrolls.
> 
> This method was previously ignoring touch scrolls because they weren't being
> sent through DidGetUserInteraction. After this CL it will still ignore touch
> scrolls, and will continue to not trigger on mouse scrolls. Is there some
> behavioural change with when GestureScrollBegin is sent to the input router
that
> I'm missing?

Sorry, I missed that touch gesture events aren't being passed through
DidGetUserInteraction.

You're right.

[dominickn, 2016-03-01 22:54:31]

dominickn@chromium.org changed reviewers:
+ isherman@chromium.org

[dominickn, 2016-03-01 22:56:36]

On 2016/03/01 15:12:10, tdresser wrote:
> Sorry, I missed that touch gesture events aren't being passed through
> DidGetUserInteraction.
> 
> You're right.

Thanks for clarifying! :)

+isherman: please review histograms.xml for clarity (this CL only has
renaming). This CL is intended to land once wheel gestures have
landed and stuck (crbug.com/568183)

[Ilya Sherman, 2016-03-02 01:00:11]

histograms.xml lgtm

[dominickn, 2016-03-25 01:11:07]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-03-25 01:11:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1748553002/#ps20001
(title: "Addressing reviewer comments")

[commit-bot: I haz the power, 2016-03-25 01:11:19]

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

[commit-bot: I haz the power, 2016-03-25 03:06:31]

Description was changed from

==========
Replace MouseWheel events with GestureScrollBegin in
WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612
==========

to

==========
Replace MouseWheel events with GestureScrollBegin in
WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612
==========

[commit-bot: I haz the power, 2016-03-25 03:06:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-25 03:08:06]

Description was changed from

==========
Replace MouseWheel events with GestureScrollBegin in
WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612
==========

to

==========
Replace MouseWheel events with GestureScrollBegin in
WebContentsObserver::DidGetUserInteraction

crbug.com/568183 implements GestureScrollBegin for mouse wheel events.
Once this has landed, the mouse event coalescing currently performed by
WebContentsObserver::DidGetUserInteraction will be redundant.

This CL removes the mouse event coalescing, and changes
DidGetUserInteraction to fire on GestureScrollBegin events. The event
type used to signal scrolls in the method is changed from MouseWheel to
GestureScrollBegin, as scroll events will now be fired on mobile scrolls
as well as wheel scrolls. Finally, in clients of DidGetUserInteraction,
MouseWheel is replaced with GestureScrollBegin, and references to
'wheel' are replaced with 'scroll'.

BUG=590612

Committed: https://crrev.com/5cc29a417bcb7b86ca8ebc4304eefa4d74a7e919
Cr-Commit-Position: refs/heads/master@{#383231}
==========

[commit-bot: I haz the power, 2016-03-25 03:08:07]

Patchset 2 (id:??) landed as
https://crrev.com/5cc29a417bcb7b86ca8ebc4304eefa4d74a7e919
Cr-Commit-Position: refs/heads/master@{#383231}