[GRC] UKM Support

[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

Design Doc Link:
https://docs.google.com/document/d/1p03qPe0mwkUcY9t9drbUucSDYsScgpIXxGPbDQIcvkk/edit

R=*dcheng@chromium.org,*holte@chromium.org,nasko@chromium.org,*oysteine@chromium.org,zhenw@chromium.org,fmeawad@chromium.org
BUG=724306
Review-Url: https://codereview.chromium.org/2938443002
Cr-Commit-Position: refs/heads/master@{#482069}
Committed: https://chromium.googlesource.com/chromium/src/+/097bcd379f843c5dcb8821d6049fd9e8b6814cfd

[matthalp, 2017-06-12 16:40:16]

Description was changed from

==========
Initialize CL

BUG=
==========

to

==========
UKM Integration

BUG=
==========

[matthalp, 2017-06-12 18:40:33]

Description was changed from

==========
UKM Integration

BUG=
==========

to

==========
UKM Integration

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-12 18:40:34]

matthalp@google.com changed reviewers:
+ oysteine@chromium.org

[matthalp, 2017-06-12 18:51:02]

Description was changed from

==========
UKM Integration

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support within GRC
straightforward to implement. The UKM service currently lives within the
content_browser service which is out-of-scope for the resource_coordinator
service
to bind to. When UKM will be moved into its own service is TBD, so as a
workaround
the resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator service
through a newly added interface: ServiceCallbacks. The ServiceCallbacks will
also be used as a stopgap to solve the same layering problem that may occur with
services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM. Note that
the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-13 19:15:15]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 19:15:41]

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-06-13 19:21:04]

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

[commit-bot: I haz the power, 2017-06-13 19:21:04]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[matthalp, 2017-06-13 19:26:06]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 19:26:48]

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-06-13 19:41:24]

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

[commit-bot: I haz the power, 2017-06-13 19:41:25]

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...)

[matthalp, 2017-06-13 20:09:35]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 20:10:29]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-13 20:20:41]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 20:21:29]

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-06-13 20:47:24]

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

[commit-bot: I haz the power, 2017-06-13 20:47:25]

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...)

[matthalp, 2017-06-13 21:15:02]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 21:15:41]

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-06-13 21:20:11]

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

[commit-bot: I haz the power, 2017-06-13 21:20:12]

Dry run: 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...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[matthalp, 2017-06-13 21:32:21]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 21:33:29]

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-06-13 21:52:55]

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

[commit-bot: I haz the power, 2017-06-13 21:52:56]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[matthalp, 2017-06-13 22:16:40]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 22:17:22]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-13 22:18:16]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-13 22:27:34]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 22:28:18]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-13 22:34:17]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-13 22:39:01]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 22:39:44]

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-06-13 23:04:17]

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

[commit-bot: I haz the power, 2017-06-13 23:04:20]

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...)

[matthalp, 2017-06-13 23:09:51]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 23:10:41]

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-06-13 23:47:59]

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

[commit-bot: I haz the power, 2017-06-13 23:48:00]

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...)

[matthalp, 2017-06-13 23:54:15]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 23:54:47]

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-06-14 00:20:02]

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

[commit-bot: I haz the power, 2017-06-14 00:20:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[matthalp, 2017-06-14 00:37:22]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 00:37:56]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-14 00:54:28]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-14 00:56:43]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 00:57:02]

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-06-14 03:13:21]

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

[commit-bot: I haz the power, 2017-06-14 03:13:22]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-14 14:06:41]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 14:06:54]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-14 14:18:52]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-14 14:29:47]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 14:30:25]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-14 14:39:11]

The CQ bit was unchecked by matthalp@google.com

[matthalp, 2017-06-14 14:54:18]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 14:54:44]

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-06-14 15:07:12]

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

[commit-bot: I haz the power, 2017-06-14 15:07:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[matthalp, 2017-06-14 15:31:16]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:31:38]

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-06-14 15:52:46]

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

