Update PaintTiming Web Perf APIs for FP & FCP to report swap time

Update PaintTiming Web Perf APIs for FP & FCP to report swap time

BUG=657826, 657825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2873033002
Cr-Commit-Position: refs/heads/master@{#474504}
Committed: https://chromium.googlesource.com/chromium/src/+/e7366cc17bd54dc1f347e98e40668cf845e472b5

[panicker, 2017-05-09 21:16:36]

Description was changed from

==========
Update PaintTiming Web Perf APIs for FP & FCP to report swap time

BUG=657826,657825
==========

to

==========
Update PaintTiming Web Perf APIs for FP & FCP to report swap time

BUG=657826,657825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[panicker, 2017-05-09 21:21:32]

panicker@chromium.org changed reviewers:
+ tdresser@chromium.org, wangxianzhu@chromium.org

[panicker, 2017-05-09 21:21:33]

Starting with updating the web perf API timestamp.
Subsequent CLs will update the TBM trace and then UMA (recorded in
page-load-metrics)

[panicker, 2017-05-09 21:23:02]

On 2017/05/09 21:21:33, panicker wrote:
> Starting with updating the web perf API timestamp.
> Subsequent CLs will update the TBM trace and then UMA (recorded in
> page-load-metrics)

Any reason not to add an additonal UMA (in PaintTiming.cpp) matching up with the
new swap-time timestamps?

[tdresser, 2017-05-10 13:40:19]

Can we write some tests for this? (LGTM if you want to write tests in a followup
patch, but we should have some).

+1 to adding a new UMA with these values in a followup patch.

[Xianzhu, 2017-05-10 15:44:25]

(rs) lgtm.

[Bryan McQuade, 2017-05-10 15:57:30]

On 2017/05/10 at 15:44:25, wangxianzhu wrote:
> (rs) lgtm.

Thanks for this! Last I saw first image and first text paints had also been
affected. Will those also be fixed to be scoped to their frame, or is additional
work needed there?

[panicker, 2017-05-10 21:23:12]

On 2017/05/10 13:40:19, tdresser wrote:
> Can we write some tests for this? (LGTM if you want to write tests in a
followup
> patch, but we should have some).
> 
> +1 to adding a new UMA with these values in a followup patch.

Happy to write tests as follow-up - if we have interesting test cases. Any
suggestions?
i.e. the test shouldn't just be that we called
PerformanceBase::AddFirstPaintTiming (this is already covered in the layout
test)

[panicker, 2017-05-10 21:23:45]

On 2017/05/10 15:57:30, Bryan McQuade wrote:
> On 2017/05/10 at 15:44:25, wangxianzhu wrote:
> > (rs) lgtm.
> 
> Thanks for this! Last I saw first image and first text paints had also been
> affected. Will those also be fixed to be scoped to their frame, or is
additional
> work needed there?

Good catch Bryan, I'll fix this in a followup!

[panicker, 2017-05-10 21:27:22]

On 2017/05/10 21:23:45, panicker wrote:
> On 2017/05/10 15:57:30, Bryan McQuade wrote:
> > On 2017/05/10 at 15:44:25, wangxianzhu wrote:
> > > (rs) lgtm.
> > 
> > Thanks for this! Last I saw first image and first text paints had also been
> > affected. Will those also be fixed to be scoped to their frame, or is
> additional
> > work needed there?
> 
> Good catch Bryan, I'll fix this in a followup!

To clarify - I think you mean that text-paint and image-paint should also have
the "swap" counterpart so we can record the right time for TBM trace etc right?
Is this needed for UMA also ?

[panicker, 2017-05-10 21:27:54]

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

[commit-bot: I haz the power, 2017-05-10 21:29:04]

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-10 22:50:45]

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

[commit-bot: I haz the power, 2017-05-10 22:50:46]

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

[tdresser, 2017-05-11 12:29:31]

On 2017/05/10 21:27:22, panicker wrote:
> On 2017/05/10 21:23:45, panicker wrote:
> > On 2017/05/10 15:57:30, Bryan McQuade wrote:
> > > On 2017/05/10 at 15:44:25, wangxianzhu wrote:
> > > > (rs) lgtm.
> > > 
> > > Thanks for this! Last I saw first image and first text paints had also
been
> > > affected. Will those also be fixed to be scoped to their frame, or is
> > additional
> > > work needed there?
> > 
> > Good catch Bryan, I'll fix this in a followup!
> 
> To clarify - I think you mean that text-paint and image-paint should also have
> the "swap" counterpart so we can record the right time for TBM trace etc
right?
> Is this needed for UMA also ?

Regarding tests - ideally we'd have a test case where we verify that we are now
including the raster cost.

[panicker, 2017-05-11 22:20:41]

The CQ bit was checked by panicker@chromium.org

[commit-bot: I haz the power, 2017-05-11 22:21:49]

CQ is trying da patch.

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

[panicker, 2017-05-11 22:23:16]

On 2017/05/11 12:29:31, tdresser wrote:
> On 2017/05/10 21:27:22, panicker wrote:
> > On 2017/05/10 21:23:45, panicker wrote:
> > > On 2017/05/10 15:57:30, Bryan McQuade wrote:
> > > > On 2017/05/10 at 15:44:25, wangxianzhu wrote:
> > > > > (rs) lgtm.
> > > > 
> > > > Thanks for this! Last I saw first image and first text paints had also
> been
> > > > affected. Will those also be fixed to be scoped to their frame, or is
> > > additional
> > > > work needed there?
> > > 
> > > Good catch Bryan, I'll fix this in a followup!
> > 
> > To clarify - I think you mean that text-paint and image-paint should also
have
> > the "swap" counterpart so we can record the right time for TBM trace etc
> right?
> > Is this needed for UMA also ?
> 
> Regarding tests - ideally we'd have a test case where we verify that we are
now
> including the raster cost.

Yeah that's hard in a unit-test with the current plumbing - punching through so
many layers.
We should perhaps craft a layout test that makes compositing / raster expensive
- thoughts?

[commit-bot: I haz the power, 2017-05-11 23:43:19]

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

[commit-bot: I haz the power, 2017-05-11 23:43:22]

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

[panicker, 2017-05-24 17:57:39]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-05-24 17:57:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2873033002/#ps20001
(title: "Update basic test and suppress some tests. The suppressed tests are
actively being worked on to mak…")

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

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-24 20:10:03]

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

[commit-bot: I haz the power, 2017-05-24 20:10:04]

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

[panicker, 2017-05-24 21:12:39]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-05-24 21:12:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2873033002/#ps40001
(title: "missed virtual/mojo-loading tests")

[commit-bot: I haz the power, 2017-05-24 21:13:10]

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495660359464980,
"parent_rev": "4f51183e751e780ae7cd12d9782bd2bf104c8eeb", "commit_rev":
"e7366cc17bd54dc1f347e98e40668cf845e472b5"}

[commit-bot: I haz the power, 2017-05-25 01:03:55]

Description was changed from

==========
Update PaintTiming Web Perf APIs for FP & FCP to report swap time

BUG=657826,657825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Update PaintTiming Web Perf APIs for FP & FCP to report swap time

BUG=657826,657825
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e7366cc17bd54dc1f347e98e4066...

[yosin_UTC9, 2017-05-25 06:51:48]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2904023002/ by yosin@chromium.org.

The reason for reverting is: Probably causes leaks:
* http/tests/performance-timing/paint-timing/observable.html
*
virtual/mojo-loading/http/tests/performance-timing/paint-timing/observable.html

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty....