Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(23)

Issue 2906503003: ARC: Add BootType parameter to ReportBootProgress() in metrics.mojom (Closed)

Created:
3 years, 7 months ago by Yusuke Sato
Modified:
3 years, 7 months ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, asvitkine+watch_chromium.org, Aaron Boodman, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org, cywang
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

ARC: Add BootType parameter to ReportBootProgress() in metrics.mojom ARC has 3 different boot mode. Previously, ARC boot time was recorded with the same UMA metric like Arc.AndroidBootTime regardless of the boot mode. This CL allows Chrome to use a dedicated UMA metric for each boot mode. This is better because a typical boot time is different for each boot mode. BUG=726346 TEST=tested with opt-in, ota, and regular ARC boot Review-Url: https://codereview.chromium.org/2906503003 Cr-Commit-Position: refs/heads/master@{#475184} Committed: https://chromium.googlesource.com/chromium/src/+/d6ea165b0d47b5442e7b87cbb0b0fd1006f2a115

Patch Set 1 #

Total comments: 10

Patch Set 2 : Address comments from Luis #

Total comments: 2

Patch Set 3 : Address comment #

Total comments: 14

Patch Set 4 : Address comments from Ilya #

Total comments: 5

Patch Set 5 : Address more comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -25 lines) Patch
M components/arc/common/metrics.mojom View 1 2 3 2 chunks +20 lines, -1 line 0 comments Download
M components/arc/metrics/arc_metrics_service.h View 1 1 chunk +2 lines, -2 lines 0 comments Download
M components/arc/metrics/arc_metrics_service.cc View 1 2 3 4 3 chunks +45 lines, -22 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 48 (25 generated)
Yusuke Sato
PTAL
3 years, 7 months ago (2017-05-25 02:41:19 UTC) #4
cywang
https://codereview.chromium.org/2906503003/diff/1/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/1/components/arc/metrics/arc_metrics_service.cc#newcode19 components/arc/metrics/arc_metrics_service.cc:19: UMA_HISTOGRAM_CUSTOM_TIMES("Arc.AndroidBootTime" suffix, (elapsed_time), \ You seem to use "UNKNOWN" ...
3 years, 7 months ago (2017-05-25 04:26:06 UTC) #5
Yusuke Sato
https://codereview.chromium.org/2906503003/diff/1/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/1/components/arc/metrics/arc_metrics_service.cc#newcode190 components/arc/metrics/arc_metrics_service.cc:190: switch (boot_type) { On 2017/05/25 04:26:06, cywang wrote: > ...
3 years, 7 months ago (2017-05-25 05:37:56 UTC) #8
Yusuke Sato
https://codereview.chromium.org/2906503003/diff/1/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/1/components/arc/metrics/arc_metrics_service.cc#newcode19 components/arc/metrics/arc_metrics_service.cc:19: UMA_HISTOGRAM_CUSTOM_TIMES("Arc.AndroidBootTime" suffix, (elapsed_time), \ On 2017/05/25 04:26:06, cywang wrote: ...
3 years, 7 months ago (2017-05-25 05:57:07 UTC) #9
Yusuke Sato
Luis: PTAL
3 years, 7 months ago (2017-05-25 15:41:01 UTC) #11
Luis Héctor Chávez
https://codereview.chromium.org/2906503003/diff/1/components/arc/common/metrics.mojom File components/arc/common/metrics.mojom (right): https://codereview.chromium.org/2906503003/diff/1/components/arc/common/metrics.mojom#newcode14 components/arc/common/metrics.mojom:14: // ARC++ image. nit: s/ARC++/ARC/ https://codereview.chromium.org/2906503003/diff/1/components/arc/common/metrics.mojom#newcode43 components/arc/common/metrics.mojom:43: ReportBootProgress@0(array<BootProgressEvent> events); ...
3 years, 7 months ago (2017-05-25 16:00:36 UTC) #12
Yusuke Sato
PTAL https://codereview.chromium.org/2906503003/diff/1/components/arc/common/metrics.mojom File components/arc/common/metrics.mojom (right): https://codereview.chromium.org/2906503003/diff/1/components/arc/common/metrics.mojom#newcode14 components/arc/common/metrics.mojom:14: // ARC++ image. On 2017/05/25 16:00:35, Luis Héctor ...
3 years, 7 months ago (2017-05-25 18:21:13 UTC) #15
Luis Héctor Chávez
lgtm https://codereview.chromium.org/2906503003/diff/20001/components/arc/common/metrics.mojom File components/arc/common/metrics.mojom (right): https://codereview.chromium.org/2906503003/diff/20001/components/arc/common/metrics.mojom#newcode10 components/arc/common/metrics.mojom:10: UNKNOWN = 0, nit: Add a comment that ...
3 years, 7 months ago (2017-05-25 18:24:30 UTC) #16
Yusuke Sato
https://codereview.chromium.org/2906503003/diff/20001/components/arc/common/metrics.mojom File components/arc/common/metrics.mojom (right): https://codereview.chromium.org/2906503003/diff/20001/components/arc/common/metrics.mojom#newcode10 components/arc/common/metrics.mojom:10: UNKNOWN = 0, On 2017/05/25 18:24:30, Luis Héctor Chávez ...
3 years, 7 months ago (2017-05-25 19:50:50 UTC) #20
Yusuke Sato
+dcheng@ Could you do an OWNER review for components/arc/common/metrics.mojom?
3 years, 7 months ago (2017-05-25 19:52:06 UTC) #22
Yusuke Sato
+isherman@ too. Could you have a look at the xml change? This is for ARC ...
3 years, 7 months ago (2017-05-25 20:01:19 UTC) #24
Ilya Sherman
On 2017/05/25 20:01:19, Yusuke Sato wrote: > +isherman@ too. > > Could you have a ...
3 years, 7 months ago (2017-05-25 22:32:08 UTC) #27
Yusuke Sato
Still addressing your comments, but let me answer the questions first: https://codereview.chromium.org/2906503003/diff/60001/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): ...
3 years, 7 months ago (2017-05-25 22:48:00 UTC) #28
Yusuke Sato
Please take another look. https://codereview.chromium.org/2906503003/diff/60001/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/60001/components/arc/metrics/arc_metrics_service.cc#newcode17 components/arc/metrics/arc_metrics_service.cc:17: #define RECORD_ARC_UMA(suffix, elapsed_time, max_seconds) \ ...
3 years, 7 months ago (2017-05-25 23:18:42 UTC) #31
Ilya Sherman
Metrics LGTM, thanks. https://codereview.chromium.org/2906503003/diff/60001/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/60001/components/arc/metrics/arc_metrics_service.cc#newcode17 components/arc/metrics/arc_metrics_service.cc:17: #define RECORD_ARC_UMA(suffix, elapsed_time, max_seconds) \ On ...
3 years, 7 months ago (2017-05-25 23:48:57 UTC) #32
Yusuke Sato
On 2017/05/25 23:48:57, Ilya Sherman wrote: > Metrics LGTM, thanks. > > https://codereview.chromium.org/2906503003/diff/60001/components/arc/metrics/arc_metrics_service.cc > File ...
3 years, 7 months ago (2017-05-26 02:56:39 UTC) #34
Yusuke Sato
3 years, 7 months ago (2017-05-26 18:03:31 UTC) #38
dcheng
mojo lgtm https://codereview.chromium.org/2906503003/diff/80001/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/80001/components/arc/metrics/arc_metrics_service.cc#newcode169 components/arc/metrics/arc_metrics_service.cc:169: "Arc." + event->event + BootTypeToString(boot_type); It would ...
3 years, 7 months ago (2017-05-26 20:53:46 UTC) #39
Yusuke Sato
https://codereview.chromium.org/2906503003/diff/80001/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/80001/components/arc/metrics/arc_metrics_service.cc#newcode169 components/arc/metrics/arc_metrics_service.cc:169: "Arc." + event->event + BootTypeToString(boot_type); On 2017/05/26 20:53:45, dcheng ...
3 years, 7 months ago (2017-05-26 21:34:53 UTC) #40
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2906503003/100001
3 years, 7 months ago (2017-05-26 21:35:54 UTC) #43
Ilya Sherman
https://codereview.chromium.org/2906503003/diff/80001/components/arc/metrics/arc_metrics_service.cc File components/arc/metrics/arc_metrics_service.cc (right): https://codereview.chromium.org/2906503003/diff/80001/components/arc/metrics/arc_metrics_service.cc#newcode175 components/arc/metrics/arc_metrics_service.cc:175: base::UmaHistogramCustomTimes(name, elapsed_time, kUmaMinTime, max_time, On 2017/05/26 20:53:46, dcheng wrote: ...
3 years, 7 months ago (2017-05-26 21:56:32 UTC) #44
Ilya Sherman
Oh, still LGTM, by the way. Hopefully I didn't just derail the commit queue...
3 years, 7 months ago (2017-05-26 21:58:06 UTC) #45
commit-bot: I haz the power
3 years, 7 months ago (2017-05-26 23:26:24 UTC) #48
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/d6ea165b0d47b5442e7b87cbb0b0...

Powered by Google App Engine
This is Rietveld 408576698