Convert first contentful paint logging to the new UKM client API

Convert first contentful paint logging to the new UKM client API

This change switches FCP logging from the Source proto to the new
UKM client API.

This change also does the following:
* extends the UKM test infrastructure slightly, in order to enable testing
  of metrics that use thew new client API.
* renames committed_url to url in the client-side code, to match the proto
* stops populating the first_contentful_paint source field and marks it as
  deprecated, as FCP is now recorded using the new client API.

BUG=697454
Review-Url: https://codereview.chromium.org/2719823003
Cr-Commit-Position: refs/heads/master@{#454262}
Committed: https://chromium.googlesource.com/chromium/src/+/117fb7823a566d83abc3db2d6f1cd06825e67d03

[Bryan McQuade, 2017-02-27 21:56:50]

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

[commit-bot: I haz the power, 2017-02-27 21:57:53]

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

[Bryan McQuade, 2017-02-27 22:04:23]

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

[commit-bot: I haz the power, 2017-02-27 22:05:15]

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

[commit-bot: I haz the power, 2017-02-27 22:13:12]

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

[commit-bot: I haz the power, 2017-02-27 22:13:13]

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

[Zhen Wang, 2017-02-27 23:19:23]

zhenw@chromium.org changed reviewers:
+ zhenw@chromium.org

[Zhen Wang, 2017-02-27 23:19:24]

The usage of UKM interface looks ok to me. The build dependency issue should be
fixed in my CL now (except for ios).

[Bryan McQuade, 2017-02-28 01:47:38]

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

[commit-bot: I haz the power, 2017-02-28 01:48:10]

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

[commit-bot: I haz the power, 2017-02-28 01:57:41]

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

[commit-bot: I haz the power, 2017-02-28 01:57:42]

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

[Bryan McQuade, 2017-02-28 02:04:48]

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

[commit-bot: I haz the power, 2017-02-28 02:05:43]

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

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

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

[commit-bot: I haz the power, 2017-02-28 02:14:07]

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

[Bryan McQuade, 2017-02-28 03:08:11]

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

[commit-bot: I haz the power, 2017-02-28 03:08:50]

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

[Bryan McQuade, 2017-02-28 03:10:51]

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

[commit-bot: I haz the power, 2017-02-28 03:11:00]

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

[Bryan McQuade, 2017-02-28 03:25:04]

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

[commit-bot: I haz the power, 2017-02-28 03:25:15]

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

[Bryan McQuade, 2017-02-28 03:35:06]

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

[commit-bot: I haz the power, 2017-02-28 03:35:23]

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

[commit-bot: I haz the power, 2017-02-28 04:20:06]

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

[commit-bot: I haz the power, 2017-02-28 04:20:07]

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

[Bryan McQuade, 2017-02-28 13:52:46]

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

[commit-bot: I haz the power, 2017-02-28 13:53:06]

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

[Bryan McQuade, 2017-02-28 13:56:34]

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

[commit-bot: I haz the power, 2017-02-28 13:56:50]

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

[commit-bot: I haz the power, 2017-02-28 14:03:15]

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

[commit-bot: I haz the power, 2017-02-28 14:03:16]

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

[Bryan McQuade, 2017-03-01 03:56:03]

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

[commit-bot: I haz the power, 2017-03-01 03:56:23]

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-03-01 04:04:32]

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

[commit-bot: I haz the power, 2017-03-01 04:04:33]

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

[Bryan McQuade, 2017-03-01 14:56:42]

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

[commit-bot: I haz the power, 2017-03-01 14:56:50]

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

[Bryan McQuade, 2017-03-01 15:06:52]

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

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

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

[Bryan McQuade, 2017-03-01 15:07:18]

Description was changed from

==========
Add additional UKM metrics using new UKM client API.

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

to

==========
Add additional PageLoad UKM metrics using new UKM client API.

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

[Bryan McQuade, 2017-03-01 15:11:19]

Description was changed from

==========
Add additional PageLoad UKM metrics using new UKM client API.

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

to

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

Additionally, this change stops populating the first_contentful_paint source
field and marks it as deprecated, as FCP is now set using the new client API.

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

[Bryan McQuade, 2017-03-01 15:13:29]

Description was changed from

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

Additionally, this change stops populating the first_contentful_paint source
field and marks it as deprecated, as FCP is now set using the new client API.

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

