Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270183002:
[FRE] Add MobileFre.Progress histograms (Closed)
DescriptionAdd MobileFre.Progress histograms
Removed unused histograms.
BUG=639365
Committed: https://crrev.com/a65902c6f9f316e1a74184084df8013a80739ccd
Cr-Commit-Position: refs/heads/master@{#414190}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : nits #
Messages
Total messages: 36 (23 generated)
Description was changed from ========== format fix headers add metrics add metrics add metrics BUG= ========== to ========== Add MobileFre.Progress histograms BUG= ==========
The CQ bit was checked by gogerald@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...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Add MobileFre.Progress histograms BUG= ========== to ========== Add MobileFre.Progress histograms Removed unused histograms. BUG=639365 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (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...
gogerald@chromium.org changed reviewers: + bauerb@chromium.org, jwd@chromium.org
Hi, PTAL,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:80: private static final int FRE_PROGRESS_TERMINATOR = 128; Why 128? It's not only kind of arbitrary, but using an enum histogram will allocate every single bucket up to the maximum value, so you'd be allocating way more buckets than necessary. https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:99: private List<Integer> mFreProgressSates; Nit: ...States https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:210: RecordHistogram.recordEnumeratedHistogram(UMA_FRE_PROGRESS + mFreProgressEntryType, Would it be worth to extract this to a shared method? https://codereview.chromium.org/2270183002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2270183002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24962: + <summary>Android: Progress of the "first run experience".</summary> Not that I'm a histogram OWNER, but you probably want to explain some more what this means and when it's recorded. I guess it's a histogram over which states of the FRE have been reached, recorded every time we change state?
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java (right): https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:80: private static final int FRE_PROGRESS_TERMINATOR = 128; On 2016/08/24 09:27:23, Bernhard Bauer wrote: > Why 128? It's not only kind of arbitrary, but using an enum histogram will > allocate every single bucket up to the maximum value, so you'd be allocating way > more buckets than necessary. Done. yes, enum histogram is not sparse, https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:99: private List<Integer> mFreProgressSates; On 2016/08/24 09:27:23, Bernhard Bauer wrote: > Nit: ...States Done. https://codereview.chromium.org/2270183002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/firstrun/FirstRunActivity.java:210: RecordHistogram.recordEnumeratedHistogram(UMA_FRE_PROGRESS + mFreProgressEntryType, On 2016/08/24 09:27:23, Bernhard Bauer wrote: > Would it be worth to extract this to a shared method? Done. Might worth https://codereview.chromium.org/2270183002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2270183002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:24962: + <summary>Android: Progress of the "first run experience".</summary> On 2016/08/24 09:27:23, Bernhard Bauer wrote: > Not that I'm a histogram OWNER, but you probably want to explain some more what > this means and when it's recorded. I guess it's a histogram over which states of > the FRE have been reached, recorded every time we change state? Done. A little bit more explanations,
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
LGTM with nits https://codereview.chromium.org/2270183002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2270183002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24963: + Android: Records states of the "first run experience" have been nit: Records which states... https://codereview.chromium.org/2270183002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24964: + reached. Each bucket represents a state and recorded everytime when state nit: when -> the
https://codereview.chromium.org/2270183002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2270183002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24963: + Android: Records states of the "first run experience" have been On 2016/08/24 15:36:53, jwd wrote: > nit: Records which states... Done. https://codereview.chromium.org/2270183002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:24964: + reached. Each bucket represents a state and recorded everytime when state On 2016/08/24 15:36:53, jwd wrote: > nit: when -> the Done.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2270183002/#ps120001 (title: "nit")
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gogerald@chromium.org
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
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by gogerald@chromium.org
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 ========== Add MobileFre.Progress histograms Removed unused histograms. BUG=639365 ========== to ========== Add MobileFre.Progress histograms Removed unused histograms. BUG=639365 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add MobileFre.Progress histograms Removed unused histograms. BUG=639365 ========== to ========== Add MobileFre.Progress histograms Removed unused histograms. BUG=639365 Committed: https://crrev.com/a65902c6f9f316e1a74184084df8013a80739ccd Cr-Commit-Position: refs/heads/master@{#414190} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a65902c6f9f316e1a74184084df8013a80739ccd Cr-Commit-Position: refs/heads/master@{#414190} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
