Fix FirstContentfulPaint metric

Fix FirstContentfulPaint metric

The firstContentfulPaint trace event is logged under the "loading"
cateogry now.

BUG=chromium:711144
Review-Url: https://codereview.chromium.org/2814963004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/dee2ad3570ce578a81ad73a1acbf4f4c7bf5cdf0

[Kunihiko Sakamoto, 2017-04-13 04:31:12]

Description was changed from

==========
Fix FirstContentfulPaint metric

The firstContentfulPaint trace event is logged now under the loading
cateogry.

BUG=chromium:711144
==========

to

==========
Fix FirstContentfulPaint metric

The firstContentfulPaint trace event is logged under the "loading"
cateogry now.

BUG=chromium:711144
==========

[Kunihiko Sakamoto, 2017-04-13 04:31:31]

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

[commit-bot: I haz the power, 2017-04-13 04:31:35]

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-04-13 04:55:25]

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

[commit-bot: I haz the power, 2017-04-13 04:55:26]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2017-04-13 05:02:39]

ksakamoto@chromium.org changed reviewers:
+ nednguyen@google.com

[nednguyen, 2017-04-13 10:27:06]

nednguyen@google.com changed reviewers:
+ tdresser@chromium.org

[nednguyen, 2017-04-13 10:27:07]

Is it possible to expand smoke test for this to catch the bug?

https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

[Kunihiko Sakamoto, 2017-04-14 01:41:45]

On 2017/04/13 10:27:07, nednguyen wrote:
> Is it possible to expand smoke test for this to catch the bug?
> 
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

There's a assertion for FCP in the smoke test, but it's disabled because ref
build didn't have FCP event when it was added:
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

Is the ref build updated since then? FCP event is added in Jan 2016 (Chrome
M50).

Probably we should search for the FCP event from both loading and
blink.user_timing categories, so that it works for the ref build too.

[nednguyen, 2017-04-14 01:43:53]

On 2017/04/14 01:41:45, Kunihiko Sakamoto wrote:
> On 2017/04/13 10:27:07, nednguyen wrote:
> > Is it possible to expand smoke test for this to catch the bug?
> > 
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> There's a assertion for FCP in the smoke test, but it's disabled because ref
> build didn't have FCP event when it was added:
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> Is the ref build updated since then? FCP event is added in Jan 2016 (Chrome
> M50).
> 
> Probably we should search for the FCP event from both loading and
> blink.user_timing categories, so that it works for the ref build too.

For now, we can just disable the whole test with @Disabled('reference'), it will
still be run on TOT chrome on CQ (you can make the change to catapult in your
chromium src tree & run: tools/perf/run_telemetry_tests
testFirstPaintMetricSmoke)

LGTM so you can add tests & land this without long cycle review.

[Kunihiko Sakamoto, 2017-04-14 02:07:27]

Patchset #2 (id:20001) has been deleted

[Kunihiko Sakamoto, 2017-04-14 02:08:58]

On 2017/04/14 01:43:53, nednguyen wrote:
> On 2017/04/14 01:41:45, Kunihiko Sakamoto wrote:
> > On 2017/04/13 10:27:07, nednguyen wrote:
> > > Is it possible to expand smoke test for this to catch the bug?
> > > 
> > >
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> > 
> > There's a assertion for FCP in the smoke test, but it's disabled because ref
> > build didn't have FCP event when it was added:
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> > 
> > Is the ref build updated since then? FCP event is added in Jan 2016 (Chrome
> > M50).
> > 
> > Probably we should search for the FCP event from both loading and
> > blink.user_timing categories, so that it works for the ref build too.
> 
> For now, we can just disable the whole test with @Disabled('reference'), it
will
> still be run on TOT chrome on CQ (you can make the change to catapult in your
> chromium src tree & run: tools/perf/run_telemetry_tests
> testFirstPaintMetricSmoke)
> 
> LGTM so you can add tests & land this without long cycle review.

OK, enabled the FCP (and FMP) assertions, and disabled the test on reference
builds for now.

Thanks for the review!

[Kunihiko Sakamoto, 2017-04-14 02:09:17]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2017-04-14 02:09:18]

The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2814963004/#ps40001
(title: "smoke test")

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

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-04-14 02:33:10]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492135757935980,
"parent_rev": "7d36fc672e8f08d29f85b3ea4ef93d6ff1ab18cb", "commit_rev":
"dee2ad3570ce578a81ad73a1acbf4f4c7bf5cdf0"}

[commit-bot: I haz the power, 2017-04-14 02:33:16]

Description was changed from

==========
Fix FirstContentfulPaint metric

The firstContentfulPaint trace event is logged under the "loading"
cateogry now.

BUG=chromium:711144
==========

to

==========
Fix FirstContentfulPaint metric

The firstContentfulPaint trace event is logged under the "loading"
cateogry now.

BUG=chromium:711144

Review-Url: https://codereview.chromium.org/2814963004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-04-14 02:33:17]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...