Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270173002:
add stylus metrics (Closed)
|
Created:
4 years, 4 months ago by xiaoyinh(OOO Sep 11-29) Modified:
4 years, 3 months ago CC:
chromium-reviews, kalyank, sadrul, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRe-submit: Adding the following stylus metrics:
1. Number of “down” events generated by stylus/touch/mouse
2. Number of “down” events received by clamshell vs touchview
BUG=630070
Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832
Committed: https://crrev.com/06e3125412bb55231c6d26b20f5dbcdbdc1e8583
Cr-Original-Commit-Position: refs/heads/master@{#414304}
Cr-Commit-Position: refs/heads/master@{#414574}
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Patch Set 1 #
Total comments: 13
Patch Set 2 : incorporate comments and rebase #
Total comments: 8
Patch Set 3 : incorporate comments from jdufault@ #
Total comments: 4
Patch Set 4 : incoporate comments and rebase #Patch Set 5 : rebase and add switch case for POINTER_TYPE_ERASER #Patch Set 6 : Incorporate A11y review comments #
Messages
Total messages: 52 (31 generated)
Description was changed from ========== add stylus metrics BUG=630070 ========== to ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 ==========
xiaoyinh@chromium.org changed reviewers: + isherman@chromium.org, jdufault@chromium.org, oshima@chromium.org
isherman@, could you take a look at histograms.xml oshima@, jdufault@, could you take a look at changes in ash/ I'm not sure if ash/wm is a good place to add the new files since the stylus metrics will be used for chromeos only. Could you advice? Thanks!
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:28: }; For each of these enums, please add a brief comment documenting that the enum is used to back an UMA histogram, and therefore should be treated as append-only. https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:57: } I'd recommend writing this to reduce repetition, like so: DownEventFormFactor form_factor; if (...) { form_factor = DOWN_EVENT_FORMFACTOR_TOUCHVIEW; } else { form_factor = DOWN_EVENT_FORMFACTOR_CLAMSHELL; } UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerFormFactor", form_factor, DOWN_EVENT_FORMFACTOR_COUNT); Ditto for the enum below. https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:75: default: Please handle all possible cases explicitly, so that the compiler will warn if there are ever additional point types added in the future. I'd recommend having an UNKNOWN bucket in the histogram, to match the C++ enum. And, as in the C++ enum, I'd make that bucket be the first one. That way, if additional types are ever added, it won't end up trapped in the middle.
https://codereview.chromium.org/2270173002/diff/1/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2270173002/diff/1/ash/ash.gyp#newcode776 ash/ash.gyp:776: 'wm/stylus_metrics_recorder.h', if these are for cros, please exclude them on other platforms https://codereview.chromium.org/2270173002/diff/1/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2270173002/diff/1/ash/shell.cc#newcode651 ash/shell.cc:651: #endif // defined(OS_CHROMEOS) Please put then in the single OS_CHROMEOS. (It probably because of some refactoring) https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:57: } On 2016/08/23 20:50:29, Ilya Sherman wrote: > I'd recommend writing this to reduce repetition, like so: > > DownEventFormFactor form_factor; > if (...) { > form_factor = DOWN_EVENT_FORMFACTOR_TOUCHVIEW; > } else { > form_factor = DOWN_EVENT_FORMFACTOR_CLAMSHELL; > } > UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerFormFactor", > form_factor, > DOWN_EVENT_FORMFACTOR_COUNT); > > Ditto for the enum below. IIRC, it has to be that way because this macro creates a static object? https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... File ash/wm/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.h:21: // Overridden from ui::EventHandler: nit: // ui::EventHandler:
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2270173002/diff/1/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2270173002/diff/1/ash/ash.gyp#newcode776 ash/ash.gyp:776: 'wm/stylus_metrics_recorder.h', On 2016/08/23 21:40:57, oshima wrote: > if these are for cros, please exclude them on other platforms Done. https://codereview.chromium.org/2270173002/diff/1/ash/shell.cc File ash/shell.cc (right): https://codereview.chromium.org/2270173002/diff/1/ash/shell.cc#newcode651 ash/shell.cc:651: #endif // defined(OS_CHROMEOS) On 2016/08/23 21:40:57, oshima wrote: > Please put then in the single OS_CHROMEOS. (It probably because of some > refactoring) I have grouped the neighbors into single OS_CHROMEOS. Thanks! https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:28: }; On 2016/08/23 20:50:29, Ilya Sherman wrote: > For each of these enums, please add a brief comment documenting that the enum is > used to back an UMA histogram, and therefore should be treated as append-only. Done. https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:57: } On 2016/08/23 20:50:29, Ilya Sherman wrote: > I'd recommend writing this to reduce repetition, like so: > > DownEventFormFactor form_factor; > if (...) { > form_factor = DOWN_EVENT_FORMFACTOR_TOUCHVIEW; > } else { > form_factor = DOWN_EVENT_FORMFACTOR_CLAMSHELL; > } > UMA_HISTOGRAM_ENUMERATION("Event.DownEventCount.PerFormFactor", > form_factor, > DOWN_EVENT_FORMFACTOR_COUNT); > > Ditto for the enum below. Done. https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.cc:75: default: On 2016/08/23 20:50:29, Ilya Sherman wrote: > Please handle all possible cases explicitly, so that the compiler will warn if > there are ever additional point types added in the future. I'd recommend having > an UNKNOWN bucket in the histogram, to match the C++ enum. And, as in the C++ > enum, I'd make that bucket be the first one. That way, if additional types are > ever added, it won't end up trapped in the middle. Done. https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... File ash/wm/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2270173002/diff/1/ash/wm/stylus_metrics_recor... ash/wm/stylus_metrics_recorder.h:21: // Overridden from ui::EventHandler: On 2016/08/23 21:40:57, oshima wrote: > nit: // ui::EventHandler: Done.
https://codereview.chromium.org/2270173002/diff/20001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2270173002/diff/20001/ash/ash.gyp#newcode1196 ash/ash.gyp:1196: 'wm/stylus_metrics_recorder.cc', Use spaces instead of tabs. https://codereview.chromium.org/2270173002/diff/20001/ash/wm/stylus_metrics_r... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/20001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.cc:17: // Form factor of the down event. Continue comment on the same line. https://codereview.chromium.org/2270173002/diff/20001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.cc:26: // Input type of the down event. Continue comment on the same line. https://codereview.chromium.org/2270173002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2270173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:73970: + <int value="2" label="stylus"/> Stylus, Touch (note capitalization)?
https://codereview.chromium.org/2270173002/diff/20001/ash/ash.gyp File ash/ash.gyp (right): https://codereview.chromium.org/2270173002/diff/20001/ash/ash.gyp#newcode1196 ash/ash.gyp:1196: 'wm/stylus_metrics_recorder.cc', On 2016/08/23 23:09:53, jdufault wrote: > Use spaces instead of tabs. Done. https://codereview.chromium.org/2270173002/diff/20001/ash/wm/stylus_metrics_r... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/20001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.cc:17: // Form factor of the down event. On 2016/08/23 23:09:53, jdufault wrote: > Continue comment on the same line. Done. https://codereview.chromium.org/2270173002/diff/20001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.cc:26: // Input type of the down event. On 2016/08/23 23:09:53, jdufault wrote: > Continue comment on the same line. Done. https://codereview.chromium.org/2270173002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2270173002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:73970: + <int value="2" label="stylus"/> On 2016/08/23 23:09:54, jdufault wrote: > Stylus, Touch (note capitalization)? Done.
lgtm https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... File ash/wm/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.h:21: // ui::EventHandler: Make override implementation private
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Metrics lgtm % a nit: https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.cc:63: DownEventSource input_type; nit: I think the Visual Studio compiler isn't clever enough to realize that the switch stmt covers all cases, so you'll need to initialize this variable before the switch stmt -- probably to DOWN_EVENT_SOURCE_UNKNOWN. This isn't an issue for clang-based compilers, but unfortunately such is the cost of supporting multiple toolchains.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... File ash/wm/stylus_metrics_recorder.cc (right): https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.cc:63: DownEventSource input_type; On 2016/08/24 00:48:01, Ilya Sherman wrote: > nit: I think the Visual Studio compiler isn't clever enough to realize that the > switch stmt covers all cases, so you'll need to initialize this variable before > the switch stmt -- probably to DOWN_EVENT_SOURCE_UNKNOWN. This isn't an issue > for clang-based compilers, but unfortunately such is the cost of supporting > multiple toolchains. Done. https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... File ash/wm/stylus_metrics_recorder.h (right): https://codereview.chromium.org/2270173002/diff/40001/ash/wm/stylus_metrics_r... ash/wm/stylus_metrics_recorder.h:21: // ui::EventHandler: On 2016/08/24 00:24:37, jdufault wrote: > Make override implementation private Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2270173002/#ps60001 (title: "incoporate comments and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 ========== to ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 ========== to ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304}
Message was sent while issue was closed.
FYI: Findit try jobs (rerunning failed compile or tests) identified this CL at revision 414304 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2016/08/25 04:41:57, findit-for-me wrote: > FYI: Findit try jobs (rerunning failed compile or tests) identified this CL > at revision 414304 as the culprit for failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... This CL broke compilation of Chrome OS Debug: https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
Message was sent while issue was closed.
On 2016/08/25 05:13:09, haraken wrote: > On 2016/08/25 04:41:57, findit-for-me wrote: > > FYI: Findit try jobs (rerunning failed compile or tests) identified this CL > > at revision 414304 as the culprit for failures in the build cycles as shown > on: > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > This CL broke compilation of Chrome OS Debug: > > https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... Reverting in https://codereview.chromium.org/2271223003/ ("Revert Patchset" is not working now... :-/)
Message was sent while issue was closed.
On 2016/08/25 05:19:53, haraken wrote: > On 2016/08/25 05:13:09, haraken wrote: > > On 2016/08/25 04:41:57, findit-for-me wrote: > > > FYI: Findit try jobs (rerunning failed compile or tests) identified this CL > > > at revision 414304 as the culprit for failures in the build cycles as shown > > on: > > > > > > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... > > > > This CL broke compilation of Chrome OS Debug: > > > > > https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%... > > Reverting in https://codereview.chromium.org/2271223003/ > > ("Revert Patchset" is not working now... :-/) So sorry about that! I think this is because the newly added EventPointerType POINTER_TYPE_ERASER didn't get handled in the switch statement.
Message was sent while issue was closed.
Description was changed from ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ========== to ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ==========
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
new patch (patch set 5) lgtm
Description was changed from ========== Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ========== to ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2270173002/#ps80001 (title: "rebase and add switch case for POINTER_TYPE_ERASER")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ========== to ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Cr-Commit-Position: refs/heads/master@{#414304} ========== to ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Committed: https://crrev.com/06e3125412bb55231c6d26b20f5dbcdbdc1e8583 Cr-Original-Commit-Position: refs/heads/master@{#414304} Cr-Commit-Position: refs/heads/master@{#414574} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/06e3125412bb55231c6d26b20f5dbcdbdc1e8583 Cr-Commit-Position: refs/heads/master@{#414574}
Message was sent while issue was closed.
Description was changed from ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Committed: https://crrev.com/06e3125412bb55231c6d26b20f5dbcdbdc1e8583 Cr-Original-Commit-Position: refs/heads/master@{#414304} Cr-Commit-Position: refs/heads/master@{#414574} ========== to ========== Re-submit: Adding the following stylus metrics: 1. Number of “down” events generated by stylus/touch/mouse 2. Number of “down” events received by clamshell vs touchview BUG=630070 Committed: https://crrev.com/693c002fc07b010ab3ef17360a0557bd87be2832 Committed: https://crrev.com/06e3125412bb55231c6d26b20f5dbcdbdc1e8583 Cr-Original-Commit-Position: refs/heads/master@{#414304} Cr-Commit-Position: refs/heads/master@{#414574} CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