[commit-bot: I haz the power, 2017-06-14 15:52:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[matthalp, 2017-06-14 18:27:24]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 18:27:54]

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-06-14 20:07:09]

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

[commit-bot: I haz the power, 2017-06-14 20:07:10]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-14 20:19:06]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support within GRC
straightforward to implement. The UKM service currently lives within the
content_browser service which is out-of-scope for the resource_coordinator
service
to bind to. When UKM will be moved into its own service is TBD, so as a
workaround
the resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator service
through a newly added interface: ServiceCallbacks. The ServiceCallbacks will
also be used as a stopgap to solve the same layering problem that may occur with
services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM. Note that
the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator service
through a newly added interface: ServiceCallbacks. The ServiceCallbacks will
also be used as a stopgap to solve the same layering problem that may occur with
services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM. Note that
the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-14 20:19:33]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator service
through a newly added interface: ServiceCallbacks. The ServiceCallbacks will
also be used as a stopgap to solve the same layering problem that may occur with
services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM. Note that
the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The ServiceCallbacks
will also be used as a stopgap to solve the same layering problem that may occur
with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM. Note that
the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-14 20:19:56]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The ServiceCallbacks
will also be used as a stopgap to solve the same layering problem that may occur
with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM. Note that
the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-14 20:20:28]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-14 20:52:56]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org
BUG=724306
==========

[matthalp, 2017-06-14 20:52:56]

matthalp@google.com changed reviewers:
+ holte@chromium.org, nasko@chromium.org

[matthalp, 2017-06-14 20:53:05]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 20:53:26]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-14 23:26:45]

+nasko
 
  services/resource_coordinator/public/interfaces/service_callbacks.mojom
    - Create interface to register service callbacks due to layering issues (see
comments)

  content/public/app/mojo/content_browser_manifest.json
    - Allow content_browser::renderer to make mojo calls to itself
 
+holte
 
  components/ukm/public/mojo_ukm_recorder.cc
    - Make MojoUkmRecorder class exportable
    - Make MojoUkmRecorder constructor explicit
  
  components/ukm/public/mojo_ukm_recorder.h
     - Add correct header (as exposed by ‘git cl lint’)
 
  components/ukm/public/ukm_recorder.h
     - Add correct header (as exposed by ‘git cl lint’)
     - Register resource_coordinator::CoordinationUnitManager as friend class
 
+oysteine
 
   chrome/browser/resource_coordinator/DEPS
  
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
  
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h
   services/resource_coordinator/BUILD.gn
   services/resource_coordinator/DEPS
   services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
   services/resource_coordinator/coordination_unit/coordination_unit_manager.h
   services/resource_coordinator/manifest.json
   services/resource_coordinator/public/interfaces/BUILD.gn
   services/resource_coordinator/resource_coordinator_service.cc
   services/resource_coordinator/resource_coordinator_service.h
   services/resource_coordinator/service_callbacks_impl.cc
   services/resource_coordinator/service_callbacks_impl.h
     - Changes to resource_coordinator service

[commit-bot: I haz the power, 2017-06-14 23:26:54]

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

[commit-bot: I haz the power, 2017-06-14 23:26:55]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-14 23:40:37]

matthalp@google.com changed reviewers:
+ zhenw@chromium.org

[matthalp, 2017-06-14 23:40:37]

matthalp@google.com changed required reviewers:
+ holte@chromium.org, nasko@chromium.org, oysteine@chromium.org

[matthalp, 2017-06-15 17:00:58]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org,fmeawad@chromiu...
BUG=724306
==========

[matthalp, 2017-06-15 17:00:58]

matthalp@google.com changed reviewers:
+ fmeawad@chromium.org

[matthalp, 2017-06-15 17:01:21]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org,fmeawad@chromiu...
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org,zhenw@chromium....
BUG=724306
==========

