Switch UKM service to properly mark upload data as UKM and not UMA.

Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the correct attribution.

TBR=olivierrobin@chromium.org,boliu@chromium.org

BUG=694735
Review-Url: https://codereview.chromium.org/2708293002
Cr-Commit-Position: refs/heads/master@{#452830}
Committed: https://chromium.googlesource.com/chromium/src/+/9c420825985ca92a144a7e74ec114cd06759bf9a

[rkaplow, 2017-02-22 01:38:13]

Description was changed from

==========
Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the
correct attribution.

BUG=694735
==========

to

==========
Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the
correct attribution.

BUG=694735
==========

[rkaplow, 2017-02-22 01:38:13]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org, bengr@chromium.org

[rkaplow, 2017-02-22 01:39:07]

ben, can your review the data_use_measurement
alexei - everything

[Alexei Svitkine (slow), 2017-02-22 15:46:11]

lgtm % comments

https://codereview.chromium.org/2708293002/diff/1/components/metrics/metrics_...
File components/metrics/metrics_log_uploader.h (right):

https://codereview.chromium.org/2708293002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log_uploader.h:48: MetricServiceType service_type_;
Nit: const?

https://codereview.chromium.org/2708293002/diff/1/components/metrics/net/net_...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/2708293002/diff/1/components/metrics/net/net_...
components/metrics/net/net_metrics_log_uploader.cc:38: if (service_type_ ==
MetricsLogUploader::UKM)
Can you do a switch instead?

Switch guarantees if a new enum is added, there will be an error if the switch
wasn't updated due to compiler options Chromium build uses.

[rkaplow, 2017-02-22 16:11:35]

https://codereview.chromium.org/2708293002/diff/1/components/metrics/metrics_...
File components/metrics/metrics_log_uploader.h (right):

https://codereview.chromium.org/2708293002/diff/1/components/metrics/metrics_...
components/metrics/metrics_log_uploader.h:48: MetricServiceType service_type_;
On 2017/02/22 15:46:10, Alexei Svitkine (slow) wrote:
> Nit: const?

Done.

https://codereview.chromium.org/2708293002/diff/1/components/metrics/net/net_...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/2708293002/diff/1/components/metrics/net/net_...
components/metrics/net/net_metrics_log_uploader.cc:38: if (service_type_ ==
MetricsLogUploader::UKM)
On 2017/02/22 15:46:10, Alexei Svitkine (slow) wrote:
> Can you do a switch instead?
> 
> Switch guarantees if a new enum is added, there will be an error if the switch
> wasn't updated due to compiler options Chromium build uses.

Done.

[rkaplow, 2017-02-23 02:47:00]

rkaplow@chromium.org changed reviewers:
+ tbansal@chromium.org

[tbansal1, 2017-02-23 18:29:27]

lgtm  components/data_use_measurement/*
+ A nit about the switch statement in net_metrics_log_uploader.cc.

Thanks for doing this.

https://codereview.chromium.org/2708293002/diff/20001/components/metrics/net/...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/2708293002/diff/20001/components/metrics/net/...
components/metrics/net/net_metrics_log_uploader.cc:45: default:
Suggest not having a default case so that compiler catches the missing  case
(instead of relying on run-time catch).
See
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch...

[rkaplow, 2017-02-23 18:53:33]

On 2017/02/23 18:29:27, tbansal1 wrote:
> lgtm  components/data_use_measurement/*
> + A nit about the switch statement in net_metrics_log_uploader.cc.
> 
> Thanks for doing this.
> 
>
https://codereview.chromium.org/2708293002/diff/20001/components/metrics/net/...
> File components/metrics/net/net_metrics_log_uploader.cc (right):
> 
>
https://codereview.chromium.org/2708293002/diff/20001/components/metrics/net/...
> components/metrics/net/net_metrics_log_uploader.cc:45: default:
> Suggest not having a default case so that compiler catches the missing  case
> (instead of relying on run-time catch).
> See
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch...

ah thanks, didn't know.

[rkaplow, 2017-02-23 18:53:47]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2017-02-23 18:53:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2708293002/#ps40001
(title: "remove default")

[commit-bot: I haz the power, 2017-02-23 18:54:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 18:56:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 18:56:31]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rkaplow, 2017-02-23 19:54:21]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2017-02-23 19:54:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2708293002/#ps60001
(title: "merge")

[commit-bot: I haz the power, 2017-02-23 19:54:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 20:05:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 20:05:03]

Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rkaplow, 2017-02-23 20:33:04]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2017-02-23 20:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2708293002/#ps80001
(title: "fix uploadlog after merge")

[commit-bot: I haz the power, 2017-02-23 20:33:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 20:42:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 20:42:50]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[rkaplow, 2017-02-23 21:56:56]

The CQ bit was checked by rkaplow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 21:57:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 22:06:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:06:43]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[rkaplow, 2017-02-23 22:15:03]

The CQ bit was checked by rkaplow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 22:15:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 22:25:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 22:25:36]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rkaplow, 2017-02-23 22:48:59]

The CQ bit was checked by rkaplow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 22:50:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:00:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:00:21]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[rkaplow, 2017-02-23 23:13:57]

The CQ bit was checked by rkaplow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 23:14:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:49:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:49:20]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[rkaplow, 2017-02-24 00:12:50]

The CQ bit was checked by rkaplow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 00:13:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2017-02-24 00:14:26]

lgtm

[commit-bot: I haz the power, 2017-02-24 01:50:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 01:50:30]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-02-24 05:47:18]

The CQ bit was checked by rkaplow@chromium.org

[rkaplow, 2017-02-24 05:47:19]

The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org
Link to the patchset: https://codereview.chromium.org/2708293002/#ps180001
(title: "no switch default now requires a default value")

[commit-bot: I haz the power, 2017-02-24 05:47:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 05:55:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 05:55:22]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rkaplow, 2017-02-24 15:15:31]

rkaplow@chromium.org changed reviewers:
+ boliu@chromium.org, olivierrobin@chromium.org

[rkaplow, 2017-02-24 15:15:55]

Olivier - owner review for iOS
boliu - webview. 

Thanks

[Olivier, 2017-02-24 15:23:03]

ios LGTM

[rkaplow, 2017-02-24 15:24:28]

Description was changed from

==========
Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the
correct attribution.

BUG=694735
==========

to

==========
Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the
correct attribution.

TBR=olivierrobin@chromium.org,boliu@chromium.org

BUG=694735
==========

[rkaplow, 2017-02-24 15:24:38]

The CQ bit was checked by rkaplow@chromium.org

[commit-bot: I haz the power, 2017-02-24 15:24:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 15:30:01]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487949878617900,
"parent_rev": "a817f9bb675d37c4da247568b7291224fc1cf6a8", "commit_rev":
"9c420825985ca92a144a7e74ec114cd06759bf9a"}

[commit-bot: I haz the power, 2017-02-24 15:30:40]

Description was changed from

==========
Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the
correct attribution.

TBR=olivierrobin@chromium.org,boliu@chromium.org

BUG=694735
==========

to

==========
Switch UKM service to properly mark upload data as UKM and not UMA.

This also refactors the api such that new services can more easily do the
correct attribution.

TBR=olivierrobin@chromium.org,boliu@chromium.org

BUG=694735

Review-Url: https://codereview.chromium.org/2708293002
Cr-Commit-Position: refs/heads/master@{#452830}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c420825985ca92a144a7e74ec11...
==========

[commit-bot: I haz the power, 2017-02-24 15:30:41]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/9c420825985ca92a144a7e74ec11...

[boliu, 2017-02-24 16:03:24]

rs lgtm