1465983 - Don't resolve FontFaceSet's ready promise in its constructor

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Jonathan Watt [:jwatt]]

Currently we resolve FontFaceSet's ready promise in its constructor. Doing a bit of bug archeology it seems that happened after heycam sent this email to the list:

https://lists.w3.org/Archives/Public/www-style/2014Jul/0047.html

at which point Tab updated the spec:

https://github.com/w3c/csswg-drafts/commit/e4cdd12c2e19ae6d24a22582ccc189783dfce4da

However, doing this will usually break waiting on document.fonts.ready before the initial loading of a document and its fonts. Any code that waits at that point will trigger immediately, before the fonts have actually loaded.

Tab also seems to have realized too, since he changed the spec to require the promise to be pending 4 days later:

https://github.com/w3c/csswg-drafts/commit/60433e453c26f7add86cba35240beca1426ddd19

I don't see any discussion about that on the list though.

Comment 1

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #0)
> Currently we resolve FontFaceSet's ready promise in its constructor.

Not quite technically correct (digging through old versions of the code, I got a bit confused). We actually currently set mResolveLazilyCreatedReadyPromise to true, but the effect is the same when the Promise is created by FontFaceSet::GetReady, since we'll then resolve it immediately after creating it.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Don't resolve FontFaceSet's ready promise in its constructor

This fixes at least web-platform/tests/css/css-fonts/variations/font-weight-matching.html and possibly others.

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8982376 [details] [diff] [review]
part 2 - Don't resolve FontFaceSet's ready promise in its constructor

Review of attachment 8982376 [details] [diff] [review]:
-----------------------------------------------------------------

If this fixes WPT tests I guess you'll need .ini updates.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Fix race in test_font_loading_api.html.

I'll likely land preemptively today given you r+'ed a similar issue in bug 1466723, but requesting retrospective r? just to make sure you saw this.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/94409f81ab0e
part 1 - Fix race in test_font_loading_api.html. rs=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/023d0b84b7a9
part 2 - Don't resolve FontFaceSet's ready promise in its constructor. r=heycam

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8983374 [details] [diff] [review]
part 1 - Fix race in test_font_loading_api.html.

Review of attachment 8983374 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/test/test_font_loading_api.html
@@ +229,3 @@
>      var p = Promise.resolve();
>      sourceDocuments.forEach(function({ doc, what }) {
> +      p = p.then(doc.fonts.ready).then(function() {

Oops, it seems:

  p.then(doc.fonts.ready)

is essentially the same as not calling .then() at all. The 'onFulfilled' argument must be a function that returns doc.fonts.ready, it can't simply be doc.fonts.ready itself. I'll land a follow-up to fix this (and fix bug 1466903).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4825bdfd6927
part 3 - Fix the use of Promise.then() in test_font_loading_api.html. r=orange

Comment 8

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/94409f81ab0e
https://hg.mozilla.org/mozilla-central/rev/023d0b84b7a9

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8983374 [details] [diff] [review]
part 1 - Fix race in test_font_loading_api.html.

This plus the followup fix seems fine.

Makes me realise though that it would be nice to re-write this whole test file to use async/await...

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4825bdfd6927