Convert some fontfaceset tests to use jsharness

Convert some fontfaceset tests to use jsharness

This was basically an exercise for me in learning about how FontFace and
promise_test works.

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

[meade_UTC10, 2017-01-13 00:19:16]

meade@chromium.org changed reviewers:
+ sashab@chromium.org

[meade_UTC10, 2017-01-13 00:19:17]

Hi Sasha!

These are the tests I was talking about yesterday (they fail when I fix a FIXME,
but I wanted to understand the tests better before trying to fix the failures).

WDYT? (obviously I'll send to eae/drott after you have a look)

Thanks!
Eddy

[meade_UTC10, 2017-01-13 00:20:30]

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

[commit-bot: I haz the power, 2017-01-13 00:21:43]

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

[sashab, 2017-01-13 01:01:41]

LGTM, the conversion looks good, I'm assuming you did this sensibly with a
find-and-replace or whatever and there are no actual logic changes :)

Yup, follow up review with one of them sounds good.

[commit-bot: I haz the power, 2017-01-13 01:40:56]

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

[commit-bot: I haz the power, 2017-01-13 01:40:57]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-01-13 03:01:58]

On 2017/01/13 01:01:41, sashab wrote:
> LGTM, the conversion looks good, I'm assuming you did this sensibly with a
> find-and-replace or whatever and there are no actual logic changes :)
> 
> Yup, follow up review with one of them sounds good.

Thanks Sasha! I didn't find the conversion particularly scriptable because each
test was written slightly differently and I broke long tests up into shorter
tests. But AFAICT from reading the code, the logic should be the same.

[meade_UTC10, 2017-01-13 03:12:44]

meade@chromium.org changed reviewers:
+ eae@chromium.org

[meade_UTC10, 2017-01-13 03:12:45]

Hi Emil!

Continuing my investigation into how fonts work, I came across these tests while
looking at a different FIXME in the fonts code. I decided to try converting
tests tests to use testharness. WDYT?

Thanks!
Eddy

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:23:
assert_equals(document.fonts.status, 'loaded')
This is loaded, not loading, because face1 is not in the document yet?

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:50:
assert_equals(document.fonts.status, 'loaded');
I don't really understand the final checks in this test (including the
original). 

Should deleting face2 from document.fonts change document.fonts.status or
face2.status? Should there be a check somewhere that face2 was removed from
document.fonts?

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:66:
assert_equals(face3.status, 'loading');
ditto...

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove.html:40:
document.fonts.delete(ahemFace);
These all work here because document.fonts.add/delete/clear is synchronous?

[eae, 2017-01-31 20:51:12]

eae@chromium.org changed reviewers:
+ drott@chromium.org

[eae, 2017-01-31 20:51:12]

This looks reasonable to me (LGTM) but please double check with drott.

Thanks for your patience!

[drott, 2017-02-01 20:01:36]

Looks like a good cleanup and modernization of these files, thank you! LGTM. I
don't have answers to your questions yet, I'll try to look into it once I have a
bit more time after BlinkOn. Ksakamoto@ seems to be the original author of these
tests, I am adding him as a reviewer, maybe he can answer some of the questions
right away.

[drott, 2017-02-01 20:01:50]

drott@chromium.org changed reviewers:
+ ksakamoto@chromium.org

[Kunihiko Sakamoto, 2017-02-07 10:13:16]

lgtm

Nice, thank you for doing this!

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:23:
assert_equals(document.fonts.status, 'loaded')
On 2017/01/13 03:12:45, Eddy wrote:
> This is loaded, not loading, because face1 is not in the document yet?

