Refactor UKM interface for mojo-ification

Refactor UKM interface for mojo-ification

* ukm::UkmEntry replaced by ukm::mojom::UkmEntry
 - Several tests updated to look at these entries instead of protos
* Created an interface class and updated recording code to use it:
  UkmService -> UkmRecorder.
* Moved code need for implementing that interface from
  UkmService -> UkmRecorderImpl.
* Replaced TestUkmService with TestUkmRecorder, and removed the harness
  code which became unnecessary.
* Moved UkmEntryBuilder to components/ukm/public/

TBR=sebsg,yzshen1,sky,groby
BUG=

[Steven Holte, 2017-05-15 19:48:51]

Description was changed from

==========
Mojo interface for UKM

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

to

==========
Mojo interface for UKM

* ukm::UkmEntry replaced by ukm::mojom::UkmEntry
 - Several tests updated to look at these entries instead of protos
* Created an interface class and updated recording code to use it:
  UkmService -> UkmRecorder.
* Moved code need for implementing that interface from
  UkmService -> UkmRecorderImpl.
* Replaced TestUkmService with TestUkmRecorder, and removed the harness
  code which became unnecessary.
* Moved UkmEntryBuilder to components/ukm/public/
* Added Mojo RPC code, with a helper wrapper:
  - MojoUkmRecorder wraps ukm::mojom::UkmRecorderInterface
  - ukm::UkmInterface implements ukm::mojom::UkmRecorderInterface


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

[Steven Holte, 2017-05-15 20:07:30]

holte@chromium.org changed reviewers:
+ oysteine@chromium.org

[Steven Holte, 2017-05-15 20:07:31]

oysteine, here's a first draft of the Mojo interface for UKM.

I'll probably split this CL into two parts, one being the interface refactor and
the other being the addition of the mojo bits, but the end result should be
basically the same.

I'm getting some errors about 
"Connection InterfaceProviderSpec prevented service: content_renderer from
binding interface: ukm::mojom::UkmRecorderInterface exposed by: content_browser"
when attempting to record the sample metrics.  I would have thought that was
just a chrome_content_browser_manifest_overlay.json issue, but that doesn't seem
to be enough.  Any idea what I'm missing?

[oystein (OOO til 10th of July), 2017-05-15 22:13:26]

On 2017/05/15 at 20:07:31, holte wrote:
> oysteine, here's a first draft of the Mojo interface for UKM.
> 
> I'll probably split this CL into two parts, one being the interface refactor
and the other being the addition of the mojo bits, but the end result should be
basically the same.
> 
> I'm getting some errors about 
> "Connection InterfaceProviderSpec prevented service: content_renderer from
binding interface: ukm::mojom::UkmRecorderInterface exposed by: content_browser"
when attempting to record the sample metrics.  I would have thought that was
just a chrome_content_browser_manifest_overlay.json issue, but that doesn't seem
to be enough.  Any idea what I'm missing?

This is awesome! I'll go through it in more detail later today/tomorrow, but to
answer the question: I think it's because you're accessing the navigation:frame
interface provider when you call GetRemoteInterfaces() in RenderFrameImpl, i.e.
frame-specific interfaces, see
https://cs.chromium.org/chromium/src/ipc/README.md?type=cs&q=GetRemoteInterfa...

I think the recommended approach now is to go through the
service_manager::Connector now though, see
https://cs.chromium.org/chromium/src/chrome/renderer/net_benchmarking_extensi...
for an example.

[Steven Holte, 2017-05-15 23:19:39]

Description was changed from

==========
Mojo interface for UKM

* ukm::UkmEntry replaced by ukm::mojom::UkmEntry
 - Several tests updated to look at these entries instead of protos
* Created an interface class and updated recording code to use it:
  UkmService -> UkmRecorder.
* Moved code need for implementing that interface from
  UkmService -> UkmRecorderImpl.
* Replaced TestUkmService with TestUkmRecorder, and removed the harness
  code which became unnecessary.
* Moved UkmEntryBuilder to components/ukm/public/
* Added Mojo RPC code, with a helper wrapper:
  - MojoUkmRecorder wraps ukm::mojom::UkmRecorderInterface
  - ukm::UkmInterface implements ukm::mojom::UkmRecorderInterface


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

to

==========
Refactor UKM interface for mojo-ification

* ukm::UkmEntry replaced by ukm::mojom::UkmEntry
 - Several tests updated to look at these entries instead of protos
