|
|
Description[Mac] UMA Metrics for the Default Touch Bar
BUG=699321
Review-Url: https://codereview.chromium.org/2742633002
Cr-Commit-Position: refs/heads/master@{#456769}
Committed: https://chromium.googlesource.com/chromium/src/+/89dcacd0a5b19051970fafaab3321338a50833b4
Patch Set 1 #Patch Set 2 : Added a comment #Patch Set 3 : nit #
Total comments: 5
Patch Set 4 : update for home button #
Total comments: 2
Patch Set 5 : Fix for rsesek #
Total comments: 2
Patch Set 6 : Fix for isherman #
Messages
Total messages: 55 (41 generated)
The CQ bit was checked by spqchan@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 checked by spqchan@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by spqchan@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: This issue passed the CQ dry run.
spqchan@chromium.org changed reviewers: + isherman@chromium.org, rsesek@chromium.org
PTAL +rsesek for cocoa +isherman for histogram.xml
https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:114: UMA_HISTOGRAM_ENUMERATION("DefaultTouchBar.Metrics", action, Since we may have multiple touch bars, how about something like: TouchBar.Default.Metrics or OSX.TouchBar.Metrics ? https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:268: LogTouchBarUMA(command); In -backOrForward, the UMA is logged before the ExecuteCommand call, and here it's done after. Does the order matter? (IIRC, ExecuteCommand also records an UMA). Regardless, the order should probably match.
On 2017/03/09 18:11:55, Robert Sesek wrote: > https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... > File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): > > https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:114: > UMA_HISTOGRAM_ENUMERATION("DefaultTouchBar.Metrics", action, > Since we may have multiple touch bars, how about something like: > > TouchBar.Default.Metrics or OSX.TouchBar.Metrics ? > > https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... > chrome/browser/ui/cocoa/browser_window_touch_bar.mm:268: > LogTouchBarUMA(command); > In -backOrForward, the UMA is logged before the ExecuteCommand call, and here > it's done after. Does the order matter? (IIRC, ExecuteCommand also records an > UMA). Regardless, the order should probably match. Sorry, can we hold the CL? I just got a message that we need a home button on the Touch Bar so I'm going to add that first (probably in this CL). Thanks!
Metrics LGTM % the metric name discussion. https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:114: UMA_HISTOGRAM_ENUMERATION("DefaultTouchBar.Metrics", action, On 2017/03/09 18:11:55, Robert Sesek wrote: > Since we may have multiple touch bars, how about something like: > > TouchBar.Default.Metrics or OSX.TouchBar.Metrics ? +1 -- let's choose a fairly general top-level category. Either of Robert's suggestions seems reasonable.
The CQ bit was checked by spqchan@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...
Description was changed from ========== [Mac] Added Default Touch Bar UMA Metrics BUG=699321 ========== to ========== [Mac] Home Button and UMA Metrics for the Default Touch Bar BUG=699321 ==========
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...)
The CQ bit was checked by spqchan@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...
On 2017/03/09 18:32:31, spqchan wrote: > Sorry, can we hold the CL? I just got a message that we need a home button on > the Touch Bar so I'm going to add that first (probably in this CL). > Thanks! Please do separate out adding metrics from adding new functionality. Keeping CLs smaller and focused makes review easier.
On 2017/03/09 23:46:55, Robert Sesek wrote: > On 2017/03/09 18:32:31, spqchan wrote: > > Sorry, can we hold the CL? I just got a message that we need a home button on > > the Touch Bar so I'm going to add that first (probably in this CL). > > Thanks! > > Please do separate out adding metrics from adding new functionality. Keeping CLs > smaller and focused makes review easier. Sure
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== [Mac] Home Button and UMA Metrics for the Default Touch Bar BUG=699321 ========== to ========== [Mac] UMA Metrics for the Default Touch Bar BUG=699321 ==========
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #4 (id:60001) has been deleted
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: This issue passed the CQ dry run.
PTAL, I added the metric for the home button here. isherman@, can you look at histogram.xml again? I added a new line there Thanks! https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:114: UMA_HISTOGRAM_ENUMERATION("DefaultTouchBar.Metrics", action, On 2017/03/09 18:11:55, Robert Sesek wrote: > Since we may have multiple touch bars, how about something like: > > TouchBar.Default.Metrics or OSX.TouchBar.Metrics ? Sure thing https://codereview.chromium.org/2742633002/diff/40001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:268: LogTouchBarUMA(command); On 2017/03/09 18:11:55, Robert Sesek wrote: > In -backOrForward, the UMA is logged before the ExecuteCommand call, and here > it's done after. Does the order matter? (IIRC, ExecuteCommand also records an > UMA). Regardless, the order should probably match. Done.
https://codereview.chromium.org/2742633002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2742633002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: if (command == IDC_BACK) Would this be better as a switch?
LGTM https://codereview.chromium.org/2742633002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2742633002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:72228: +<histogram name="TouchBar.Default.Metrics" enum="DefaultTouchBarAction"> nit: Maybe s/Metrics/Actions?
The CQ bit was checked by spqchan@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 checked by spqchan@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...
Thanks! rsesek, PTAL https://codereview.chromium.org/2742633002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/browser_window_touch_bar.mm (right): https://codereview.chromium.org/2742633002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/browser_window_touch_bar.mm:104: if (command == IDC_BACK) On 2017/03/13 21:05:25, Robert Sesek wrote: > Would this be better as a switch? Done. https://codereview.chromium.org/2742633002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2742633002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:72228: +<histogram name="TouchBar.Default.Metrics" enum="DefaultTouchBarAction"> On 2017/03/13 22:25:02, Ilya Sherman wrote: > nit: Maybe s/Metrics/Actions? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
On 2017/03/14 02:20:05, Robert Sesek wrote: > LGTM thanks!
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2742633002/#ps140001 (title: "Fix for isherman")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489515959603320, "parent_rev": "b6eb9203f08cb87cdbaa93f7f6ea4b6b5a9dd41b", "commit_rev": "89dcacd0a5b19051970fafaab3321338a50833b4"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] UMA Metrics for the Default Touch Bar BUG=699321 ========== to ========== [Mac] UMA Metrics for the Default Touch Bar BUG=699321 Review-Url: https://codereview.chromium.org/2742633002 Cr-Commit-Position: refs/heads/master@{#456769} Committed: https://chromium.googlesource.com/chromium/src/+/89dcacd0a5b19051970fafaab332... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/89dcacd0a5b19051970fafaab332... |