Remove WebContentsObserver::DidGetUserGesture.

Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154
Committed: https://crrev.com/9c7ceaa3b757e38ebf6ac5b59516ff54c8f786b6
Cr-Commit-Position: refs/heads/master@{#378071}

[dominickn, 2016-02-25 04:38:47]

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

[dominickn, 2016-02-25 04:38:48]

PTAL, and recommend a content/ owner for review as nasko@ is currently away.
Thanks!

[tdresser, 2016-02-25 14:42:48]

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

[tdresser, 2016-02-25 14:42:48]

LGTM with nit.

cc: +dtapuska@ FYI, as crbug.com/568183 could clean this up a bit.

+pfeldman@ for:
content/browser/web_contents/web_contents_impl.*
content/public/browser/web_contents_observer.h

https://codereview.chromium.org/1735833002/diff/1/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1735833002/diff/1/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:2102: void DidGetUserInteraction(const
blink::WebInputEvent::Type type) override {
Can we add a test that the type here is sane?

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1909: if
(mouse_wheel_coalesce_timer_->Elapsed().InSecondsF() >
dtapuska@ is working on dispatching gesture events for mousewheel scrolls.
crbug.com/568183.

Once that has landed, we should be able to get rid of the mousewheel timer logic
here, and just listen for GestureScrollBegin.

[dtapuska, 2016-02-25 14:48:58]

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

[dtapuska, 2016-02-25 14:48:58]

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1909: if
(mouse_wheel_coalesce_timer_->Elapsed().InSecondsF() >
On 2016/02/25 14:42:48, tdresser wrote:
> dtapuska@ is working on dispatching gesture events for mousewheel scrolls.
> crbug.com/568183.
> 
> Once that has landed, we should be able to get rid of the mousewheel timer
logic
> here, and just listen for GestureScrollBegin.

Can you please add code for GestureScrollBegin? You can enable the feature via
--enable-wheel-gestures

[Charlie Reis, 2016-02-25 18:28:02]

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

[Charlie Reis, 2016-02-25 18:28:02]

Drive-by content owner review.  :)

LGTM with nits.

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:82: // wheel, raw
key down, gesture tap, or browser navigation) while focus was on
nit: browser-initiated navigation

https://codereview.chromium.org/1735833002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2730:
OnUserInteraction(GetRenderViewHost()->GetWidget(),
Side question: Would you be a good person to chat with about how to update user
gesture/interaction support for out-of-process iframes?  This code only handles
the main frame's widget today, so it will miss interactions in OOPIFs.  I can
follow up and/or file a bug for it separately.  (No need to hold up this CL for
it.)

https://codereview.chromium.org/1735833002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4353: // Ignore when the
renderer is swapped out.
We should probably have a TODO here to support widgets for out-of-process
iframes.

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

https://codereview.chromium.org/1735833002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:326: // 4) any gesture tap event
(blink::WebInputEvent::GestureTapDown).
nit: Update the punctuation now that the list is longer.

[dominickn, 2016-02-26 00:36:11]

Thanks!

https://codereview.chromium.org/1735833002/diff/1/chrome/browser/ui/browser_b...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1735833002/diff/1/chrome/browser/ui/browser_b...
chrome/browser/ui/browser_browsertest.cc:2102: void DidGetUserInteraction(const
blink::WebInputEvent::Type type) override {
On 2016/02/25 14:42:48, tdresser wrote:
> Can we add a test that the type here is sane?

Done.

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_delegate.h:82: // wheel, raw
key down, gesture tap, or browser navigation) while focus was on
On 2016/02/25 18:28:02, Charlie Reis (slow til Mar 7) wrote:
> nit: browser-initiated navigation

Done.

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1909: if
(mouse_wheel_coalesce_timer_->Elapsed().InSecondsF() >
On 2016/02/25 14:48:58, dtapuska wrote:
> On 2016/02/25 14:42:48, tdresser wrote:
> > dtapuska@ is working on dispatching gesture events for mousewheel scrolls.
> > crbug.com/568183.
> > 
> > Once that has landed, we should be able to get rid of the mousewheel timer
> logic
> > here, and just listen for GestureScrollBegin.
> 
> Can you please add code for GestureScrollBegin? You can enable the feature via
> --enable-wheel-gestures
> 

Thanks for the FYI. It looks like GestureScrollBegin fires for tap-scrolls on
Android; scrolls on mobile seem to send GestureTapDown and then
GestureScrollBegin immediately afterwards. That's a bit sad - is there a way to
distinguish between a tap and a scroll on Android?

I'll add code for GestureScrollBegin in a followup CL that also changes the
clients of this method. It will be better to send GestureScrollBegin as the
event type for scrolling rather than MouseWheel since it fires for Android
scrolling as well; since all of the clients of this method explicitly
handle/ignore scroll events in some way, I'll need to update all of them at
once.

https://codereview.chromium.org/1735833002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2730:
OnUserInteraction(GetRenderViewHost()->GetWidget(),
On 2016/02/25 18:28:02, Charlie Reis (slow til Mar 7) wrote:
> Side question: Would you be a good person to chat with about how to update
user
> gesture/interaction support for out-of-process iframes?  This code only
handles
> the main frame's widget today, so it will miss interactions in OOPIFs.  I can
> follow up and/or file a bug for it separately.  (No need to hold up this CL
for
> it.)

I'm by no means an expert on the widget hierarchy, but I'm happy to help out in
any way I can for OOPIF.

https://codereview.chromium.org/1735833002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4353: // Ignore when the
renderer is swapped out.
On 2016/02/25 18:28:02, Charlie Reis (slow til Mar 7) wrote:
> We should probably have a TODO here to support widgets for out-of-process
> iframes.

Done.

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

https://codereview.chromium.org/1735833002/diff/1/content/public/browser/web_...
content/public/browser/web_contents_observer.h:326: // 4) any gesture tap event
(blink::WebInputEvent::GestureTapDown).
On 2016/02/25 18:28:02, Charlie Reis (slow til Mar 7) wrote:
> nit: Update the punctuation now that the list is longer.

Done.

[dominickn, 2016-02-26 00:36:40]

Description was changed from

==========
Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154
==========

to

==========
Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154
==========

[dominickn, 2016-02-26 00:36:40]

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

[dominickn, 2016-02-26 00:37:19]

+calamity: please check site_engagement_helper.cc

+thestig: please check chrome/browser/ui/

[calamity, 2016-02-26 00:54:13]

site_engagement lgtm.

[Lei Zhang, 2016-02-26 02:38:39]

lgtm

https://codereview.chromium.org/1735833002/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/1735833002/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:2105: EXPECT_EQ(type,
blink::WebInputEvent::Undefined);
Reverse the arguments - EXPECT_EQ(expected_val, actual_val);

[tdresser, 2016-02-26 13:14:18]

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:1909: if
(mouse_wheel_coalesce_timer_->Elapsed().InSecondsF() >
On 2016/02/26 00:36:11, dominickn wrote:
> On 2016/02/25 14:48:58, dtapuska wrote:
> > On 2016/02/25 14:42:48, tdresser wrote:
> > > dtapuska@ is working on dispatching gesture events for mousewheel scrolls.
> > > crbug.com/568183.
> > > 
> > > Once that has landed, we should be able to get rid of the mousewheel timer
> > logic
> > > here, and just listen for GestureScrollBegin.
> > 
> > Can you please add code for GestureScrollBegin? You can enable the feature
via
> > --enable-wheel-gestures
> > 
> 
> Thanks for the FYI. It looks like GestureScrollBegin fires for tap-scrolls on
> Android; scrolls on mobile seem to send GestureTapDown and then
> GestureScrollBegin immediately afterwards. That's a bit sad - is there a way
to
> distinguish between a tap and a scroll on Android?
> 
> I'll add code for GestureScrollBegin in a followup CL that also changes the
> clients of this method. It will be better to send GestureScrollBegin as the
> event type for scrolling rather than MouseWheel since it fires for Android
> scrolling as well; since all of the clients of this method explicitly
> handle/ignore scroll events in some way, I'll need to update all of them at
> once.

What do you mean GestureScrollBegin fires for "tap-scrolls"?

GestureTapDown just means that the finger hit the page. GestureTap means that a
tap occurred.

GestureScrollBegin should be fired iff the user starts scrolling.

[dominickn, 2016-02-26 22:16:22]

On 2016/02/26 13:14:18, tdresser wrote:
> What do you mean GestureScrollBegin fires for "tap-scrolls"?
> 
> GestureTapDown just means that the finger hit the page. GestureTap means that
a
> tap occurred.
> 
> GestureScrollBegin should be fired iff the user starts scrolling.

I meant that scrolls on Android will have a double signal - the GestureTapDown
for the tap,
and then GestureScrollBegin when the scroll begins. It would be nice to be able
to avoid this,
but I can live with it - and clients of DidGetUserInteraction will know the type
of event
explicitly.

@pfeldman, @dtapuska, are you okay with me sending this to CQ? I'd like to get
it in
before branch point.

[dtapuska, 2016-02-26 22:37:10]

On 2016/02/26 22:16:22, dominickn wrote:
> On 2016/02/26 13:14:18, tdresser wrote:
> > What do you mean GestureScrollBegin fires for "tap-scrolls"?
> > 
> > GestureTapDown just means that the finger hit the page. GestureTap means
that
> a
> > tap occurred.
> > 
> > GestureScrollBegin should be fired iff the user starts scrolling.
> 
> I meant that scrolls on Android will have a double signal - the GestureTapDown
> for the tap,
> and then GestureScrollBegin when the scroll begins. It would be nice to be
able
> to avoid this,
> but I can live with it - and clients of DidGetUserInteraction will know the
type
> of event
> explicitly.
> 
> @pfeldman, @dtapuska, are you okay with me sending this to CQ? I'd like to get
> it in
> before branch point.

lgtm

[dtapuska, 2016-02-26 22:38:00]

On 2016/02/26 22:37:10, dtapuska wrote:
> On 2016/02/26 22:16:22, dominickn wrote:
> > On 2016/02/26 13:14:18, tdresser wrote:
> > > What do you mean GestureScrollBegin fires for "tap-scrolls"?
> > > 
> > > GestureTapDown just means that the finger hit the page. GestureTap means
> that
> > a
> > > tap occurred.
> > > 
> > > GestureScrollBegin should be fired iff the user starts scrolling.
> > 
> > I meant that scrolls on Android will have a double signal - the
GestureTapDown
> > for the tap,
> > and then GestureScrollBegin when the scroll begins. It would be nice to be
> able
> > to avoid this,
> > but I can live with it - and clients of DidGetUserInteraction will know the
> type
> > of event
> > explicitly.
> > 
> > @pfeldman, @dtapuska, are you okay with me sending this to CQ? I'd like to
get
> > it in
> > before branch point.
> 
> lgtm

You can check the gesture source of the ScrollBegin. It will be touchscreen if
you want to avoid updating in that case.

[dominickn, 2016-02-27 01:25:20]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-02-27 01:25:21]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
tdresser@chromium.org, calamity@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1735833002/#ps40001
(title: "Addressing reviewer comment")

[commit-bot: I haz the power, 2016-02-27 01:33:19]

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

[commit-bot: I haz the power, 2016-02-27 01:53:01]

Description was changed from

==========
Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154
==========

to

==========
Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154
==========

[commit-bot: I haz the power, 2016-02-27 01:53:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-27 01:54:30]

Description was changed from

==========
Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154
==========

to

==========
Remove WebContentsObserver::DidGetUserGesture.

This CL removes DidGetUserGesture and its various hooks in content/. All
existing clients of this method have been migrated to
WebContentsObserver::DidGetUserInteraction.

This is a follow up to https://crrev.com/1388293002, which implemented
WebContentsObserver::DidGetUserInteraction. Compared to
DidGetUserGesture, DidGetUserInteraction has a simpler implementation in
content/, and signals to clients the type of user interaction which
triggered it. DidGetUserGesture was intended to signal the types of
gestures which would indicate that the user was attempting to trigger an
action (Enter, Space, mouse click, gesture tap). However, gesture
signals are always noisy; the download request limiter, external
protocol handler, and resource dispatch host (which consumed
DidGetUserGesture) treated the gesture as a heuristic, rather than as
a canonical indication of a user's intent. In general, it is impossible
to know whether any of the user inputs were actually intended to trigger
an action. This makes DidGetUserGesture suitable *only* as a heuristic.

DidGetUserInteraction is intended as a complete replacement for
DidGetUserGesture, except it triggers on all keypress events rather than
only Enter and Space. This expansion is appropriate given the
aforementioned heuristic-nature of this signal.

BUG=584154

Committed: https://crrev.com/9c7ceaa3b757e38ebf6ac5b59516ff54c8f786b6
Cr-Commit-Position: refs/heads/master@{#378071}
==========

[commit-bot: I haz the power, 2016-02-27 01:54:32]

Patchset 3 (id:??) landed as
https://crrev.com/9c7ceaa3b757e38ebf6ac5b59516ff54c8f786b6
Cr-Commit-Position: refs/heads/master@{#378071}