Bug 157310 – Rework FontFace promise attribute handling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Current implementation requires passing an ExecState to create the promise.
It may be better to create the promise in custom binding code.

[youenn fablet]

Created attachment 277992 [details]
WIP

[youenn fablet]

Created attachment 277994 [details]
WIP

[Build Bot]

Comment on attachment 277994 [details]
WIP

Attachment 277994 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1261204

New failing tests:
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 277996 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277994 [details]
WIP

Attachment 277994 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1261205

New failing tests:
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 277997 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277994 [details]
WIP

Attachment 277994 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1261299

New failing tests:
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278003 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277994 [details]
WIP

Attachment 277994 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1261307

New failing tests:
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278004 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[youenn fablet]

Created attachment 278083 [details]
Patch

[youenn fablet]

Created attachment 278092 [details]
Improving style

[Myles C. Maxfield]

Comment on attachment 278092 [details]
Improving style

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

This is good. Thanks for doing this.

> Source/WebCore/bindings/js/JSFontFaceCustom.cpp:-41
> -    return promise.deferred().promise();

I believe I added the deferred() function because of this use. We should try to remove that function now that the use use gone.

> Source/WebCore/bindings/js/JSFontFaceCustom.cpp:43
> +        wrapped().loaded(WTFMove(promise));

Because the semantics of this internal function have changed (rather than being an accessor, it is now a mutator), I don't think we should keep the name. Perhaps something like registerPromise()?

> Source/WebCore/css/FontFace.cpp:350
> +        ASSERT_NOT_REACHED();

I think we should replace the "default" with "CSSFontFace::Pending" to guarantee the case list is exhaustive.

> Source/WebCore/css/FontFace.cpp:364
> +        promise.resolve(*this);

Perhaps we could add some ASSERTs around that m_promise is always in a non-terminal state (but this may be more trouble than it's worth).

> Source/WebCore/css/FontFace.cpp:370
> +    default:

Same comment as above - we should make the list exhaustive instead of providing a default:

> Source/WebCore/css/FontFaceSet.cpp:171
> +void FontFaceSet::ready(Promise&& promise)

Same comment as above - I think we should rename this function.

> Source/WebCore/css/FontFaceSet.cpp:207
> +    m_isReady = true;

It appears that m_isReady is only true iff m_promise is Nullopt. Perhaps we can remove the redundancy.

[youenn fablet]

> > Source/WebCore/bindings/js/JSFontFaceCustom.cpp:-41
> > -    return promise.deferred().promise();
> 
> I believe I added the deferred() function because of this use. We should try
> to remove that function now that the use use gone.

OK, I will check it.

> > Source/WebCore/bindings/js/JSFontFaceCustom.cpp:43
> > +        wrapped().loaded(WTFMove(promise));
> 
> Because the semantics of this internal function have changed (rather than
> being an accessor, it is now a mutator), I don't think we should keep the
> name. Perhaps something like registerPromise()?

I am not quite sure about it, this is somehow the standard behavior for cached attribute.
Let's rename it to registerLoaded.

> > Source/WebCore/css/FontFace.cpp:350
> > +        ASSERT_NOT_REACHED();
> 
> I think we should replace the "default" with "CSSFontFace::Pending" to
> guarantee the case list is exhaustive.

OK

> > Source/WebCore/css/FontFace.cpp:364
> > +        promise.resolve(*this);
> 
> Perhaps we could add some ASSERTs around that m_promise is always in a
> non-terminal state (but this may be more trouble than it's worth).

This is done within promise.resolve call.

> > Source/WebCore/css/FontFace.cpp:370
> > +    default:
> 
> Same comment as above - we should make the list exhaustive instead of
> 

OK

> > Source/WebCore/css/FontFaceSet.cpp:171
> > +void FontFaceSet::ready(Promise&& promise)
> 
> Same comment as above - I think we should rename this function.

Let's change it to registerReady.

> > Source/WebCore/css/FontFaceSet.cpp:207
> > +    m_isReady = true;
> 
> It appears that m_isReady is only true iff m_promise is Nullopt. Perhaps we
> can remove the redundancy.

We need to know whether the set is already loaded, so as to resolve the promise when registering it loading is completed.
We can probably remove m_isReady if FontFaceSet backing has this information.

[youenn fablet]

Created attachment 278334 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 278334 [details]
Patch for landing

Clearing flags on attachment: 278334

Committed r200546: <http://trac.webkit.org/changeset/200546>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.