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

Issue 1748553002: Replace MouseWheel events with GestureScrollBegin in WebContentsObserver::DidGetUserInteraction (Closed)

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

Description

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}

Patch Set 1 #

Total comments: 9

Patch Set 2 : Addressing reviewer comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -56 lines) Patch
M chrome/browser/download/download_request_limiter.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/download/download_request_limiter_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/engagement/site_engagement_helper.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/engagement/site_engagement_helper_unittest.cc View 3 chunks +5 lines, -4 lines 0 comments Download
M chrome/browser/engagement/site_engagement_metrics.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/engagement/site_engagement_service_unittest.cc View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/external_protocol/external_protocol_observer.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_delegate.h View 1 chunk +3 lines, -4 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 chunk +0 lines, -6 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 3 chunks +5 lines, -23 lines 0 comments Download
M content/public/browser/web_contents_observer.h View 1 1 chunk +1 line, -6 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 1 chunk +3 lines, -3 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 21 (8 generated)
dominickn
It is intended that this will land once crbug.com/568183 is finished. asanka: please review chrome/browser/download ...
3 years, 4 months ago (2016-02-29 04:24:24 UTC) #2
tdresser
content/ LGTM https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/download_request_limiter.cc File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/download_request_limiter.cc#newcode102 chrome/browser/download/download_request_limiter.cc:102: if (is_showing_prompt() || type == blink::WebInputEvent::GestureScrollBegin) { ...
3 years, 4 months ago (2016-02-29 14:14:12 UTC) #3
dtapuska
On 2016/02/29 14:14:12, tdresser wrote: > content/ LGTM > > https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/download_request_limiter.cc > File chrome/browser/download/download_request_limiter.cc (right): ...
3 years, 4 months ago (2016-02-29 14:49:00 UTC) #4
dominickn
Thanks! On 2016/02/29 14:49:00, dtapuska wrote: > So mouse wheel gestures is conditionally enabled right ...
3 years, 4 months ago (2016-02-29 22:52:02 UTC) #5
dominickn
jochen: PTAL at chrome/browser/external_protocol. Thanks!
3 years, 4 months ago (2016-02-29 22:58:15 UTC) #8
jochen (gone - plz use gerrit)
c/b/external_protocol lgtm
3 years, 4 months ago (2016-03-01 14:38:27 UTC) #9
asanka
c/b/download lgtm
3 years, 4 months ago (2016-03-01 14:54:10 UTC) #10
tdresser
https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/download_request_limiter.cc File chrome/browser/download/download_request_limiter.cc (right): https://codereview.chromium.org/1748553002/diff/1/chrome/browser/download/download_request_limiter.cc#newcode102 chrome/browser/download/download_request_limiter.cc:102: if (is_showing_prompt() || type == blink::WebInputEvent::GestureScrollBegin) { On 2016/02/29 ...
3 years, 4 months ago (2016-03-01 15:12:10 UTC) #11
dominickn
On 2016/03/01 15:12:10, tdresser wrote: > Sorry, I missed that touch gesture events aren't being ...
3 years, 4 months ago (2016-03-01 22:56:36 UTC) #13
Ilya Sherman
histograms.xml lgtm
3 years, 4 months ago (2016-03-02 01:00:11 UTC) #14
commit-bot: I haz the power
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
3 years, 4 months ago (2016-03-25 01:11:19 UTC) #17
commit-bot: I haz the power
Committed patchset #2 (id:20001)
3 years, 4 months ago (2016-03-25 03:06:32 UTC) #19
commit-bot: I haz the power
3 years, 4 months ago (2016-03-25 03:08:07 UTC) #21
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/5cc29a417bcb7b86ca8ebc4304eefa4d74a7e919
Cr-Commit-Position: refs/heads/master@{#383231}

Powered by Google App Engine
This is Rietveld 408576698