Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407053010:
Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. (Closed)
|
Created:
5 years, 1 month ago by dominickn Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, asvitkine+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. |
DescriptionMake mouse wheel events trigger WebContentsObserver::DidGetUserInteraction.
DidGetUserInteraction() is called for mouse clicks, keypresses, and
gesture taps. This CL additionally calls it for mouse wheel events.
Impact on scrolling performance is minimised by calling
DidGetUserInteraction only at the start of a series of mouse wheel
events; any further events which occur less than 0.1 seconds after a
previous mouse wheel do not trigger the callback.
This CL also allows the site engagement service to correctly log and
register mouse wheel events.
BUG=548011, 464234
Committed: https://crrev.com/13ce0f2bf5ad77c9fbb272bd9f3736cae4a80d0b
Cr-Commit-Position: refs/heads/master@{#357963}
Committed: https://crrev.com/ad9c99d7468a6391370f753bedb4c440150b022a
Cr-Commit-Position: refs/heads/master@{#358226}
Patch Set 1 #Patch Set 2 : Adding test #Patch Set 3 : More tests #
Total comments: 2
Patch Set 4 : Addressing nits #
Total comments: 18
Patch Set 5 : Addressing reviewer comments #Patch Set 6 : Addressing nit #
Total comments: 4
Patch Set 7 : Addressing nits #Patch Set 8 : Removing static initializer #Messages
Total messages: 38 (14 generated)
Description was changed from ========== Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. DidGetUserInteraction() is called for mouse clicks, keypresses, and gesture taps. This CL additionally calls it for mouse wheel events. Impact on scrolling performance is minimised by calling DidGetUserInteraction only at the start of a series of mouse wheel events; any further events which occur less than 0.1 seconds after a previous mouse wheel do not trigger the callback. This CL also allows the site engagement service to correctly log and register mouse wheel events. BUG=548001 ========== to ========== Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. DidGetUserInteraction() is called for mouse clicks, keypresses, and gesture taps. This CL additionally calls it for mouse wheel events. Impact on scrolling performance is minimised by calling DidGetUserInteraction only at the start of a series of mouse wheel events; any further events which occur less than 0.1 seconds after a previous mouse wheel do not trigger the callback. This CL also allows the site engagement service to correctly log and register mouse wheel events. BUG=548001 ==========
dominickn@chromium.org changed reviewers: + tdresser@chromium.org
dominickn@chromium.org changed reviewers: + calamity@chromium.org
@tdresser. please review content/ @calamity: please review site engagement Thanks!
Patchset #3 (id:40001) has been deleted
LGTM with nits. https://codereview.chromium.org/1407053010/diff/60001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1407053010/diff/60001/content/public/browser/... content/public/browser/web_contents_observer.h:323: // wheel scroll; 3) any raw key down event; and 3) any gesture tap event It might be worth putting each item on it's own line in this list. Also, replace "3) any gesture" -> "4) any gesture".
dominickn@chromium.org changed reviewers: + nasko@chromium.org
Thanks! @nasko: please review web_contents_observer. https://codereview.chromium.org/1407053010/diff/60001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1407053010/diff/60001/content/public/browser/... content/public/browser/web_contents_observer.h:323: // wheel scroll; 3) any raw key down event; and 3) any gesture tap event On 2015/11/02 15:02:10, tdresser wrote: > It might be worth putting each item on it's own line in this list. > > Also, replace "3) any gesture" -> "4) any gesture". Done.
dominickn@chromium.org changed reviewers: + isherman@chromium.org
isherman: please review histograms. Thanks!
histograms.xml lgtm
site_engagement lgtm. https://codereview.chromium.org/1407053010/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1407053010/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Check your bug number. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:98: const base::TimeDelta g_wheel_coalesce_interval = nit: Consider kWheelCoalesceInterval if this is never changed.
Description was changed from ========== Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. DidGetUserInteraction() is called for mouse clicks, keypresses, and gesture taps. This CL additionally calls it for mouse wheel events. Impact on scrolling performance is minimised by calling DidGetUserInteraction only at the start of a series of mouse wheel events; any further events which occur less than 0.1 seconds after a previous mouse wheel do not trigger the callback. This CL also allows the site engagement service to correctly log and register mouse wheel events. BUG=548001 ========== to ========== Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. DidGetUserInteraction() is called for mouse clicks, keypresses, and gesture taps. This CL additionally calls it for mouse wheel events. Impact on scrolling performance is minimised by calling DidGetUserInteraction only at the start of a series of mouse wheel events; any further events which occur less than 0.1 seconds after a previous mouse wheel do not trigger the callback. This CL also allows the site engagement service to correctly log and register mouse wheel events. BUG=548011,464234 ==========
https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:99: base::TimeDelta::FromMilliseconds(100); What is the rationale for picking this number? https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:213: wheel_coalesce_timer_(nullptr), scoped_ptr is default initialized to nullptr, so no need for explicit initialization here. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1875: wheel_coalesce_timer_.reset(new base::ElapsedTimer()); If the timer will always be reset to something, why not initialize it to a valid value in the constructor? This will allow you to drop the check for existence in the if statement above and make it trivial to read. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:859: scoped_ptr<base::ElapsedTimer> wheel_coalesce_timer_; nit: Add a "mouse_" prefix. https://codereview.chromium.org/1407053010/diff/80001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1407053010/diff/80001/content/public/browser/... content/public/browser/web_contents_observer.h:325: // 2) the start of a mouse wheel scroll (blink::WebInputEvent::MouseWheel); It isn't really the start, is it? It is the first even in an interval. If I scroll really slowly, such that each event is delivered right after the time elapsed, wouldn't it result in another event fired?
Thanks! https://codereview.chromium.org/1407053010/diff/80001/chrome/browser/engageme... File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1407053010/diff/80001/chrome/browser/engageme... chrome/browser/engagement/site_engagement_helper.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/11/03 02:16:13, calamity wrote: > Check your bug number. Done. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:98: const base::TimeDelta g_wheel_coalesce_interval = On 2015/11/03 02:16:13, calamity wrote: > nit: Consider kWheelCoalesceInterval if this is never changed. I'll leave this unless one of the content reviewers wants it. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:99: base::TimeDelta::FromMilliseconds(100); On 2015/11/03 20:45:59, nasko (slow to review) wrote: > What is the rationale for picking this number? It's the same value used by blink in its orthogonal heuristic to detect the end of a scroll event (if no mouse wheel has been seen for 0.1 seconds, send end scroll). https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:213: wheel_coalesce_timer_(nullptr), On 2015/11/03 20:45:59, nasko (slow to review) wrote: > scoped_ptr is default initialized to nullptr, so no need for explicit > initialization here. Done. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1875: wheel_coalesce_timer_.reset(new base::ElapsedTimer()); On 2015/11/03 20:45:59, nasko (slow to review) wrote: > If the timer will always be reset to something, why not initialize it to a valid > value in the constructor? This will allow you to drop the check for existence in > the if statement above and make it trivial to read. Done. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.h:859: scoped_ptr<base::ElapsedTimer> wheel_coalesce_timer_; On 2015/11/03 20:45:59, nasko (slow to review) wrote: > nit: Add a "mouse_" prefix. Done. https://codereview.chromium.org/1407053010/diff/80001/content/public/browser/... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1407053010/diff/80001/content/public/browser/... content/public/browser/web_contents_observer.h:325: // 2) the start of a mouse wheel scroll (blink::WebInputEvent::MouseWheel); On 2015/11/03 20:45:59, nasko (slow to review) wrote: > It isn't really the start, is it? It is the first even in an interval. If I > scroll really slowly, such that each event is delivered right after the time > elapsed, wouldn't it result in another event fired? Yes. I've updated this comment to match.
https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:98: const base::TimeDelta g_wheel_coalesce_interval = On 2015/11/03 21:29:42, dominickn wrote: > On 2015/11/03 02:16:13, calamity wrote: > > nit: Consider kWheelCoalesceInterval if this is never changed. > > I'll leave this unless one of the content reviewers wants it. I think kWheelCoalesceInterval would be more standard.
Thanks! https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:98: const base::TimeDelta g_wheel_coalesce_interval = On 2015/11/03 21:39:48, tdresser wrote: > On 2015/11/03 21:29:42, dominickn wrote: > > On 2015/11/03 02:16:13, calamity wrote: > > > nit: Consider kWheelCoalesceInterval if this is never changed. > > > > I'll leave this unless one of the content reviewers wants it. > > I think kWheelCoalesceInterval would be more standard. Done.
Few more nits, otherwise it looks good. https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:99: base::TimeDelta::FromMilliseconds(100); On 2015/11/03 21:29:42, dominickn wrote: > On 2015/11/03 20:45:59, nasko (slow to review) wrote: > > What is the rationale for picking this number? > > It's the same value used by blink in its orthogonal heuristic to detect the end > of a scroll event (if no mouse wheel has been seen for 0.1 seconds, send end > scroll). Please put this in a comment, so it is clear for people reading the code. https://codereview.chromium.org/1407053010/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1407053010/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:858: // event has not been seen for kWheelCoalesceInterval seconds prior. Please add Mouse prefix everywhere there is only wheel. We should be precise. https://codereview.chromium.org/1407053010/diff/120001/content/public/browser... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1407053010/diff/120001/content/public/browser... content/public/browser/web_contents_observer.h:331: // blink to detect the end of scrolls. nit: Blink
Thanks! https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1407053010/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:99: base::TimeDelta::FromMilliseconds(100); On 2015/11/04 17:31:22, nasko (slow to review) wrote: > On 2015/11/03 21:29:42, dominickn wrote: > > On 2015/11/03 20:45:59, nasko (slow to review) wrote: > > > What is the rationale for picking this number? > > > > It's the same value used by blink in its orthogonal heuristic to detect the > end > > of a scroll event (if no mouse wheel has been seen for 0.1 seconds, send end > > scroll). > > Please put this in a comment, so it is clear for people reading the code. Done, expanded the preceding comment. https://codereview.chromium.org/1407053010/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1407053010/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.h:858: // event has not been seen for kWheelCoalesceInterval seconds prior. On 2015/11/04 17:31:23, nasko (slow to review) wrote: > Please add Mouse prefix everywhere there is only wheel. We should be precise. Done. https://codereview.chromium.org/1407053010/diff/120001/content/public/browser... File content/public/browser/web_contents_observer.h (right): https://codereview.chromium.org/1407053010/diff/120001/content/public/browser... content/public/browser/web_contents_observer.h:331: // blink to detect the end of scrolls. On 2015/11/04 17:31:23, nasko (slow to review) wrote: > nit: Blink Done.
LGTM
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, calamity@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1407053010/#ps140001 (title: "Addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407053010/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407053010/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/13ce0f2bf5ad77c9fbb272bd9f3736cae4a80d0b Cr-Commit-Position: refs/heads/master@{#357963}
Message was sent while issue was closed.
Description was changed from ========== Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. DidGetUserInteraction() is called for mouse clicks, keypresses, and gesture taps. This CL additionally calls it for mouse wheel events. Impact on scrolling performance is minimised by calling DidGetUserInteraction only at the start of a series of mouse wheel events; any further events which occur less than 0.1 seconds after a previous mouse wheel do not trigger the callback. This CL also allows the site engagement service to correctly log and register mouse wheel events. BUG=548011,464234 Committed: https://crrev.com/13ce0f2bf5ad77c9fbb272bd9f3736cae4a80d0b Cr-Commit-Position: refs/heads/master@{#357963} ========== to ========== Make mouse wheel events trigger WebContentsObserver::DidGetUserInteraction. DidGetUserInteraction() is called for mouse clicks, keypresses, and gesture taps. This CL additionally calls it for mouse wheel events. Impact on scrolling performance is minimised by calling DidGetUserInteraction only at the start of a series of mouse wheel events; any further events which occur less than 0.1 seconds after a previous mouse wheel do not trigger the callback. This CL also allows the site engagement service to correctly log and register mouse wheel events. BUG=548011,464234 Committed: https://crrev.com/13ce0f2bf5ad77c9fbb272bd9f3736cae4a80d0b Cr-Commit-Position: refs/heads/master@{#357963} ==========
CL reverted because of an unpermitted static initializer in render_widget_host_impl.cc. The latest patchset simply swaps the static TimeDelta constant for a double constant, and calls InSecondsF on the timer to suit. Everything else is unchanged. @tdresser or @nasko: PTAL at render_widget_host_impl.cc. Thanks!
On 2015/11/05 12:19:31, dominickn wrote: > CL reverted because of an unpermitted static initializer in > render_widget_host_impl.cc. The latest patchset simply swaps the static > TimeDelta constant for a double constant, and calls InSecondsF on the timer to > suit. Everything else is unchanged. > > @tdresser or @nasko: PTAL at render_widget_host_impl.cc. Thanks! render_widget_host_impl.cc LGTM.
On 2015/11/05 12:58:11, tdresser wrote: > On 2015/11/05 12:19:31, dominickn wrote: > > CL reverted because of an unpermitted static initializer in > > render_widget_host_impl.cc. The latest patchset simply swaps the static > > TimeDelta constant for a double constant, and calls InSecondsF on the timer to > > suit. Everything else is unchanged. > > > > @tdresser or @nasko: PTAL at render_widget_host_impl.cc. Thanks! > > render_widget_host_impl.cc LGTM. In the future, please use a new CL for relanding a change. Upload the first patch to be equivalent to what was reverted and then upload the fixes in another patch. This way it is clear what has changed and the existing CL is not reused. For this one, LGTM.
The CQ bit was checked by dominickn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from calamity@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1407053010/#ps160001 (title: "Removing static initializer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407053010/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407053010/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dominickn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407053010/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407053010/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ad9c99d7468a6391370f753bedb4c440150b022a Cr-Commit-Position: refs/heads/master@{#358226} |