* Created an interface class and updated recording code to use it:
  UkmService -> UkmRecorder.
* Moved code need for implementing that interface from
  UkmService -> UkmRecorderImpl.
* Replaced TestUkmService with TestUkmRecorder, and removed the harness
  code which became unnecessary.
* Moved UkmEntryBuilder to components/ukm/public/


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

[Steven Holte, 2017-05-15 23:20:52]

On 2017/05/15 22:13:26, oystein wrote:
> On 2017/05/15 at 20:07:31, holte wrote:
> > oysteine, here's a first draft of the Mojo interface for UKM.
> > 
> > I'll probably split this CL into two parts, one being the interface refactor
> and the other being the addition of the mojo bits, but the end result should
be
> basically the same.
> > 
> > I'm getting some errors about 
> > "Connection InterfaceProviderSpec prevented service: content_renderer from
> binding interface: ukm::mojom::UkmRecorderInterface exposed by:
content_browser"
> when attempting to record the sample metrics.  I would have thought that was
> just a chrome_content_browser_manifest_overlay.json issue, but that doesn't
seem
> to be enough.  Any idea what I'm missing?
> 
> This is awesome! I'll go through it in more detail later today/tomorrow, but
to
> answer the question: I think it's because you're accessing the
navigation:frame
> interface provider when you call GetRemoteInterfaces() in RenderFrameImpl,
i.e.
> frame-specific interfaces, see
>
https://cs.chromium.org/chromium/src/ipc/README.md?type=cs&q=GetRemoteInterfa...
> 
> I think the recommended approach now is to go through the
> service_manager::Connector now though, see
>
https://cs.chromium.org/chromium/src/chrome/renderer/net_benchmarking_extensi...
> for an example.

Split the Mojo interface implementation to
https://codereview.chromium.org/2882353002/ using service_manager::Connector

[Steven Holte, 2017-05-17 17:53:30]

holte@chromium.org changed reviewers:
+ yzshen@chromium.org

[Steven Holte, 2017-05-17 17:53:31]

+yzshen for DEP on mojo/public from components/ukm

Note the follow up to actually expose the interface is in this CL:
https://codereview.chromium.org/2882353002/

[oystein (OOO til 10th of July), 2017-05-17 18:25:04]

oysteine@chromium.org changed reviewers:
- yzshen@chromium.org

[oystein (OOO til 10th of July), 2017-05-17 18:25:07]

