Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(31)

Issue 1735833002: Remove WebContentsObserver::DidGetUserGesture. (Closed)

Created:
3 years, 4 months ago by dominickn
Modified:
3 years, 4 months ago
CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 14

Patch Set 2 : Addressing reviewer comments #

Total comments: 1

Patch Set 3 : Addressing reviewer comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -44 lines) Patch
M chrome/browser/engagement/site_engagement_helper.cc View 1 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_browsertest.cc View 1 2 1 chunk +6 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_delegate.h View 1 1 chunk +6 lines, -9 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 chunks +4 lines, -15 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 chunks +12 lines, -10 lines 0 comments Download
M content/public/browser/web_contents_observer.h View 1 1 chunk +4 lines, -7 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 25 (10 generated)
dominickn
PTAL, and recommend a content/ owner for review as nasko@ is currently away. Thanks!
3 years, 4 months ago (2016-02-25 04:38:48 UTC) #2
tdresser
LGTM with nit. cc: +dtapuska@ FYI, as crbug.com/568183 could clean this up a bit. +pfeldman@ ...
3 years, 4 months ago (2016-02-25 14:42:48 UTC) #4
dtapuska
https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_host/render_widget_host_impl.cc#newcode1909 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: > ...
3 years, 4 months ago (2016-02-25 14:48:58 UTC) #6
Charlie Reis
Drive-by content owner review. :) LGTM with nits. https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_host/render_widget_host_delegate.h File content/browser/renderer_host/render_widget_host_delegate.h (right): https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_host/render_widget_host_delegate.h#newcode82 content/browser/renderer_host/render_widget_host_delegate.h:82: // ...
3 years, 4 months ago (2016-02-25 18:28:02 UTC) #8
dominickn
Thanks! https://codereview.chromium.org/1735833002/diff/1/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1735833002/diff/1/chrome/browser/ui/browser_browsertest.cc#newcode2102 chrome/browser/ui/browser_browsertest.cc:2102: void DidGetUserInteraction(const blink::WebInputEvent::Type type) override { On 2016/02/25 ...
3 years, 4 months ago (2016-02-26 00:36:11 UTC) #9
dominickn
+calamity: please check site_engagement_helper.cc +thestig: please check chrome/browser/ui/
3 years, 4 months ago (2016-02-26 00:37:19 UTC) #12
calamity
site_engagement lgtm.
3 years, 4 months ago (2016-02-26 00:54:13 UTC) #13
Lei Zhang
lgtm https://codereview.chromium.org/1735833002/diff/20001/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (right): https://codereview.chromium.org/1735833002/diff/20001/chrome/browser/ui/browser_browsertest.cc#newcode2105 chrome/browser/ui/browser_browsertest.cc:2105: EXPECT_EQ(type, blink::WebInputEvent::Undefined); Reverse the arguments - EXPECT_EQ(expected_val, actual_val);
3 years, 4 months ago (2016-02-26 02:38:39 UTC) #14
tdresser
https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1735833002/diff/1/content/browser/renderer_host/render_widget_host_impl.cc#newcode1909 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: > ...
3 years, 4 months ago (2016-02-26 13:14:18 UTC) #15
dominickn
On 2016/02/26 13:14:18, tdresser wrote: > What do you mean GestureScrollBegin fires for "tap-scrolls"? > ...
3 years, 4 months ago (2016-02-26 22:16:22 UTC) #16
dtapuska
On 2016/02/26 22:16:22, dominickn wrote: > On 2016/02/26 13:14:18, tdresser wrote: > > What do ...
3 years, 4 months ago (2016-02-26 22:37:10 UTC) #17
dtapuska
On 2016/02/26 22:37:10, dtapuska wrote: > On 2016/02/26 22:16:22, dominickn wrote: > > On 2016/02/26 ...
3 years, 4 months ago (2016-02-26 22:38:00 UTC) #18
commit-bot: I haz the power
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
3 years, 4 months ago (2016-02-27 01:33:19 UTC) #21
commit-bot: I haz the power
Committed patchset #3 (id:40001)
3 years, 4 months ago (2016-02-27 01:53:02 UTC) #23
commit-bot: I haz the power
3 years, 4 months ago (2016-02-27 01:54:32 UTC) #25
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/9c7ceaa3b757e38ebf6ac5b59516ff54c8f786b6
Cr-Commit-Position: refs/heads/master@{#378071}

Powered by Google App Engine
This is Rietveld 408576698