[matthalp, 2017-06-15 17:23:58]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org,zhenw@chromium....
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

Design Doc Link:
https://docs.google.com/document/d/1p03qPe0mwkUcY9t9drbUucSDYsScgpIXxGPbDQIcv...

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org,zhenw@chromium....
BUG=724306
==========

[nasko, 2017-06-16 15:51:39]

I'm still not fully confident in my abilities to competently review service
manifest changes. Since this CL has only
content/public/app/mojo/content_browser_manifest.json for me to review, please
ask someone else with more manifest experience and I'll be happy to stamp it.

[Steven Holte, 2017-06-16 20:09:41]

The UKM changes lgtm, but be aware of
https://chromium-review.googlesource.com/c/536933/

Which will move these files to services/metrics/public

[matthalp, 2017-06-21 02:02:19]

On 2017/06/16 at 20:09:41, holte wrote:
> The UKM changes lgtm, but be aware of
https://chromium-review.googlesource.com/c/536933/
> 
> Which will move these files to services/metrics/public

Thank you for letting us know! It looks like that CL is close to landing, so I'm
going to close this CL in hopes of using that one.

[matthalp, 2017-06-22 17:29:43]

matthalp@google.com changed reviewers:
- fmeawad@chromium.org

[matthalp, 2017-06-22 17:29:43]

matthalp@google.com changed required reviewers:
- nasko@chromium.org

[matthalp, 2017-06-22 17:33:57]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

Design Doc Link:
https://docs.google.com/document/d/1p03qPe0mwkUcY9t9drbUucSDYsScgpIXxGPbDQIcv...

R=holte@chromium.org,nasko@chromium.org,oysteine@chromium.org,zhenw@chromium....
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

Design Doc Link:
https://docs.google.com/document/d/1p03qPe0mwkUcY9t9drbUucSDYsScgpIXxGPbDQIcv...

R=*dcheng@chromium.org,*holte@chromium.org,nasko@chromium.org,*oysteine@chrom...
BUG=724306
==========

[matthalp, 2017-06-22 17:33:57]

matthalp@google.com changed reviewers:
+ dcheng@chromium.org

[matthalp, 2017-06-22 17:33:57]

matthalp@google.com changed required reviewers:
+ dcheng@chromium.org

[matthalp, 2017-06-22 17:41:40]

On 2017/06/21 at 02:02:19, matthalp wrote:
> On 2017/06/16 at 20:09:41, holte wrote:
> > The UKM changes lgtm, but be aware of
https://chromium-review.googlesource.com/c/536933/
> > 
> > Which will move these files to services/metrics/public
> 
> Thank you for letting us know! It looks like that CL is close to landing, so
I'm going to close this CL in hopes of using that one.

CL reopened after discussion with holte and oysteine.

+dcheng to review content/public/app/mojo/content_browser_manifest.json:

  (1) Allow the content_browser service to make request to its own interfaces
under content_browser:renderer. This allows the UKM mojo interface to be
initialized in content_browser and then pass it to resource_coordinator service
via the new ServiceCallbacks interface. resource_coordinator is unable to do
this itself due to software layering issues.
  (2) Register service_callbacks interface to resource_coordinator in order to
pass the UKM interface from content_browser into resource_coordinator.

