Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1474443002:
Add UMA metrics to determine the usefulness of passive event listeners. (Closed)
|
Created:
5 years, 1 month ago by dtapuska Modified:
5 years ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master_passive_uma Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA metrics to determine the usefulness of passive event listeners.
The UMA metrics track whether an event was passive (not yet implemented),
uncancelable, cancelable&canceled, cancelable¬ canceled.
UMA metrics are calculated for TouchStart,Move,End and MouseWheel
listeners.
For items that are cancelable¬ canceled; the latency info is
computed and logged; this of course is captured in the render_widget
so it doesn't account for the full round trip time but more or less the
queuing delay.
BUG=543611
Committed: https://crrev.com/e747361b3877af48ebb20ec6e7999c2120d4a16a
Cr-Commit-Position: refs/heads/master@{#363201}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fix nits #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Fix time calculation #
Depends on Patchset: Messages
Total messages: 28 (13 generated)
Description was changed from ========== Add UMA metrics to determine the usefulness of passive event listeners. The UMA metrics track whether an event was passive (not yet implemented), uncancelable, cancelable&canceled, cancelable¬ canceled. UMA metrics are calculated for TouchStart,Move,End and MouseWheel listeners. For items that are cancelable¬ canceled; the latency info is computed and logged; this of course is captured in the render_widget so it doesn't account for the full round trip time but more or less the queuing delay. BUG=543611 ========== to ========== Add UMA metrics to determine the usefulness of passive event listeners. The UMA metrics track whether an event was passive (not yet implemented), uncancelable, cancelable&canceled, cancelable¬ canceled. UMA metrics are calculated for TouchStart,Move,End and MouseWheel listeners. For items that are cancelable¬ canceled; the latency info is computed and logged; this of course is captured in the render_widget so it doesn't account for the full round trip time but more or less the queuing delay. BUG=543611 ==========
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
LGTM with nits. https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1136: bool passive = false; Add a comment linking to the bug indicating that this is not yet implemented. https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1233: input_event->type == WebInputEvent::TouchEnd) { Does isTouchEventType work for you? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1239: LogPassiveEventListenersUma(processed, passive, !passive, Does this make it more difficult to draw conclusions from the cancelable vs non-cancelable entries in the histogram? I'm tempted to say we should actually split up touch vs mousewheel here, but maybe that's overkill. Your call, but I'm a bit worried that the way in which |cancellable| is defined differently for wheel will cause confusion. https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1349: processed != WebInputEventResult::NotHandled) I'm a fan of braces when the condition is multi-line, and I think that's consistent with the style around here. https://codereview.chromium.org/1474443002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1474443002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:10713: +<histogram name="Event.PassiveListeners" enum="EventResultType"> I'm not sure I've seen this pattern before, where one histogram name is the prefix of a different histogram name. It might make sense to switch this to Event.PassiveListeners.Result. The histograms.xml reviewer will probably know whether that would be preferable. https://codereview.chromium.org/1474443002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:10725: + yet no action was executed for the event. This histogram tracks the "yet" -> "for cases when" "no action was executed" -> "preventDefault wasn't called"?
https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1233: input_event->type == WebInputEvent::TouchEnd) { On 2015/11/23 21:14:34, tdresser wrote: > Does isTouchEventType work for you? > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I didn't necessarily want to capture TouchCancel since that isn't an event we'd wait for anyways. https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1239: LogPassiveEventListenersUma(processed, passive, !passive, On 2015/11/23 21:14:34, tdresser wrote: > Does this make it more difficult to draw conclusions from the cancelable vs > non-cancelable entries in the histogram? > > I'm tempted to say we should actually split up touch vs mousewheel here, but > maybe that's overkill. > > Your call, but I'm a bit worried that the way in which |cancellable| is defined > differently for wheel will cause confusion. UMA would only record a non-cancelable event for touch; its not possible for it to log one for mouse wheel.. And those aren't really useful. The real useful ones are cancelable and not canceled metric. But then again we can't possibly draw too much conclusions as to those other than the portion of events that are processed that way because the results can be dependent on scripts choosing behavior certain ways. https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1349: processed != WebInputEventResult::NotHandled) On 2015/11/23 21:14:34, tdresser wrote: > I'm a fan of braces when the condition is multi-line, and I think that's > consistent with the style around here. woops; that is me not checking git cl format.
https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1136: bool passive = false; On 2015/11/23 21:14:34, tdresser wrote: > Add a comment linking to the bug indicating that this is not yet implemented. Done. https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:1349: processed != WebInputEventResult::NotHandled) On 2015/11/23 21:33:57, dtapuska wrote: > On 2015/11/23 21:14:34, tdresser wrote: > > I'm a fan of braces when the condition is multi-line, and I think that's > > consistent with the style around here. > > woops; that is me not checking git cl format. Done.
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org
On 2015/11/27 20:05:25, dtapuska wrote: > https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... > content/renderer/render_widget.cc:1136: bool passive = false; > On 2015/11/23 21:14:34, tdresser wrote: > > Add a comment linking to the bug indicating that this is not yet implemented. > > Done. > > https://codereview.chromium.org/1474443002/diff/1/content/renderer/render_wid... > content/renderer/render_widget.cc:1349: processed != > WebInputEventResult::NotHandled) > On 2015/11/23 21:33:57, dtapuska wrote: > > On 2015/11/23 21:14:34, tdresser wrote: > > > I'm a fan of braces when the condition is multi-line, and I think that's > > > consistent with the style around here. > > > > woops; that is me not checking git cl format. > > Done. Rick here is what we were discussing earlier today.
dtapuska@chromium.org changed reviewers: + isherman@chromium.org
LGTM, thanks for the fix and additional TODO.
histograms lgtm
The CQ bit was checked by dtapuska@chromium.org
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/1474443002/#ps80001 (title: "Fix time calculation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474443002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474443002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474443002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474443002/80001
dtapuska@chromium.org changed reviewers: + jochen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474443002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474443002/80001
Message was sent while issue was closed.
Description was changed from ========== Add UMA metrics to determine the usefulness of passive event listeners. The UMA metrics track whether an event was passive (not yet implemented), uncancelable, cancelable&canceled, cancelable¬ canceled. UMA metrics are calculated for TouchStart,Move,End and MouseWheel listeners. For items that are cancelable¬ canceled; the latency info is computed and logged; this of course is captured in the render_widget so it doesn't account for the full round trip time but more or less the queuing delay. BUG=543611 ========== to ========== Add UMA metrics to determine the usefulness of passive event listeners. The UMA metrics track whether an event was passive (not yet implemented), uncancelable, cancelable&canceled, cancelable¬ canceled. UMA metrics are calculated for TouchStart,Move,End and MouseWheel listeners. For items that are cancelable¬ canceled; the latency info is computed and logged; this of course is captured in the render_widget so it doesn't account for the full round trip time but more or less the queuing delay. BUG=543611 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add UMA metrics to determine the usefulness of passive event listeners. The UMA metrics track whether an event was passive (not yet implemented), uncancelable, cancelable&canceled, cancelable¬ canceled. UMA metrics are calculated for TouchStart,Move,End and MouseWheel listeners. For items that are cancelable¬ canceled; the latency info is computed and logged; this of course is captured in the render_widget so it doesn't account for the full round trip time but more or less the queuing delay. BUG=543611 ========== to ========== Add UMA metrics to determine the usefulness of passive event listeners. The UMA metrics track whether an event was passive (not yet implemented), uncancelable, cancelable&canceled, cancelable¬ canceled. UMA metrics are calculated for TouchStart,Move,End and MouseWheel listeners. For items that are cancelable¬ canceled; the latency info is computed and logged; this of course is captured in the render_widget so it doesn't account for the full round trip time but more or less the queuing delay. BUG=543611 Committed: https://crrev.com/e747361b3877af48ebb20ec6e7999c2120d4a16a Cr-Commit-Position: refs/heads/master@{#363201} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e747361b3877af48ebb20ec6e7999c2120d4a16a Cr-Commit-Position: refs/heads/master@{#363201} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