lgtm w/ nits, nice!

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/public/u...
File components/ukm/public/ukm_recorder.h (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/public/u...
components/ukm/public/ukm_recorder.h:23: }  // namespace autofill
nit: no need for the //namespace comment for forward declares i think.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/public/u...
components/ukm/public/ukm_recorder.h:35: }  // namespace payments
nit: same as above about redundant comment.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
File components/ukm/test_ukm_recorder.cc (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:29: source = kv.second.get();
Right now we'll keep looping and return the last matching URL, if there's
several; is that intentional or can we just return the first one? If not maybe
the function should specify this (i.e. "GetLastMatchingSourceForUrl".

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:41: source = kv.second.get();
Same comment as above.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:48: return entries()[entry_num].get();
DCHECK(entry_num < entries().size());

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:54: if (it->event_hash ==
base::HashMetricName(entry_name))
Same as above; in this case we're returning the first matching rather than the
last; intentional difference?

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
File components/ukm/test_ukm_recorder.h (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.h:12: 
Should have #include "base/macros.h" here I think.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.h:18: explicit TestUkmRecorder();
nit: No need for explicit here.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.h:22: const std::map<ukm::SourceId,
std::unique_ptr<UkmSource>>& GetSources() const;
Why isn't this inlined, like the others, if it's basically just forwarding the
call to the protected version of the function?

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
File components/ukm/ukm_recorder_impl.cc (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.cc:173: }
nit: Some inconsistency in whether single-line blocks are wrapped in {} or not
(with the if() on :105), should keep one or the other.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
File components/ukm/ukm_recorder_impl.h (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.h:46: // UkmRecorder:
nit: "UkmRecorder implementation:" for consistency

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.h:61: THREAD_CHECKER(thread_checker_);
nit: missing some includes for this (and the STL stuff) I think.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.h:63: }
nit: // namespace ukm

[Steven Holte, 2017-05-17 20:44:32]

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/public/u...
File components/ukm/public/ukm_recorder.h (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/public/u...
components/ukm/public/ukm_recorder.h:23: }  // namespace autofill
On 2017/05/17 18:25:06, oystein wrote:
> nit: no need for the //namespace comment for forward declares i think.

Done.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/public/u...
components/ukm/public/ukm_recorder.h:35: }  // namespace payments
On 2017/05/17 18:25:06, oystein wrote:
> nit: same as above about redundant comment. 

Done.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
File components/ukm/test_ukm_recorder.cc (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:29: source = kv.second.get();
On 2017/05/17 18:25:06, oystein wrote:
> Right now we'll keep looping and return the last matching URL, if there's
> several; is that intentional or can we just return the first one? If not maybe
> the function should specify this (i.e. "GetLastMatchingSourceForUrl".

I've just renamed this from the same function in TestUkmService.  Since this has
a DCHECK_EQ(nullptr, source), it is actually requiring that looking up the
source this way is unique, and loops over the whole thing to check that.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:41: source = kv.second.get();
On 2017/05/17 18:25:06, oystein wrote:
> Same comment as above.

Ditto.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:48: return entries()[entry_num].get();
On 2017/05/17 18:25:06, oystein wrote:
> DCHECK(entry_num < entries().size());

Done.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.cc:54: if (it->event_hash ==
base::HashMetricName(entry_name))
On 2017/05/17 18:25:06, oystein wrote:
> Same as above; in this case we're returning the first matching rather than the
> last; intentional difference?

I think this one just isn't bothering with the DCHECK, but I haven't really
changed it in this CL.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
File components/ukm/test_ukm_recorder.h (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.h:12: 
On 2017/05/17 18:25:06, oystein wrote:
> Should have #include "base/macros.h" here I think.

Done.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.h:18: explicit TestUkmRecorder();
On 2017/05/17 18:25:06, oystein wrote:
> nit: No need for explicit here.

Done.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/test_ukm...
components/ukm/test_ukm_recorder.h:22: const std::map<ukm::SourceId,
std::unique_ptr<UkmSource>>& GetSources() const;
On 2017/05/17 18:25:06, oystein wrote:
> Why isn't this inlined, like the others, if it's basically just forwarding the
> call to the protected version of the function?

inlined

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
File components/ukm/ukm_recorder_impl.cc (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.cc:173: }
On 2017/05/17 18:25:06, oystein wrote:
> nit: Some inconsistency in whether single-line blocks are wrapped in {} or not
> (with the if() on :105), should keep one or the other.

Removed it here.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
File components/ukm/ukm_recorder_impl.h (right):

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.h:46: // UkmRecorder:
On 2017/05/17 18:25:06, oystein wrote:
> nit: "UkmRecorder implementation:" for consistency

Not including the word implementation seems to be more consistent with the how
overrides are commented in existing components/metrics & components/ukm code, so
leaving as is.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.h:61: THREAD_CHECKER(thread_checker_);
On 2017/05/17 18:25:06, oystein wrote:
> nit: missing some includes for this (and the STL stuff) I think.

Done.

https://codereview.chromium.org/2883563002/diff/40001/components/ukm/ukm_reco...
components/ukm/ukm_recorder_impl.h:63: }
On 2017/05/17 18:25:06, oystein wrote:
> nit: // namespace ukm

Done.

[Steven Holte, 2017-05-17 20:45:37]

holte@chromium.org changed reviewers:
+ yzshen@chromium.org

[Steven Holte, 2017-05-17 20:50:00]

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

[commit-bot: I haz the power, 2017-05-17 20:50:52]

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-05-17 20:59:32]

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

[commit-bot: I haz the power, 2017-05-17 20:59:34]

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_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Steven Holte, 2017-05-17 21:55:27]

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

[commit-bot: I haz the power, 2017-05-17 21:57:05]

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-05-17 22:09:16]

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

[commit-bot: I haz the power, 2017-05-17 22:09:17]

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

[Steven Holte, 2017-05-17 22:26:51]

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

[commit-bot: I haz the power, 2017-05-17 22:28:22]

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-05-17 22:43:02]

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

[commit-bot: I haz the power, 2017-05-17 22:43:03]

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

[Steven Holte, 2017-05-17 22:51:01]

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

[commit-bot: I haz the power, 2017-05-17 22:52:52]

Dry run: CQ is trying da patch.

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

[Steven Holte, 2017-05-17 23:00:16]

