Bug 136502 - [CSS Font Loading] Add ready attribute to document.fonts
Summary: [CSS Font Loading] Add ready attribute to document.fonts
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bear Travis
URL: http://dev.w3.org/csswg/css-font-load...
Keywords:
Depends on:
Blocks: 135390 158884
  Show dependency treegraph
 
Reported: 2014-09-03 14:40 PDT by Bear Travis
Modified: 2016-06-20 14:40 PDT (History)
5 users (show)

See Also:


Attachments
Initial Patch (14.50 KB, patch)
2014-11-24 15:38 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updated patch (18.87 KB, patch)
2014-11-25 11:30 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updated patch (21.94 KB, patch)
2014-12-11 10:13 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Updated patch (24.66 KB, patch)
2014-12-15 16:01 PST, Bear Travis
no flags Details | Formatted Diff | Diff
Fixing ready test (24.67 KB, patch)
2014-12-16 10:26 PST, Bear Travis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bear Travis 2014-09-03 14:40:43 PDT
document.fonts.ready should result in a promise that resolves when the document finishes loading fonts.
Comment 1 Bear Travis 2014-11-24 15:38:22 PST
Created attachment 242179 [details]
Initial Patch
Comment 2 Bear Travis 2014-11-25 11:30:56 PST
Created attachment 242206 [details]
Updated patch
Comment 3 Sam Weinig 2014-12-03 15:41:04 PST
Comment on attachment 242206 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242206&action=review

> Source/WebCore/css/FontLoader.h:120
> +    OwnPtr<DeferredWrapper> m_ready;

We have so far been able to keep DeferredWrapper in the bindings layer only, and I would prefer to keep it that way.  Instead, what I recommend is having two std::functions (one for success, one for failure) that capture the DeferredWrapper.  See JSSubtleCrypto::generateKey() for an example of how we do this.
Comment 4 Bear Travis 2014-12-11 10:13:13 PST
Created attachment 243130 [details]
Updated patch
Comment 5 Sam Weinig 2014-12-11 12:36:18 PST
Comment on attachment 243130 [details]
Updated patch

Looks good. I would like to see a test that calls document.fonts.ready multiple times.  I am not clear whether ready should return the same promise each time it is called, but it should be tested either way (and we should verify with the spec author what the intent is).
Comment 6 Bear Travis 2014-12-11 14:02:27 PST
I can add a test containing multiple ready reads. I'm not clear on when/if the ready attribute must return the same value on read, and have a question out to the style list.

Storing the promise without sharing DeferredWrapper with FontLoader seemed a bit tricky to me, as I couldn't find an example of code that added a member variable to the JS layer like JSFontLoaderCustom.h would need to. Would that be the right way, and is it possible?
Comment 7 Bear Travis 2014-12-15 16:01:31 PST
Created attachment 243326 [details]
Updated patch

document.fonts.ready should return the same promise until it has been fulfilled and a font begins loading, at which point document.fonts.ready should return a new, unfulfilled promise. See http://dev.w3.org/csswg/css-font-loading/#FontFaceSet-events
Comment 8 Bear Travis 2014-12-16 10:26:42 PST
Created attachment 243366 [details]
Fixing ready test
Comment 9 Brent Fulgham 2016-03-10 11:25:21 PST
Sam, it looks like he added the test you asked for. Was there anything else you wanted to see here?

Bear: Could you please upload a rebaselined patch we can apply to the tree?
Comment 10 Myles C. Maxfield 2016-03-10 13:53:27 PST
(In reply to comment #9)
> Sam, it looks like he added the test you asked for. Was there anything else
> you wanted to see here?
> 
> Bear: Could you please upload a rebaselined patch we can apply to the tree?

This is done already. (since r196747)

Also, IIRC Bear doesn't work on WebKit anymore.