Expose UkmService to content/

Expose UkmService to content/

Expose UkmService to content so input metrics
can be reported via UKM like what the following
dependent CL does:
https://codereview.chromium.org/2888153002/

BUG=719604
Review-Url: https://codereview.chromium.org/2877673003
Cr-Commit-Position: refs/heads/master@{#474698}
Committed: https://chromium.googlesource.com/chromium/src/+/bbffc6cbcfa52fc2a8eec5b8ecc686f880541464

[Navid Zolghadr, 2017-05-11 15:47:25]

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

[Navid Zolghadr, 2017-05-11 15:47:26]

Oysteine, this is still a work in progress. But I seem to get a link error for
the functions I call from UKM Service (e.g. GetNewSourceID) in
render_widget_host_latency_tracker.cc. I believe I added the dependencies
similar to what I did back then for RAPPOR. Is there anything I'm missing here?

[oystein (OOO til 10th of July), 2017-05-11 22:12:10]

On 2017/05/11 at 15:47:26, nzolghadr wrote:
> Oysteine, this is still a work in progress. But I seem to get a link error for
the functions I call from UKM Service (e.g. GetNewSourceID) in
render_widget_host_latency_tracker.cc. I believe I added the dependencies
similar to what I did back then for RAPPOR. Is there anything I'm missing here?

Looks like the lib dependency is currently added to chrome/browser/BUILD.gn
instead of content/browser/BUILD.gn?

[Navid Zolghadr, 2017-05-12 14:25:22]

On 2017/05/11 22:12:10, oystein wrote:
> On 2017/05/11 at 15:47:26, nzolghadr wrote:
> > Oysteine, this is still a work in progress. But I seem to get a link error
for
> the functions I call from UKM Service (e.g. GetNewSourceID) in
> render_widget_host_latency_tracker.cc. I believe I added the dependencies
> similar to what I did back then for RAPPOR. Is there anything I'm missing
here?
> 
> Looks like the lib dependency is currently added to chrome/browser/BUILD.gn
> instead of content/browser/BUILD.gn?

Oh! You are right. Thanks for catching it. I was just unable to see this for a
full day.

[oystein (OOO til 10th of July), 2017-05-12 18:44:34]

On 2017/05/12 at 14:25:22, nzolghadr wrote:
> On 2017/05/11 22:12:10, oystein wrote:
> > On 2017/05/11 at 15:47:26, nzolghadr wrote:
> > > Oysteine, this is still a work in progress. But I seem to get a link error
for
> > the functions I call from UKM Service (e.g. GetNewSourceID) in
> > render_widget_host_latency_tracker.cc. I believe I added the dependencies
> > similar to what I did back then for RAPPOR. Is there anything I'm missing
here?
> > 
> > Looks like the lib dependency is currently added to chrome/browser/BUILD.gn
> > instead of content/browser/BUILD.gn?
> 
> Oh! You are right. Thanks for catching it. I was just unable to see this for a
full day.

Np, very easy thing to "be blind" to, I do it all the time :).

[Navid Zolghadr, 2017-05-15 14:58:32]

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

[commit-bot: I haz the power, 2017-05-15 14:59:08]

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-15 15:03:12]

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

[commit-bot: I haz the power, 2017-05-15 15:03:12]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  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_...)

[Navid Zolghadr, 2017-05-15 15:35:28]

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

[commit-bot: I haz the power, 2017-05-15 15:36:07]

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-15 15:42:06]

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

[commit-bot: I haz the power, 2017-05-15 15:42:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Navid Zolghadr, 2017-05-15 15:52:23]

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

[commit-bot: I haz the power, 2017-05-15 15:52:59]

Dry run: CQ is trying da patch.

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

[Navid Zolghadr, 2017-05-15 16:22:08]

nzolghadr@chromium.org changed reviewers:
+ tdresser@chromium.org

[Navid Zolghadr, 2017-05-15 16:22:08]

ptal.