to

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

Additionally, this change stops populating the first_contentful_paint source
field and marks it as deprecated, as FCP is now set using the new client API.

BUG=697454
==========

[Bryan McQuade, 2017-03-01 15:14:00]

Description was changed from

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

Additionally, this change stops populating the first_contentful_paint source
field and marks it as deprecated, as FCP is now set using the new client API.

BUG=697454
==========

to

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

This change also extends the UKM test infrastructure slightly, in order to
enable testing of metrics that use thew new client API.

Additionally, this change stops populating the first_contentful_paint source
field and marks it as deprecated, as FCP is now set using the new client API.

BUG=697454
==========

[Bryan McQuade, 2017-03-01 15:30:44]

Description was changed from

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

This change also extends the UKM test infrastructure slightly, in order to
enable testing of metrics that use thew new client API.

Additionally, this change stops populating the first_contentful_paint source
field and marks it as deprecated, as FCP is now set using the new client API.

BUG=697454
==========

to

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

This change also does the following:
* extends the UKM test infrastructure slightly, in order to enable testing
  of metrics that use thew new client API.
* renames committed_url to url in the client-side code, to match the proto
* stops populating the first_contentful_paint source field and marks it as
  deprecated, as FCP is now recorded using the new client API.

BUG=697454
==========

[commit-bot: I haz the power, 2017-03-01 16:42:47]

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

[commit-bot: I haz the power, 2017-03-01 16:42:48]

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

[Zhen Wang, 2017-03-01 17:06:57]

By the way, should we separate this CL into 2: one to change the use of the UKM
API and the other to add metrics?

[Bryan McQuade, 2017-03-01 17:20:24]

On 2017/03/01 at 17:06:57, zhenw wrote:
> By the way, should we separate this CL into 2: one to change the use of the
UKM API and the other to add metrics?

We can split it if you want, sure. Does that make it easier to review?

[Zhen Wang, 2017-03-01 17:37:34]

On 2017/03/01 17:20:24, Bryan McQuade wrote:
> On 2017/03/01 at 17:06:57, zhenw wrote:
> > By the way, should we separate this CL into 2: one to change the use of the
> UKM API and the other to add metrics?
> 
> We can split it if you want, sure. Does that make it easier to review?

That would be best. Thanks!

[Bryan McQuade, 2017-03-01 18:21:03]

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

[commit-bot: I haz the power, 2017-03-01 18:21:33]

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

[Bryan McQuade, 2017-03-01 18:22:24]

Description was changed from

==========
Add additional PageLoad UKM metrics using new UKM client API.

This change adds core PageLoad metrics to UKM. The new metrics are:
* first contentful paint
* first meaningful paint (experimental)
* foreground duration
* parse start
* dom content loaded
* load event

This change also does the following:
* extends the UKM test infrastructure slightly, in order to enable testing
  of metrics that use thew new client API.
* renames committed_url to url in the client-side code, to match the proto
* stops populating the first_contentful_paint source field and marks it as
  deprecated, as FCP is now recorded using the new client API.

BUG=697454
==========

to

==========
Convert first contentful paint logging to the new UKM client API

This change switches FCP logging from the Source proto to the new
UKM client API.

This change also does the following:
* extends the UKM test infrastructure slightly, in order to enable testing
  of metrics that use thew new client API.
* renames committed_url to url in the client-side code, to match the proto
* stops populating the first_contentful_paint source field and marks it as
  deprecated, as FCP is now recorded using the new client API.

BUG=697454
==========

[Bryan McQuade, 2017-03-01 18:26:02]

I limited this change to porting FCP over to the new API. PTAL, thanks!

[Bryan McQuade, 2017-03-01 18:56:23]

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

[Bryan McQuade, 2017-03-01 18:57:48]

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

[Bryan McQuade, 2017-03-01 18:57:49]

rkaplow, holte, zhenw, PTAL, thanks!

[Zhen Wang, 2017-03-01 19:18:00]

https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:60:
std::unique_ptr<ukm::UkmEntryBuilder> builder_;
Builder will only add UkmEntry to UkmService when it is destructed. I remember
the observer's life ends when another navigation in the same frame commits. I am
not sure if you want to wait that long. Is that desired?

