Add tests for paint-timing, and move existing tests to same location

Add tests for paint-timing, and move existing tests to same location

BUG=657826, 657825
Review-Url: https://codereview.chromium.org/2860143002
Cr-Commit-Position: refs/heads/master@{#470441}
Committed: https://chromium.googlesource.com/chromium/src/+/2e0ee81a6380db6c714980a30a53a8c002ff46e7

[panicker, 2017-05-04 19:13:20]

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

[panicker, 2017-05-04 19:13:21]

PTAL

[tdresser, 2017-05-04 20:53:57]

Can we add a test where FP doesn't fire?

I'd feel a bit better about these tests if we checked that we hadn't observed
the paints until we're supposed to. For example, that we can add the blank
canvas, and do everything except stroke it without getting the notification, and
then when we stroke it, we get the notification.

It would be a bit messy to get the timing right, but I think it would be worth
it.

[panicker, 2017-05-08 18:03:45]

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

[igrigorik, 2017-05-08 20:37:03]

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

[igrigorik, 2017-05-08 20:37:04]

Thanks for driving this Shubhie! A few questions..

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

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html:34:
window.onload=function(){
May be redundant, but perhaps snapshot the time here and make sure that the
startTime of paint entries is greater than onload time?

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-image.html
(right):

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-image.html:35:
var img = document.createElement("IMG");
Ditto here? We can check that FP happens before FCP by recording onload
timestamp?

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-paint-only.html
(right):

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-paint-only.html:29:
}, "Performance first paint timing entry is observable. No first contentful
paint.");
Does it make sense to t.done() after onload?

[panicker, 2017-05-09 00:54:56]

On 2017/05/04 20:53:57, tdresser wrote:
> Can we add a test where FP doesn't fire?
Added.
> 
> I'd feel a bit better about these tests if we checked that we hadn't observed
> the paints until we're supposed to. For example, that we can add the blank
> canvas, and do everything except stroke it without getting the notification,
and
> then when we stroke it, we get the notification.
> 
> It would be a bit messy to get the timing right, but I think it would be worth
> it.
I've added one such test: first-contentful-paint.html
I don't think we need to convert every test to this (bit messy) format.

[igrigorik, 2017-05-09 02:09:58]

On 2017/05/09 00:54:56, panicker wrote:
> On 2017/05/04 20:53:57, tdresser wrote:
> > Can we add a test where FP doesn't fire?
> Added.
> > 
> > I'd feel a bit better about these tests if we checked that we hadn't
observed
> > the paints until we're supposed to. For example, that we can add the blank
> > canvas, and do everything except stroke it without getting the notification,
> and
> > then when we stroke it, we get the notification.
> > 
> > It would be a bit messy to get the timing right, but I think it would be
worth
> > it.
> I've added one such test: first-contentful-paint.html
> I don't think we need to convert every test to this (bit messy) format.

Makes sense. Updated tests look great! Thanks Shubhie.

[tdresser, 2017-05-09 13:53:28]

LGTM, thanks!

https://codereview.chromium.org/2860143002/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/2860143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:17:
"There should be only first paint timing instance.");
This string is incorrect, isn't it?

The second time this runs, this will be the first contentful paint.

https://codereview.chromium.org/2860143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:22:
} else if (entries[0].name == "first-contentful-paint") {
Maybe check explicitly that t.first_paint_time has been set?

https://codereview.chromium.org/2860143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:38:
}, "First Paint triggerd by non-contentful paint. Image load triggers First
Contentful Paint.");
triggerd -> triggered

[panicker, 2017-05-09 17:55:06]

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

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-canvas.html:34:
window.onload=function(){
On 2017/05/08 20:37:04, igrigorik wrote:
> May be redundant, but perhaps snapshot the time here and make sure that the
> startTime of paint entries is greater than onload time?

As mentioned let's keep the sequence check in first-contentful-paint.html test

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-image.html
(right):

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-image.html:35:
var img = document.createElement("IMG");
On 2017/05/08 20:37:04, igrigorik wrote:
> Ditto here? We can check that FP happens before FCP by recording onload
> timestamp?

Acknowledged.

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-paint-only.html
(right):

https://codereview.chromium.org/2860143002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-paint-only.html:29:
}, "Performance first paint timing entry is observable. No first contentful
paint.");
On 2017/05/08 20:37:04, igrigorik wrote:
> Does it make sense to t.done() after onload? 

Good point, I added a step_timeout to wait for 1s before wrapping up.

https://codereview.chromium.org/2860143002/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/2860143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:17:
"There should be only first paint timing instance.");
On 2017/05/09 13:53:27, tdresser wrote:
> This string is incorrect, isn't it?
> 
> The second time this runs, this will be the first contentful paint.

Done.

https://codereview.chromium.org/2860143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:22:
} else if (entries[0].name == "first-contentful-paint") {
On 2017/05/09 13:53:27, tdresser wrote:
> Maybe check explicitly that t.first_paint_time has been set?

Done.

https://codereview.chromium.org/2860143002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/performance-timing/paint-timing/first-contentful-paint.html:38:
}, "First Paint triggerd by non-contentful paint. Image load triggers First
Contentful Paint.");
On 2017/05/09 13:53:27, tdresser wrote:
> triggerd -> triggered

Done.

[panicker, 2017-05-09 17:59:34]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-05-09 17:59:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2860143002/#ps60001
(title: "minor formatting")

[commit-bot: I haz the power, 2017-05-09 18:00:48]

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-09 20:09:22]

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

[commit-bot: I haz the power, 2017-05-09 20:09:23]

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-09 21:10:51]

The CQ bit was checked by panicker@chromium.org

[panicker, 2017-05-09 21:10:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2860143002/#ps80001
(title: "add missing image resources")

[commit-bot: I haz the power, 2017-05-09 21:11:57]

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-09 23:55:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494364251010700,
"parent_rev": "ce2aa1a17a985a989bada5ecf204f9e2232c09ad", "commit_rev":
"2e0ee81a6380db6c714980a30a53a8c002ff46e7"}

[commit-bot: I haz the power, 2017-05-09 23:55:18]

Description was changed from

==========
Add tests for paint-timing, and move existing tests to same location

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

to

==========
Add tests for paint-timing, and move existing tests to same location

BUG=657826,657825

Review-Url: https://codereview.chromium.org/2860143002
Cr-Commit-Position: refs/heads/master@{#470441}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e0ee81a6380db6c714980a30a53...
==========

[commit-bot: I haz the power, 2017-05-09 23:55:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2e0ee81a6380db6c714980a30a53...