[commit-bot: I haz the power, 2017-05-15 16:29:33]

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

[commit-bot: I haz the power, 2017-05-15 16:29:34]

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

[tdresser, 2017-05-17 14:24:10]

Can we split plumbing and reporting input metrics into separate patches?

[Navid Zolghadr, 2017-05-17 15:16:41]

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

[commit-bot: I haz the power, 2017-05-17 15:17:29]

Dry run: CQ is trying da patch.

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

[Navid Zolghadr, 2017-05-17 15:23:25]

Description was changed from

==========
Expose UkmService to content/

BUG=719604
==========

to

==========
Expose UkmService to content/

Expose UkmService to content to input metrics
can be reported via UKM like what the following
dependent CL does:
https://codereview.chromium.org/2888153002/

BUG=719604
==========

[Navid Zolghadr, 2017-05-17 17:36:55]

On 2017/05/17 14:24:10, tdresser wrote:
> Can we split plumbing and reporting input metrics into separate patches?

done.

[oystein (OOO til 10th of July), 2017-05-17 17:42:36]

lgtm

+holte

[oystein (OOO til 10th of July), 2017-05-17 17:45:56]

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

[oystein (OOO til 10th of July), 2017-05-17 17:46:23]

https://codereview.chromium.org/2883563002/ changes the source_id from int32 to
a ukm::SourceId typedef as an fyi

[oystein (OOO til 10th of July), 2017-05-17 17:48:08]

On 2017/05/17 at 17:46:23, oystein wrote:
> https://codereview.chromium.org/2883563002/ changes the source_id from int32
to a ukm::SourceId typedef as an fyi

Actually I would just wait for that patch for land, since it'll require some
more rebasing.

[Navid Zolghadr, 2017-05-17 18:00:17]

On 2017/05/17 17:48:08, oystein wrote:
> On 2017/05/17 at 17:46:23, oystein wrote:
> > https://codereview.chromium.org/2883563002/ changes the source_id from int32
> to a ukm::SourceId typedef as an fyi
> 
> Actually I would just wait for that patch for land, since it'll require some
> more rebasing.

Sure. I'll make sure to wait til that's merged and rebase.

[commit-bot: I haz the power, 2017-05-17 19:10:51]

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

[commit-bot: I haz the power, 2017-05-17 19:10:51]

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

[Navid Zolghadr, 2017-05-19 18:33:05]

nzolghadr@chromium.org changed reviewers:
+ creis@chromium.org

[Navid Zolghadr, 2017-05-19 18:33:05]

creis@chromium.org: Please review changes in 