[Bryan McQuade, 2017-03-01 19:21:31]

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

[commit-bot: I haz the power, 2017-03-01 19:21:52]

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

[Bryan McQuade, 2017-03-01 19:26:16]

On 2017/03/01 at 19:18:00, zhenw wrote:
>
https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
> File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
(right):
> 
>
https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
>
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:60:
std::unique_ptr<ukm::UkmEntryBuilder> builder_;
> Builder will only add UkmEntry to UkmService when it is destructed. I remember
the observer's life ends when another navigation in the same frame commits. I am
not sure if you want to wait that long. Is that desired?

Good question. The default lifetime of an observer is until the next page load
commits. Observers can eagerly decide to be destroyed by returning
STOP_OBSERVING from some methods, which this one does, from OnHidden and others.

We could try to be even more aggressive and keep a builder scoped to the
lifetime of the OnFirstContentfulPaint callback, which would work fine in this
version of the change, but we'd undo that in the follow up change that adds
additional metrics such as total time on page, which do want to wait until the
end of the page lifetime. So we might as well keep things simple and have a
builder scoped to the lifetime of the observer, to enable that case.

By delaying logging here, the only risk we run is that the browser process
crashes after we observe an event, but before the page load completes. This
should be very rare and I think it's acceptable to use a simpler logging
approach with a single builder scoped to the lifetime while accepting the slight
risk of browser crashes.

You're right that historically, waiting to log metrics was a big issue for page
load metrics, due to app kills on Android, but with the newer
FlushMetricsOnAppEnterBackground callback, which this observer implements, we're
able to flush all metrics out before getting killed on Android, which addresses
that issue.

[commit-bot: I haz the power, 2017-03-01 20:49:00]

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

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

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-03-01 21:49:52]

On 2017/03/01 19:26:16, Bryan McQuade wrote:
> On 2017/03/01 at 19:18:00, zhenw wrote:
> >
>
https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
> > File
> chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
> (right):
> > 
> >
>
https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
> >
>
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:60:
> std::unique_ptr<ukm::UkmEntryBuilder> builder_;
> > Builder will only add UkmEntry to UkmService when it is destructed. I
remember
> the observer's life ends when another navigation in the same frame commits. I
am
> not sure if you want to wait that long. Is that desired?
> 
> Good question. The default lifetime of an observer is until the next page load
> commits. Observers can eagerly decide to be destroyed by returning
> STOP_OBSERVING from some methods, which this one does, from OnHidden and
others.
> 
> We could try to be even more aggressive and keep a builder scoped to the
> lifetime of the OnFirstContentfulPaint callback, which would work fine in this
> version of the change, but we'd undo that in the follow up change that adds
> additional metrics such as total time on page, which do want to wait until the
> end of the page lifetime. So we might as well keep things simple and have a
> builder scoped to the lifetime of the observer, to enable that case.
> 
> By delaying logging here, the only risk we run is that the browser process
> crashes after we observe an event, but before the page load completes. This
> should be very rare and I think it's acceptable to use a simpler logging
> approach with a single builder scoped to the lifetime while accepting the
slight
> risk of browser crashes.
> 
> You're right that historically, waiting to log metrics was a big issue for
page
> load metrics, due to app kills on Android, but with the newer
> FlushMetricsOnAppEnterBackground callback, which this observer implements,
we're
> able to flush all metrics out before getting killed on Android, which
addresses
> that issue.

I don't think you should be keeping UkmEntryBuilders alive outside of the scope
you get them in.  I think the general idea is that if you have data that are
available at different times, you should just record them in different entries. 
So you'll probably have an entry for for FirstContentfulPaint, and another entry
for "end of page lifetime" which records things like total time on page.

[Bryan McQuade, 2017-03-01 22:07:45]