holte@chromium.org changed reviewers:
+ groby@chromium.org, sebsg@chromium.org, sky@chromium.org

[Steven Holte, 2017-05-17 23:00:17]

+sky for browser process ukm_service -> ukm_recorder accessors

OWNERS for UKM metrics
+sebsg for autofill + payments
+groby for translate

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

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

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

Dry run: 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-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Steven Holte, 2017-05-17 23:10:11]

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

[commit-bot: I haz the power, 2017-05-17 23:11:59]

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-05-17 23:26:38]

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

[commit-bot: I haz the power, 2017-05-17 23:26:40]

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

[Steven Holte, 2017-05-17 23:31:45]

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

[commit-bot: I haz the power, 2017-05-17 23:34:33]

Dry run: CQ is trying da patch.

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

[Steven Holte, 2017-05-17 23:54:34]

Description was changed from

==========
Refactor UKM interface for mojo-ification

* ukm::UkmEntry replaced by ukm::mojom::UkmEntry
 - Several tests updated to look at these entries instead of protos
* Created an interface class and updated recording code to use it:
  UkmService -> UkmRecorder.
* Moved code need for implementing that interface from
  UkmService -> UkmRecorderImpl.
* Replaced TestUkmService with TestUkmRecorder, and removed the harness
  code which became unnecessary.
* Moved UkmEntryBuilder to components/ukm/public/


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

to

==========
Refactor UKM interface for mojo-ification

* ukm::UkmEntry replaced by ukm::mojom::UkmEntry
 - Several tests updated to look at these entries instead of protos
* Created an interface class and updated recording code to use it:
  UkmService -> UkmRecorder.
* Moved code need for implementing that interface from
  UkmService -> UkmRecorderImpl.
* Replaced TestUkmService with TestUkmRecorder, and removed the harness
  code which became unnecessary.
* Moved UkmEntryBuilder to components/ukm/public/

TBR=sebsg,yzshen1,sky,groby
BUG=
==========

[commit-bot: I haz the power, 2017-05-18 00:09:07]

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

[commit-bot: I haz the power, 2017-05-18 00:09:08]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sebsg, 2017-05-18 15:54:10]

That's pretty nice!

Autofill & payments/journey_logger lgtm % a very small comment.

https://codereview.chromium.org/2883563002/diff/140002/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2883563002/diff/140002/components/autofill/co...
components/autofill/core/browser/autofill_metrics.h:24: class UkmRecorder;
not needed anymore because of your include of ukm_recorder.h?

[sky, 2017-05-18 16:56:26]

LGTM

[oystein (OOO til 10th of July), 2017-05-18 22:06:30]

ping groby@ and @yzshen

holte@ went OOO so I'm taking over landing his CL; new patchsets in
https://codereview.chromium.org/2893943004 and I'll TBR lgtms done in this CL
there.

https://codereview.chromium.org/2883563002/diff/140002/components/autofill/co...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2883563002/diff/140002/components/autofill/co...
components/autofill/core/browser/autofill_metrics.h:24: class UkmRecorder;
On 2017/05/18 at 15:54:10, sebsg wrote:
> not needed anymore because of your include of ukm_recorder.h?

Done.

[yzshen1, 2017-05-18 22:21:18]

On 2017/05/18 22:06:30, oystein wrote:
> ping groby@ and @yzshen
> 
> holte@ went OOO so I'm taking over landing his CL; new patchsets in
> https://codereview.chromium.org/2893943004 and I'll TBR lgtms done in this CL
> there.
> 
>
https://codereview.chromium.org/2883563002/diff/140002/components/autofill/co...
> File components/autofill/core/browser/autofill_metrics.h (right):
> 
>
https://codereview.chromium.org/2883563002/diff/140002/components/autofill/co...
> components/autofill/core/browser/autofill_metrics.h:24: class UkmRecorder;
> On 2017/05/18 at 15:54:10, sebsg wrote:
> > not needed anymore because of your include of ukm_recorder.h?
> 
> Done.

mojo/public DEPS LGTM

[groby-ooo-7-16, 2017-05-22 17:22:57]

components/translate LGTM as a purely mechanical change

cc hamelphi as FYI, since ranker is only nominally something I own :)

[oystein (OOO til 10th of July), 2017-05-22 21:45:42]

Thanks everyone,

Closing this as I'm landing (and TBRing existing LGTMs) in
https://codereview.chromium.org/2893943004 on behalf of holte@ who's OOO.