[oystein (OOO til 10th of July), 2017-06-22 20:27:02]

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
File chrome/browser/resource_coordinator/DEPS (right):

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
chrome/browser/resource_coordinator/DEPS:6:
"+content/public/renderer/render_thread.h",
Where is this used? I see it included in
resource_coordinator_web_contents_observer.cc but can't see it actually used?

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:54:
void ResourceCoordinatorWebContentsObserver::MaybeSetUkmRecorderInterface(
As an additional optimization, maybe set a static bool here which you can check
for in EnsureUkmRecorderInterface(), to avoid some of the future roundtrips.

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:33:
void CoordinationUnitManager::SetUkmRecorder(
inline this as set_ukm_recorder()

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:41:
ukm::MojoUkmRecorder* ukm_recorder_;
= nullptr;

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
File services/resource_coordinator/public/interfaces/service_callbacks.mojom
(right):

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
services/resource_coordinator/public/interfaces/service_callbacks.mojom:12: //
the UKM mojo interface lives within the content_browser service which is not
accesible
s/accesible/accessible/

I'd reword this a little bit: The interface is accessible, as it's in
//components/; it's just that it's *bound* within content_browser and hence
needs the browser to bind it and then to pass a messagepipe down to the GRC
service.

[matthalp, 2017-06-22 22:57:54]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

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

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-22 22:59:19]

On 2017/06/22 at 20:27:02, oysteine wrote:
>
https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
> File chrome/browser/resource_coordinator/DEPS (right):
> 
>
https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
> chrome/browser/resource_coordinator/DEPS:6:
"+content/public/renderer/render_thread.h",
> Where is this used? I see it included in
resource_coordinator_web_contents_observer.cc but can't see it actually used?
> 
>
https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
> File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):
> 
>
https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
>
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:54:
void ResourceCoordinatorWebContentsObserver::MaybeSetUkmRecorderInterface(
> As an additional optimization, maybe set a static bool here which you can
check for in EnsureUkmRecorderInterface(), to avoid some of the future
roundtrips.
> 
>
https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
> File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):
> 
>
https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
>
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:33:
void CoordinationUnitManager::SetUkmRecorder(
> inline this as set_ukm_recorder()
> 
>
https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
> File
services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):
> 
>
https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
>
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:41:
ukm::MojoUkmRecorder* ukm_recorder_;
> = nullptr;
> 
>
https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
> File services/resource_coordinator/public/interfaces/service_callbacks.mojom
(right):
> 
>
https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
> services/resource_coordinator/public/interfaces/service_callbacks.mojom:12: //
the UKM mojo interface lives within the content_browser service which is not
accesible
> s/accesible/accessible/
> 
> I'd reword this a little bit: The interface is accessible, as it's in
//components/; it's just that it's *bound* within content_browser and hence
needs the browser to bind it and then to pass a messagepipe down to the GRC
service.

oysteine@, your feedback has been addressed. PTAL

[matthalp, 2017-06-22 22:59:31]

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
File chrome/browser/resource_coordinator/DEPS (right):

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
chrome/browser/resource_coordinator/DEPS:6:
"+content/public/renderer/render_thread.h",
On 2017/06/22 at 20:27:02, oystein wrote:
> Where is this used? I see it included in
resource_coordinator_web_contents_observer.cc but can't see it actually used?

Removed. It was used in a previous iteration that was changed not to use it.

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
File
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc
(right):