On 2017/03/01 at 21:49:52, holte wrote:
> On 2017/03/01 19:26:16, Bryan McQuade wrote:
> > On 2017/03/01 at 19:18:00, zhenw wrote:
> > >
> >
https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
> > > File
> > chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2719823003/diff/300001/chrome/browser/page_lo...
> > >
> >
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:60:
> > std::unique_ptr<ukm::UkmEntryBuilder> builder_;
> > > Builder will only add UkmEntry to UkmService when it is destructed. I
remember
> > the observer's life ends when another navigation in the same frame commits.
I am
> > not sure if you want to wait that long. Is that desired?
> > 
> > Good question. The default lifetime of an observer is until the next page
load
> > commits. Observers can eagerly decide to be destroyed by returning
> > STOP_OBSERVING from some methods, which this one does, from OnHidden and
others.
> > 
> > We could try to be even more aggressive and keep a builder scoped to the
> > lifetime of the OnFirstContentfulPaint callback, which would work fine in
this
> > version of the change, but we'd undo that in the follow up change that adds
> > additional metrics such as total time on page, which do want to wait until
the
> > end of the page lifetime. So we might as well keep things simple and have a
> > builder scoped to the lifetime of the observer, to enable that case.
> > 
> > By delaying logging here, the only risk we run is that the browser process
> > crashes after we observe an event, but before the page load completes. This
> > should be very rare and I think it's acceptable to use a simpler logging
> > approach with a single builder scoped to the lifetime while accepting the
slight
> > risk of browser crashes.
> > 
> > You're right that historically, waiting to log metrics was a big issue for
page
> > load metrics, due to app kills on Android, but with the newer
> > FlushMetricsOnAppEnterBackground callback, which this observer implements,
we're
> > able to flush all metrics out before getting killed on Android, which
addresses
> > that issue.
> 
> I don't think you should be keeping UkmEntryBuilders alive outside of the
scope you get them in.  I think the general idea is that if you have data that
are available at different times, you should just record them in different
entries.  So you'll probably have an entry for for FirstContentfulPaint, and
another entry for "end of page lifetime" which records things like total time on
page.

Sounds fine. Updated to use a builder in local scope. Thanks!

[Bryan McQuade, 2017-03-01 22:08:12]

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

[commit-bot: I haz the power, 2017-03-01 22:08: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-03-01 22:13:13]

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

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

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

[Bryan McQuade, 2017-03-01 22:17:40]

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

[commit-bot: I haz the power, 2017-03-01 22:18: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-03-01 22:25:57]

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

[commit-bot: I haz the power, 2017-03-01 22:25:58]

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

[Bryan McQuade, 2017-03-01 22:27:02]

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

[commit-bot: I haz the power, 2017-03-01 22:27:43]

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-03-01 22:35:02]

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

[commit-bot: I haz the power, 2017-03-01 22:35:03]

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

[Bryan McQuade, 2017-03-01 22:44:39]

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

[commit-bot: I haz the power, 2017-03-01 22:45:13]

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

[Zhen Wang, 2017-03-01 23:28:41]

https://codereview.chromium.org/2719823003/diff/400001/components/metrics/pro...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2719823003/diff/400001/components/metrics/pro...
components/metrics/proto/ukm/source.proto:27: optional int64
navigation_time_msec = 3;
Are we also going to deprecate this?

And this is not used anywhere right now, right?

[Bryan McQuade, 2017-03-01 23:35:32]

https://codereview.chromium.org/2719823003/diff/400001/components/metrics/pro...
File components/metrics/proto/ukm/source.proto (right):

https://codereview.chromium.org/2719823003/diff/400001/components/metrics/pro...
components/metrics/proto/ukm/source.proto:27: optional int64
navigation_time_msec = 3;
On 2017/03/01 at 23:28:41, Zhen Wang wrote:
> Are we also going to deprecate this?
> 
> And this is not used anywhere right now, right?

I think we want to retain this one, perhaps renaming it to start_time_msec to
make it more generic, and set it, so we can sequence Sources relative to one
another. Modifying this field is outside the scope of this change.

[Zhen Wang, 2017-03-01 23:38:00]

On 2017/03/01 23:35:32, Bryan McQuade wrote:
>
https://codereview.chromium.org/2719823003/diff/400001/components/metrics/pro...
> File components/metrics/proto/ukm/source.proto (right):
> 
>
https://codereview.chromium.org/2719823003/diff/400001/components/metrics/pro...
> components/metrics/proto/ukm/source.proto:27: optional int64
> navigation_time_msec = 3;
> On 2017/03/01 at 23:28:41, Zhen Wang wrote:
> > Are we also going to deprecate this?
> > 
> > And this is not used anywhere right now, right?
> 
> I think we want to retain this one, perhaps renaming it to start_time_msec to
> make it more generic, and set it, so we can sequence Sources relative to one
> another. Modifying this field is outside the scope of this change.