content/*

[Navid Zolghadr, 2017-05-24 15:18:09]

On 2017/05/19 18:33:05, Navid Zolghadr wrote:
> mailto:creis@chromium.org: Please review changes in 
> 
> content/*

ping

[Charlie Reis, 2017-05-24 16:10:23]

Description was changed from

==========
Expose UkmService to content/

Expose UkmService to content to input metrics
can be reported via UKM like what the following
dependent CL does:
https://codereview.chromium.org/2888153002/

BUG=719604
==========

to

==========
Expose UkmService to content/

Expose UkmService to content so input metrics
can be reported via UKM like what the following
dependent CL does:
https://codereview.chromium.org/2888153002/

BUG=719604
==========

[Charlie Reis, 2017-05-24 16:28:31]

creis@chromium.org changed reviewers:
+ nick@chromium.org

[Charlie Reis, 2017-05-24 16:28:32]

[+nick]

On 2017/05/24 15:18:09, Navid Zolghadr wrote:
> On 2017/05/19 18:33:05, Navid Zolghadr wrote:
> > mailto:creis@chromium.org: Please review changes in 
> > 
> > content/*
> 
> ping

Thanks-- sorry that this fell through the cracks.

A few high level concerns, but once those are resolved this should be fine.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS#ne...
content/browser/DEPS:18: "+components/ukm",
Nick and I have concerns that other content embedders may not want to depend on
this entire component.  Would it be possible to do something like
components/rappor/public where content just depends on an interface that an
embedder can stub out if desired?

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:247: // Update
UkmService for the given source with the URL. Returns false
Can you elaborate what UkmService is here?  I didn't know it was URL-keyed
metrics until I dug into the code.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:248: // if the Utl
is not known for |render_widget_host|.
What is Utl?

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:249: virtual bool
UpdateUrlForUkmSource(ukm::UkmService* service,
This doesn't look like it ever returns false in practice, and the proposed call
site in RenderWidgetHostLatencyTracker::GetUkmSourceId() (in
https://codereview.chromium.org/2888153002) doesn't check the return value. 
Maybe it should just be void?

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:5582:
service->UpdateSourceURL(ukm_source_id, GetLastCommittedURL());
I'm assuming this intentionally only uses the main frame URL, even if the
observed actions may be coming from subframes (possibly from a different
process)?  If so, let's leave a comment to that effect here, so that we don't
worry about this being a bug for subframes or OOPIFs.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:707: bool
UpdateUrlForUkmSource(ukm::UkmService* service,
Might need to forward declare UkmService (IWYU).

[Navid Zolghadr, 2017-05-24 16:41:54]

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS#ne...
content/browser/DEPS:18: "+components/ukm",
On 2017/05/24 16:28:31, Charlie Reis wrote:
> Nick and I have concerns that other content embedders may not want to depend
on
> this entire component.  Would it be possible to do something like
> components/rappor/public where content just depends on an interface that an
> embedder can stub out if desired?

I knew that was coming. :)
I did that rappor/public myself as well. I'll start doing it. That'll take a
while.

[Charlie Reis, 2017-05-24 16:46:25]

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS#ne...
content/browser/DEPS:18: "+components/ukm",
On 2017/05/24 16:41:54, Navid Zolghadr wrote:
> On 2017/05/24 16:28:31, Charlie Reis wrote:
> > Nick and I have concerns that other content embedders may not want to depend
> on
> > this entire component.  Would it be possible to do something like
> > components/rappor/public where content just depends on an interface that an
> > embedder can stub out if desired?
> 
> I knew that was coming. :)
> I did that rappor/public myself as well. I'll start doing it. That'll take a
> while.

Heh, apologies for the extra work, but I think it'll be worthwhile.  :)

[oystein (OOO til 10th of July), 2017-05-24 17:21:29]

On 2017/05/24 at 16:46:25, creis wrote:
> https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS
> File content/browser/DEPS (right):
> 
>
https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS#ne...
> content/browser/DEPS:18: "+components/ukm",
> On 2017/05/24 16:41:54, Navid Zolghadr wrote:
> > On 2017/05/24 16:28:31, Charlie Reis wrote:
> > > Nick and I have concerns that other content embedders may not want to
depend
> > on
> > > this entire component.  Would it be possible to do something like
> > > components/rappor/public where content just depends on an interface that
an
> > > embedder can stub out if desired?
> > 
> > I knew that was coming. :)
> > I did that rappor/public myself as well. I'll start doing it. That'll take a
> > while.
> 
> Heh, apologies for the extra work, but I think it'll be worthwhile.  :)

A CL to do just this landed a couple of days ago:
https://codereview.chromium.org/2893943004

Some rebasing will need to be done, but it should help :).

[Navid Zolghadr, 2017-05-25 15:45:29]

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

[commit-bot: I haz the power, 2017-05-25 15:45:46]

Dry run: CQ is trying da patch.

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

[Navid Zolghadr, 2017-05-25 15:45:46]

ptal.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/DEPS#ne...
content/browser/DEPS:18: "+components/ukm",
On 2017/05/24 16:46:24, Charlie Reis (overloaded) wrote:
> On 2017/05/24 16:41:54, Navid Zolghadr wrote:
> > On 2017/05/24 16:28:31, Charlie Reis wrote:
> > > Nick and I have concerns that other content embedders may not want to
depend
> > on
> > > this entire component.  Would it be possible to do something like
> > > components/rappor/public where content just depends on an interface that
an
> > > embedder can stub out if desired?
> > 
> > I knew that was coming. :)
> > I did that rappor/public myself as well. I'll start doing it. That'll take a
> > while.
> 
> Heh, apologies for the extra work, but I think it'll be worthwhile.  :)

I didn't realized the other changed already added that public folder stuff in
ukm. Alright. So I just did a rebase and a few renames and it is all good now.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_delegate.h (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:247: // Update
UkmService for the given source with the URL. Returns false
On 2017/05/24 16:28:32, Charlie Reis (overloaded) wrote:
> Can you elaborate what UkmService is here?  I didn't know it was URL-keyed
> metrics until I dug into the code.

Done.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:248: // if the Utl
is not known for |render_widget_host|.
On 2017/05/24 16:28:32, Charlie Reis (overloaded) wrote:
> What is Utl?

Done.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_delegate.h:249: virtual bool
UpdateUrlForUkmSource(ukm::UkmService* service,
On 2017/05/24 16:28:32, Charlie Reis (overloaded) wrote:
> This doesn't look like it ever returns false in practice, and the proposed
call
> site in RenderWidgetHostLatencyTracker::GetUkmSourceId() (in
> https://codereview.chromium.org/2888153002) doesn't check the return value. 
> Maybe it should just be void?

Done.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:5582:
service->UpdateSourceURL(ukm_source_id, GetLastCommittedURL());
On 2017/05/24 16:28:32, Charlie Reis (overloaded) wrote:
> I'm assuming this intentionally only uses the main frame URL, even if the
> observed actions may be coming from subframes (possibly from a different
> process)?  If so, let's leave a comment to that effect here, so that we don't
> worry about this being a bug for subframes or OOPIFs.

Done.

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.h (right):

https://codereview.chromium.org/2877673003/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.h:707: bool
UpdateUrlForUkmSource(ukm::UkmService* service,
On 2017/05/24 16:28:32, Charlie Reis (overloaded) wrote:
> Might need to forward declare UkmService (IWYU).

Done.

[Navid Zolghadr, 2017-05-25 16:20:54]

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

[commit-bot: I haz the power, 2017-05-25 16:21:09]

Dry run: CQ is trying da patch.

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

[Charlie Reis, 2017-05-25 17:10:13]

Thanks!  LGTM.

[commit-bot: I haz the power, 2017-05-25 17:43:18]

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

[commit-bot: I haz the power, 2017-05-25 17:43:19]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2017-05-25 17:49:08]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2017-05-25 17:49:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/2877673003/#ps120001
(title: "Use ukm::SourceId instead of int32")

[commit-bot: I haz the power, 2017-05-25 17:49:46]

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-25 17:56:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495734548484250,
"parent_rev": "7848fec2384849ec0663c96d3bc73e9e3afd6e25", "commit_rev":
"bbffc6cbcfa52fc2a8eec5b8ecc686f880541464"}

[commit-bot: I haz the power, 2017-05-25 17:56:56]

Description was changed from

==========
Expose UkmService to content/

Expose UkmService to content so input metrics
can be reported via UKM like what the following
dependent CL does:
https://codereview.chromium.org/2888153002/

BUG=719604
==========

to

==========
Expose UkmService to content/

Expose UkmService to content so input metrics
can be reported via UKM like what the following
dependent CL does:
https://codereview.chromium.org/2888153002/

BUG=719604

Review-Url: https://codereview.chromium.org/2877673003
Cr-Commit-Position: refs/heads/master@{#474698}
Committed:
https://chromium.googlesource.com/chromium/src/+/bbffc6cbcfa52fc2a8eec5b8ecc6...
==========

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

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bbffc6cbcfa52fc2a8eec5b8ecc6...