https://codereview.chromium.org/2938443002/diff/490001/chrome/browser/resourc...
chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.cc:54:
void ResourceCoordinatorWebContentsObserver::MaybeSetUkmRecorderInterface(
On 2017/06/22 at 20:27:02, oystein wrote:
> As an additional optimization, maybe set a static bool here which you can
check for in EnsureUkmRecorderInterface(), to avoid some of the future
roundtrips.

Good suggestion -- done.

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
File
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc
(right):

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.cc:33:
void CoordinationUnitManager::SetUkmRecorder(
On 2017/06/22 at 20:27:02, oystein wrote:
> inline this as set_ukm_recorder()

Done.

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
File services/resource_coordinator/coordination_unit/coordination_unit_manager.h
(right):

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
services/resource_coordinator/coordination_unit/coordination_unit_manager.h:41:
ukm::MojoUkmRecorder* ukm_recorder_;
On 2017/06/22 at 20:27:02, oystein wrote:
> = nullptr;

Done.

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
File services/resource_coordinator/public/interfaces/service_callbacks.mojom
(right):

https://codereview.chromium.org/2938443002/diff/490001/services/resource_coor...
services/resource_coordinator/public/interfaces/service_callbacks.mojom:12: //
the UKM mojo interface lives within the content_browser service which is not
accesible
On 2017/06/22 at 20:27:02, oystein wrote:
> s/accesible/accessible/
> 
> I'd reword this a little bit: The interface is accessible, as it's in
//components/; it's just that it's *bound* within content_browser and hence
needs the browser to bind it and then to pass a messagepipe down to the GRC
service.

Done. Let me know if this could be improved further.

[commit-bot: I haz the power, 2017-06-23 00:32:29]

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

[commit-bot: I haz the power, 2017-06-23 00:32:30]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-23 16:10:35]

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:60: //
TODO(matthalp,oysteine,holte) isolate ukm capabilities from the render
Nit: 80 chars please (same below)

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:63: "content_browser": [
"ash", "display", "memory_instrumentation", "renderer" ],
Maybe we should split the renderer capability to be a little more fine-grained?
Is that possible to do here without having to refactor everything?

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
File services/resource_coordinator/public/interfaces/service_callbacks.mojom
(right):

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
services/resource_coordinator/public/interfaces/service_callbacks.mojom:9: //
This class is intended to provide an interface to register mojo interfaces that
mojo interfaces should still obey the 80 character line limit

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
File services/resource_coordinator/service_callbacks_impl.cc (right):

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
services/resource_coordinator/service_callbacks_impl.cc:19: :
service_ref_factory_(service_ref_factory),
What's the reason we need to hang on to a pointer for this object?

[matthalp, 2017-06-23 16:36:28]

holte could you comment on dcheng's comment in
content/public/app/mojo/content_browser_manifest.json? Is there an easy way to
address this?

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:60: //
TODO(matthalp,oysteine,holte) isolate ukm capabilities from the render
On 2017/06/23 at 16:10:35, dcheng wrote:
> Nit: 80 chars please (same below)

Done.

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:63: "content_browser": [
"ash", "display", "memory_instrumentation", "renderer" ],
On 2017/06/23 at 16:10:35, dcheng wrote:
> Maybe we should split the renderer capability to be a little more
fine-grained? Is that possible to do here without having to refactor everything?

holte@ what do you think? I am not sure what entities strictly use
content_browser:renderer solely for access to UKM.

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
File services/resource_coordinator/public/interfaces/service_callbacks.mojom
(right):

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
services/resource_coordinator/public/interfaces/service_callbacks.mojom:9: //
This class is intended to provide an interface to register mojo interfaces that
On 2017/06/23 at 16:10:35, dcheng wrote:
> mojo interfaces should still obey the 80 character line limit

Done.

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
File services/resource_coordinator/service_callbacks_impl.cc (right):

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
services/resource_coordinator/service_callbacks_impl.cc:19: :
service_ref_factory_(service_ref_factory),
On 2017/06/23 at 16:10:35, dcheng wrote:
> What's the reason we need to hang on to a pointer for this object?

The service is ref-counted, so holding a reference to the service ensures that
it will stay alive while we are connected.

The service folks plan to encapsulate this eventually, but for now this his what
they recommend. oysteine@ can comment more if need be.

[Steven Holte, 2017-06-23 18:11:02]

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
File content/public/app/mojo/content_browser_manifest.json (right):

https://codereview.chromium.org/2938443002/diff/510001/content/public/app/moj...
content/public/app/mojo/content_browser_manifest.json:63: "content_browser": [
"ash", "display", "memory_instrumentation", "renderer" ],
On 2017/06/23 16:36:28, matthalp wrote:
> On 2017/06/23 at 16:10:35, dcheng wrote:
> > Maybe we should split the renderer capability to be a little more
> fine-grained? Is that possible to do here without having to refactor
everything?
> 
> holte@ what do you think? I am not sure what entities strictly use
> content_browser:renderer solely for access to UKM.

AFAIK nothing has actually started using the UKM mojo interface yet, so you can
freely move it into a new capability.

[matthalp, 2017-06-23 18:26:15]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 18:26:32]