I see. lgtm. Remember to use the other UKM API to get source ID.

[commit-bot: I haz the power, 2017-03-01 23:43:49]

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

[commit-bot: I haz the power, 2017-03-01 23:43:50]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-03-01 23:54:39]

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:59:
AddPageLoadExtraInfoMetrics(info, base::TimeTicks());
shouldn't this be Now()? Also i also see the argument seems unused

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:90:
base::TimeTicks app_background_time) {
a_b_t seems to be unused

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:52:
void AddTimingMetrics(const page_load_metrics::PageLoadTiming& timing);
wouldn't mind comments on these too

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:57:
const int32_t source_id_;
comment on how this is used

[Bryan McQuade, 2017-03-02 03:49:14]

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

[Bryan McQuade, 2017-03-02 03:49:50]

addressed comments, thanks! ptal.

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:59:
AddPageLoadExtraInfoMetrics(info, base::TimeTicks());
On 2017/03/01 at 23:54:39, rkaplow (slow) wrote:
> shouldn't this be Now()? Also i also see the argument seems unused

ah, this is used for some of the future metrics that aren't part of this change,
so can be removed for now. thanks!

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc:90:
base::TimeTicks app_background_time) {
On 2017/03/01 at 23:54:39, rkaplow (slow) wrote:
> a_b_t seems to be unused

removed

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:52:
void AddTimingMetrics(const page_load_metrics::PageLoadTiming& timing);
On 2017/03/01 at 23:54:39, rkaplow (slow) wrote:
> wouldn't mind comments on these too

done

https://codereview.chromium.org/2719823003/diff/400001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h:57:
const int32_t source_id_;
On 2017/03/01 at 23:54:39, rkaplow (slow) wrote:
> comment on how this is used

done

[commit-bot: I haz the power, 2017-03-02 03:50:39]

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-03-02 04:05:11]

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

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

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Bryan McQuade, 2017-03-02 12:53:14]

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

[commit-bot: I haz the power, 2017-03-02 12:53:30]

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-03-02 13:40:57]

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

[commit-bot: I haz the power, 2017-03-02 13:40:58]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-03-02 15:04:26]

lgtm

[Bryan McQuade, 2017-03-02 15:15:01]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-03-02 15:15:02]

The patchset sent to the CQ was uploaded after l-g-t-m from zhenw@chromium.org
Link to the patchset: https://codereview.chromium.org/2719823003/#ps440001
(title: "renames")

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

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-03-02 15:20:56]

CQ is committing da patch.

Bot data: {"patchset_id": 440001, "attempt_start_ts": 1488467701445170,
"parent_rev": "2b85b2b24e442d77896540bb88c7c8fd0228e1bc", "commit_rev":
"117fb7823a566d83abc3db2d6f1cd06825e67d03"}

[commit-bot: I haz the power, 2017-03-02 15:21:34]

Description was changed from

==========
Convert first contentful paint logging to the new UKM client API

This change switches FCP logging from the Source proto to the new
UKM client API.

This change also does the following:
* extends the UKM test infrastructure slightly, in order to enable testing
  of metrics that use thew new client API.
* renames committed_url to url in the client-side code, to match the proto
* stops populating the first_contentful_paint source field and marks it as
  deprecated, as FCP is now recorded using the new client API.

BUG=697454
==========

to

==========
Convert first contentful paint logging to the new UKM client API

This change switches FCP logging from the Source proto to the new
UKM client API.

This change also does the following:
* extends the UKM test infrastructure slightly, in order to enable testing
  of metrics that use thew new client API.
* renames committed_url to url in the client-side code, to match the proto
* stops populating the first_contentful_paint source field and marks it as
  deprecated, as FCP is now recorded using the new client API.

BUG=697454

Review-Url: https://codereview.chromium.org/2719823003
Cr-Commit-Position: refs/heads/master@{#454262}
Committed:
https://chromium.googlesource.com/chromium/src/+/117fb7823a566d83abc3db2d6f1c...
==========

[commit-bot: I haz the power, 2017-03-02 15:21:36]

Committed patchset #23 (id:440001) as
https://chromium.googlesource.com/chromium/src/+/117fb7823a566d83abc3db2d6f1c...