That's right.

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:29:
return Promise.all([face1Loaded, loadingDone]).then(() => {
Originally this was document.fonts.ready. Didn't that work?

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:30:
assert_equals(document.fonts.status, 'loaded');
Can you also check that face1.status is 'loaded' here? (and for face2 and face3
below as well)

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:50:
assert_equals(document.fonts.status, 'loaded');
On 2017/01/13 03:12:45, Eddy wrote:
> I don't really understand the final checks in this test (including the
> original). 
> 
> Should deleting face2 from document.fonts change document.fonts.status or
> face2.status? Should there be a check somewhere that face2 was removed from
> document.fonts?

IIRC, these are checking that deleting face2 from document.fonts
- does not cancel the load of face2,
- does not change document.fonts.status until loadingdone event fires, and
- document.fonts.status should be 'loaded' after loadingdone.

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove.html:40:
document.fonts.delete(ahemFace);
On 2017/01/13 03:12:45, Eddy wrote:
> These all work here because document.fonts.add/delete/clear is synchronous?

Yes these operations are synchronous.

[meade_UTC10, 2017-02-09 07:42:33]

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:29:
return Promise.all([face1Loaded, loadingDone]).then(() => {
On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> Originally this was document.fonts.ready. Didn't that work?

Huh, it does work. Maybe I was fiddling with something when I was writing this?
Anyway, changed it back to that.

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:30:
assert_equals(document.fonts.status, 'loaded');
On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> Can you also check that face1.status is 'loaded' here? (and for face2 and
face3
> below as well)

With this style of tests, face1 is not in scope here. Should I be creating
face1, face2 and face3 outside of the test cases themselves? But that breaks the
self-contained test philosophy - testharness tests should be independent and
able to run in any order.

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:50:
assert_equals(document.fonts.status, 'loaded');
On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> On 2017/01/13 03:12:45, Eddy wrote:
> > I don't really understand the final checks in this test (including the
> > original). 
> > 
> > Should deleting face2 from document.fonts change document.fonts.status or
> > face2.status? Should there be a check somewhere that face2 was removed from
> > document.fonts?
> 
> IIRC, these are checking that deleting face2 from document.fonts
> - does not cancel the load of face2,
> - does not change document.fonts.status until loadingdone event fires, and
> - document.fonts.status should be 'loaded' after loadingdone.

Thanks for the explanation. Another thing I was wondering, is why this test uses
Promise.all([face2loaded, loadingdone]), whereas the following test just uses
document.fonts.ready?

[Kunihiko Sakamoto, 2017-02-09 08:18:15]

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:30:
assert_equals(document.fonts.status, 'loaded');
On 2017/02/09 07:42:33, Eddy wrote:
> On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> > Can you also check that face1.status is 'loaded' here? (and for face2 and
> face3
> > below as well)
> 
> With this style of tests, face1 is not in scope here. Should I be creating
> face1, face2 and face3 outside of the test cases themselves? But that breaks
the
> self-contained test philosophy - testharness tests should be independent and
> able to run in any order.

Hmm, I think face1 should be accessible here?

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:50:
assert_equals(document.fonts.status, 'loaded');
On 2017/02/09 07:42:33, Eddy wrote:
> On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> > On 2017/01/13 03:12:45, Eddy wrote:
> > > I don't really understand the final checks in this test (including the
> > > original). 
> > > 
> > > Should deleting face2 from document.fonts change document.fonts.status or
> > > face2.status? Should there be a check somewhere that face2 was removed
from
> > > document.fonts?
> > 
> > IIRC, these are checking that deleting face2 from document.fonts
> > - does not cancel the load of face2,
> > - does not change document.fonts.status until loadingdone event fires, and
> > - document.fonts.status should be 'loaded' after loadingdone.
> 
> Thanks for the explanation. Another thing I was wondering, is why this test
uses
> Promise.all([face2loaded, loadingdone]), whereas the following test just uses
> document.fonts.ready?

I don't remember why... :)
Maybe we can merge those two and use
Promice.all([faceNloaded, loadingdone, document.fonts.ready])
in both tests?

[meade_UTC10, 2017-02-14 00:51:43]

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html
(right):

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:30:
assert_equals(document.fonts.status, 'loaded');
On 2017/02/09 08:18:15, Kunihiko Sakamoto wrote:
> On 2017/02/09 07:42:33, Eddy wrote:
> > On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> > > Can you also check that face1.status is 'loaded' here? (and for face2 and
> > face3
> > > below as well)
> > 
> > With this style of tests, face1 is not in scope here. Should I be creating
> > face1, face2 and face3 outside of the test cases themselves? But that breaks
> the
> > self-contained test philosophy - testharness tests should be independent and
> > able to run in any order.
> 
> Hmm, I think face1 should be accessible here?

oops, I think I misread where this comment is in the diff. Done.

https://codereview.chromium.org/2629543004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/fontfaceset-add-remove-while-loading.html:50:
assert_equals(document.fonts.status, 'loaded');
On 2017/02/09 08:18:15, Kunihiko Sakamoto wrote:
> On 2017/02/09 07:42:33, Eddy wrote:
> > On 2017/02/07 10:13:15, Kunihiko Sakamoto wrote:
> > > On 2017/01/13 03:12:45, Eddy wrote:
> > > > I don't really understand the final checks in this test (including the
> > > > original). 
> > > > 
> > > > Should deleting face2 from document.fonts change document.fonts.status
or
> > > > face2.status? Should there be a check somewhere that face2 was removed
> from
> > > > document.fonts?
> > > 
> > > IIRC, these are checking that deleting face2 from document.fonts
> > > - does not cancel the load of face2,
> > > - does not change document.fonts.status until loadingdone event fires, and
> > > - document.fonts.status should be 'loaded' after loadingdone.
> > 
> > Thanks for the explanation. Another thing I was wondering, is why this test
> uses
> > Promise.all([face2loaded, loadingdone]), whereas the following test just
uses
> > document.fonts.ready?
> 
> I don't remember why... :)
> Maybe we can merge those two and use
> Promice.all([faceNloaded, loadingdone, document.fonts.ready])
> in both tests?

Done.

[Kunihiko Sakamoto, 2017-02-14 02:24:03]

Thanks! Could you re-run "git cl upload" ?

[meade_UTC10, 2017-02-14 04:30:10]

On 2017/02/14 02:24:03, Kunihiko Sakamoto wrote:
> Thanks! Could you re-run "git cl upload" ?

Done, sorry!

[meade_UTC10, 2017-02-14 04:32:05]

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

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

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

[Kunihiko Sakamoto, 2017-02-14 05:05:42]

LGTM

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

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

[commit-bot: I haz the power, 2017-02-14 05:32:29]

Dry run: This issue passed the CQ dry run.

[meade_UTC10, 2017-02-14 05:36:19]

The CQ bit was checked by meade@chromium.org

[meade_UTC10, 2017-02-14 05:36:20]

The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org,
drott@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2629543004/#ps60001
(title: "Fix minor formatting issue")

[commit-bot: I haz the power, 2017-02-14 05:36:31]

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-14 05:41:07]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487050579866080,
"parent_rev": "c8b9ca7e60bf607a91e1f73a9bc12e8cde0940e4", "commit_rev":
"e26c0958d87bd7b1b43d83310e96323156696d3f"}

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

Description was changed from

==========
Convert some fontfaceset tests to use jsharness

This was basically an exercise for me in learning about how FontFace and
promise_test works.
==========

to

==========
Convert some fontfaceset tests to use jsharness

This was basically an exercise for me in learning about how FontFace and
promise_test works.

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

[commit-bot: I haz the power, 2017-02-14 05:42:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e26c0958d87bd7b1b43d83310e96...