Dry run: CQ is trying da patch.

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

[matthalp, 2017-06-23 18:27:11]

I created a separate UKM capability.

holte, dcheng PTAL

[commit-bot: I haz the power, 2017-06-23 18:29:50]

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

[commit-bot: I haz the power, 2017-06-23 18:29:52]

Dry run: 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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[matthalp, 2017-06-23 19:04:11]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 19:04:34]

Dry run: CQ is trying da patch.

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

[oystein (OOO til 10th of July), 2017-06-23 19:18:43]

lgtm

[commit-bot: I haz the power, 2017-06-23 20:31:23]

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

[commit-bot: I haz the power, 2017-06-23 20:31:24]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-23 21:32:32]

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
File services/resource_coordinator/service_callbacks_impl.cc (right):

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
services/resource_coordinator/service_callbacks_impl.cc:19: :
service_ref_factory_(service_ref_factory),
On 2017/06/23 16:36:28, matthalp wrote:
> On 2017/06/23 at 16:10:35, dcheng wrote:
> > What's the reason we need to hang on to a pointer for this object?
> 
> The service is ref-counted, so holding a reference to the service ensures that
> it will stay alive while we are connected.
> 
> The service folks plan to encapsulate this eventually, but for now this his
what
> they recommend. oysteine@ can comment more if need be.

Right, but isn't the ref created and held below on line 22? It doesn't seem like
the service_ref_factory pointer is useful after that.

https://codereview.chromium.org/2938443002/diff/570001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2938443002/diff/570001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_manifest_overlay.json:21: ],
Tab =)

https://codereview.chromium.org/2938443002/diff/570001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_manifest_overlay.json:22: "ukm": [
I think it's worth expanding the capability name to be url_keyed_metrics (it's a
bit cryptic as it is)

Also, don't things that used to ukm implicitly through renderer need to have
their manifests updated as well?

[matthalp, 2017-06-23 21:56:13]

dcheng, Thanks for the quick turnaround. I've addressed your latest feedback.

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
File services/resource_coordinator/service_callbacks_impl.cc (right):

https://codereview.chromium.org/2938443002/diff/510001/services/resource_coor...
services/resource_coordinator/service_callbacks_impl.cc:19: :
service_ref_factory_(service_ref_factory),
On 2017/06/23 at 21:32:32, dcheng wrote:
> On 2017/06/23 16:36:28, matthalp wrote:
> > On 2017/06/23 at 16:10:35, dcheng wrote:
> > > What's the reason we need to hang on to a pointer for this object?
> > 
> > The service is ref-counted, so holding a reference to the service ensures
that
> > it will stay alive while we are connected.
> > 
> > The service folks plan to encapsulate this eventually, but for now this his
what
> > they recommend. oysteine@ can comment more if need be.
> 
> Right, but isn't the ref created and held below on line 22? It doesn't seem
like the service_ref_factory pointer is useful after that.

Yes, you are correct. Sorry for the confusion. I've removed the factory member.

https://codereview.chromium.org/2938443002/diff/570001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_manifest_overlay.json (right):

https://codereview.chromium.org/2938443002/diff/570001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_manifest_overlay.json:21: ],
On 2017/06/23 at 21:32:32, dcheng wrote:
> Tab =)

Thanks for catching this. It didn't show up in my editor.

https://codereview.chromium.org/2938443002/diff/570001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_manifest_overlay.json:22: "ukm": [
On 2017/06/23 at 21:32:32, dcheng wrote:
> I think it's worth expanding the capability name to be url_keyed_metrics (it's
a bit cryptic as it is)
> 
> Also, don't things that used to ukm implicitly through renderer need to have
their manifests updated as well?

Using url_keyed_metrics instead of ukm makes sense to me -- done.

