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

Attempt 2: Update PaintTiming Web Perf APIs for FP & FCP to report swap time
Update tests to use capturePixelsAsyncThen to generate a frame, and check buffered values.

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

Review-Url: https://codereview.chromium.org/2932593002
Cr-Commit-Position: refs/heads/master@{#479303}
Committed: https://chromium.googlesource.com/chromium/src/+/600950a6cab835f6a2932ead72cb0c0a34782fa5

[panicker, 2017-06-07 23:37:44]

Description was changed from

==========
Attempt 2: Update PaintTiming Web Perf APIs for FP & FCP to report swap time
Update tests to use capturePixelsAsyncThen to generate a frame, and check
buffered values.

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

to

==========
Attempt 2: Update PaintTiming Web Perf APIs for FP & FCP to report swap time
Update tests to use capturePixelsAsyncThen to generate a frame, and check
buffered values.

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

[panicker, 2017-06-07 23:43:55]

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

[panicker, 2017-06-07 23:43:56]

Hey folks
This is a rollforward for this CL that you reviewed previously.
https://codereview.chromium.org/2873033002
The main issue was with tests, which I've fixed now.
PTAL.

PS: Haven't figured out how to keep using PerformanceObserver with
capturePixelsAsyncThen, but a good follow-up thing.

[tdresser, 2017-06-08 14:34:33]

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:13:
testRunner.capturePixelsAsyncThen(t.step_func(function() {
Can we make a promise based version of capturePixelsAsyncThen?
Even if we don't share it, I think it could clean up this test a fair bit.

Perhaps snazzy new async / await syntax could be useful here?
We could then just await on capture pixels?

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html:3:
<title>Performance Paint Timing Test</title>
Should these titles be more descriptive?

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html:14:
document.getElementById('svg').appendChild(img);
Would this be clearer in html vs js?

[panicker, 2017-06-08 22:47:05]

PTAL

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:13:
testRunner.capturePixelsAsyncThen(t.step_func(function() {
On 2017/06/08 14:34:32, tdresser wrote:
> Can we make a promise based version of capturePixelsAsyncThen?
> Even if we don't share it, I think it could clean up this test a fair bit.
> 
> Perhaps snazzy new async / await syntax could be useful here?
> We could then just await on capture pixels?

this is C++ exposed, I suppose we can talk to testing / relevant folks on
supporting promise based API, but I don't think I want to update the
test_runner.cc :)

FWIW - the nesting is a common pattern AFAICT, eg:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...


https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc...

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html:3:
<title>Performance Paint Timing Test</title>
On 2017/06/08 14:34:33, tdresser wrote:
> Should these titles be more descriptive?

Done.

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html:14:
document.getElementById('svg').appendChild(img);
On 2017/06/08 14:34:32, tdresser wrote:
> Would this be clearer in html vs js?

the reason to do stuff in JS is to control timing of when things happen

[tdresser, 2017-06-09 14:26:27]

LGTM % thoughts.

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:13:
testRunner.capturePixelsAsyncThen(t.step_func(function() {
On 2017/06/08 22:47:04, panicker wrote:
> On 2017/06/08 14:34:32, tdresser wrote:
> > Can we make a promise based version of capturePixelsAsyncThen?
> > Even if we don't share it, I think it could clean up this test a fair bit.
> > 
> > Perhaps snazzy new async / await syntax could be useful here?
> > We could then just await on capture pixels?
> 
> this is C++ exposed, I suppose we can talk to testing / relevant folks on
> supporting promise based API, but I don't think I want to update the
> test_runner.cc :)
> 
> FWIW - the nesting is a common pattern AFAICT, eg:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> 
> 
>
https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc...

I was thinking we'd just wrap capturePixelsAsyncThen in js. We could stick it in
some shared js file.

For example, something like https://output.jsbin.com/siduhi/quiet.

It isn't much code, and I think it would make this test especially much easier
to read.

That being said, if you'd prefer to stick with nesting, that's fine by me.

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html:14:
document.getElementById('svg').appendChild(img);
On 2017/06/08 22:47:04, panicker wrote:
> On 2017/06/08 14:34:32, tdresser wrote:
> > Would this be clearer in html vs js?
> 
> the reason to do stuff in JS is to control timing of when things happen

In this example, what would the difference in timing be? I thought they'd be the
same.

https://codereview.chromium.org/2932593002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html
(right):

https://codereview.chromium.org/2932593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html:16:
var div = document.createElement("div");
Should we use const/let in tests at this point?

[Xianzhu, 2017-06-09 15:33:33]

(rs)lgtm.

[panicker, 2017-06-09 20:29:48]

Thanks for the review!

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:13:
testRunner.capturePixelsAsyncThen(t.step_func(function() {
On 2017/06/09 14:26:27, tdresser wrote:
> On 2017/06/08 22:47:04, panicker wrote:
> > On 2017/06/08 14:34:32, tdresser wrote:
> > > Can we make a promise based version of capturePixelsAsyncThen?
> > > Even if we don't share it, I think it could clean up this test a fair bit.
> > > 
> > > Perhaps snazzy new async / await syntax could be useful here?
> > > We could then just await on capture pixels?
> > 
> > this is C++ exposed, I suppose we can talk to testing / relevant folks on
> > supporting promise based API, but I don't think I want to update the
> > test_runner.cc :)
> > 
> > FWIW - the nesting is a common pattern AFAICT, eg:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> > 
> > 
> >
>
https://cs.chromium.org/chromium/src/content/shell/test_runner/test_runner.cc...
> 
> I was thinking we'd just wrap capturePixelsAsyncThen in js. We could stick it
in
> some shared js file.
> 
> For example, something like https://output.jsbin.com/siduhi/quiet.
> 
> It isn't much code, and I think it would make this test especially much easier
> to read.
> 
> That being said, if you'd prefer to stick with nesting, that's fine by me.

Nice! This seems like a great follow-up (I couldn't get the syntax working, and
taken an AI to get upto speed on promise & es6 usage & syntax :))

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html
(right):

https://codereview.chromium.org/2932593002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-svg.html:14:
document.getElementById('svg').appendChild(img);
On 2017/06/09 14:26:27, tdresser wrote:
> On 2017/06/08 22:47:04, panicker wrote:
> > On 2017/06/08 14:34:32, tdresser wrote:
> > > Would this be clearer in html vs js?
> > 
> > the reason to do stuff in JS is to control timing of when things happen
> 
> In this example, what would the difference in timing be? I thought they'd be
the
> same.

I think in this case it's fine because I moved the div from bottom to top, but
prefer (not strong opinion) to be more explicit in test:
do a thing, then assert, do something else, then assert something else ...

https://codereview.chromium.org/2932593002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html
(right):

https://codereview.chromium.org/2932593002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html:16:
var div = document.createElement("div");
On 2017/06/09 14:26:27, tdresser wrote:
> Should we use const/let in tests at this point?

Done.

[panicker, 2017-06-09 20:31:15]

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

[commit-bot: I haz the power, 2017-06-09 20:31:51]

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-06-09 22:37:52]

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

[commit-bot: I haz the power, 2017-06-09 22:37:53]

Dry run: 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_...)

[tdresser, 2017-06-12 14:44:45]

Thanks for the changes!

1 nit: const in JS is super confusing. From MDN "The const declaration creates a
read-only reference to a value. It does not mean the value it holds is
immutable, just that the variable identifier cannot be reassigned.".

Most of the places you used let, we should be using const. I commented on a few
of them.

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html
(right):

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html:16:
let div = document.createElement("div");
const

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-bg-image.html
(right):

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-bg-image.html:13:
let body = document.getElementsByTagName('body')[0];
const

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html
(right):

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html:12:
let c = document.getElementById("canvas");
constq

https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html:13:
var ctx = c.getContext("2d");
const

[panicker, 2017-06-12 19:58:14]

On 2017/06/12 14:44:45, tdresser wrote:
> Thanks for the changes!
> 
> 1 nit: const in JS is super confusing. From MDN "The const declaration creates
a
> read-only reference to a value. It does not mean the value it holds is
> immutable, just that the variable identifier cannot be reassigned.".
> 
> Most of the places you used let, we should be using const. I commented on a
few
> of them.
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html
> (right):
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/basetest.html:16:
> let div = document.createElement("div");
> const
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-bg-image.html
> (right):
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-bg-image.html:13:
> let body = document.getElementsByTagName('body')[0];
> const
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html
> (right):
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html:12:
> let c = document.getElementById("canvas");
> constq
> 
>
https://codereview.chromium.org/2932593002/diff/80001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html:13:
> var ctx = c.getContext("2d");
> const

Done.

[panicker, 2017-06-12 19:58:21]

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

[commit-bot: I haz the power, 2017-06-12 19:58:56]

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-06-12 21:17:24]

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

[commit-bot: I haz the power, 2017-06-12 21:17:25]

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

[tdresser, 2017-06-13 15:23:01]

Still LGTM.

[panicker, 2017-06-13 22:46:46]

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

[commit-bot: I haz the power, 2017-06-13 22:47:21]

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-06-14 00:52:24]

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

[commit-bot: I haz the power, 2017-06-14 00:52:25]

Dry run: 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-06-14 01:00:21]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-06-14 01:00:23]

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/2932593002/#ps120001
(title: "move test to image onload, otherwise its inconsistent / flaky")

[commit-bot: I haz the power, 2017-06-14 01:00:49]

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-06-14 03:11:02]

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

[commit-bot: I haz the power, 2017-06-14 03:11:03]

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-06-14 04:22:17]

The CQ bit was checked by panicker@chromium.org

[commit-bot: I haz the power, 2017-06-14 04:22:26]

CQ is trying da patch.

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

[panicker, 2017-06-14 04:35:33]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-06-14 04:35:33]

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/2932593002/#ps140001
(title: "rebase to head")

[commit-bot: I haz the power, 2017-06-14 04:35:50]

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-06-14 06:23:18]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1497414933117790,
"parent_rev": "f9848df898526e51f20e95deddd426575b758418", "commit_rev":
"600950a6cab835f6a2932ead72cb0c0a34782fa5"}

[commit-bot: I haz the power, 2017-06-14 06:23:27]

Description was changed from

==========
Attempt 2: Update PaintTiming Web Perf APIs for FP & FCP to report swap time
Update tests to use capturePixelsAsyncThen to generate a frame, and check
buffered values.

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

to

==========
Attempt 2: Update PaintTiming Web Perf APIs for FP & FCP to report swap time
Update tests to use capturePixelsAsyncThen to generate a frame, and check
buffered values.

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

Review-Url: https://codereview.chromium.org/2932593002
Cr-Commit-Position: refs/heads/master@{#479303}
Committed:
https://chromium.googlesource.com/chromium/src/+/600950a6cab835f6a2932ead72cb...
==========

[commit-bot: I haz the power, 2017-06-14 06:23:29]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/600950a6cab835f6a2932ead72cb...