This is the first CL to use the UkmRecorderInterface so there are no additional
manifests to update:
https://cs.chromium.org/search/?q=ukm::mojom::UkmRecorderInterfacePtr+package...

[matthalp, 2017-06-23 21:56:23]

The CQ bit was checked by matthalp@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 21:56:38]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-06-23 22:14:37]

It makes me a little sad that we have to do this dance to get the interface
pointer over. Hopefully we can fix the layering sooner rather than later.

[dcheng, 2017-06-23 22:14:51]

(oops meant to type lgtm as well)

[chromium-reviews, 2017-06-23 22:17:34]

Agreed. Fortunately, the UKM folks are aware and working on it.

On Fri, Jun 23, 2017 at 3:14 PM, <dcheng@chromium.org> wrote:

> (oops meant to type lgtm as well)
>
> https://codereview.chromium.org/2938443002/
>



-- 

Matthew Halpern
Intern, Chrome Speed Infrastructure

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-06-23 23:21:24]

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

[commit-bot: I haz the power, 2017-06-23 23:21:25]

Dry run: This issue passed the CQ dry run.

[matthalp, 2017-06-23 23:22:03]

The CQ bit was checked by matthalp@google.com

[matthalp, 2017-06-23 23:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org,
oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/2938443002/#ps590001
(title: "Address reviewer feedback")

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

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-06-23 23:26:33]

CQ is committing da patch.

Bot data: {"patchset_id": 590001, "attempt_start_ts": 1498260123583310,
"parent_rev": "8aa76996bceb1380be8ae86f69633e776e552fb7", "commit_rev":
"097bcd379f843c5dcb8821d6049fd9e8b6814cfd"}

[commit-bot: I haz the power, 2017-06-23 23:26:45]

Description was changed from

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

Design Doc Link:
https://docs.google.com/document/d/1p03qPe0mwkUcY9t9drbUucSDYsScgpIXxGPbDQIcv...

R=*dcheng@chromium.org,*holte@chromium.org,nasko@chromium.org,*oysteine@chrom...
BUG=724306
==========

to

==========
[GRC] UKM Support

This CL adds UKM support to GRC. Of particular interest is the
ability to create UkmEntryBuilders as the ResourceCoordinatorService
serves as the UKM client for all of the modules within GRC.

There is a software layering issue that prevents making UKM support
within GRC straightforward to implement. The UKM service currently
lives within the content_browser service which is out-of-scope for 
the resource_coordinator service to bind to. When UKM will be moved
into its own service is TBD, so as a workaround the
resource_coordinator_web_contents_observer is used to create the
UkmRecorderInterface which is then passed into the resource_coordinator
service through a newly added interface: ServiceCallbacks. The
ServiceCallbacks will also be used as a stopgap to solve the same
layering problem that may occur with services of interest to GRC. 

Currently, the only module, and main user of this capability, is the
CoordinationUnitManager. The intention is to provide an interface
for CoordinationUnitManagerObservers (CL link:
https://codereview.chromium.org/2926663003) to send metrics to UKM.
Note that the exact name for CoordinationUnitMangerObservers is still TBD.

Design Doc Link:
https://docs.google.com/document/d/1p03qPe0mwkUcY9t9drbUucSDYsScgpIXxGPbDQIcv...

R=*dcheng@chromium.org,*holte@chromium.org,nasko@chromium.org,*oysteine@chrom...
BUG=724306

Review-Url: https://codereview.chromium.org/2938443002
Cr-Commit-Position: refs/heads/master@{#482069}
Committed:
https://chromium.googlesource.com/chromium/src/+/097bcd379f843c5dcb8821d6049f...
==========

[commit-bot: I haz the power, 2017-06-23 23:26:46]

Committed patchset #31 (id:590001) as
https://chromium.googlesource.com/chromium/src/+/097bcd379f843c5dcb8821d6049f...