Bug 156141 – FontFaceSet binding does not handle null correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Darin Adler]

FontFaceSet binding does not handle null correctly

[Darin Adler]

Created attachment 275489 [details]
Patch

[Alexey Proskuryakov]

Hmm, two EWS bots are failing at once, and both seem to have WindowServer in some broken state, so that DumpRenderTree processes freeze in _CGSReenableUpdateForConnection.

Very curious, but almost certainly unrelated to this patch. I'll reboot the bots for now.

[youenn fablet]

Comment on attachment 275489 [details]
Patch

Sounds good to me.
Some improvement ideas below.

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

> Source/WebCore/css/FontFaceSet.cpp:135
> +    ec = 0;

We might want to remove RaisesException from load FontFaceSet::IDL method declaration.
Instead, in line 138, we can directly reject the promise with ec.

That said, we might want to modify DOMPromise to type the resolve value but not the reject value.
That would be closer to IDL descriptions (like Promise<FontFace>) and remove the need to have RaisesException with DOMPromise.

> Source/WebCore/css/FontFaceSet.cpp:233
> +        if (pendingPromise->hasReachedTerminalState)

hasReachedTerminalState is probably not useful.
It can be computed from DeferredWrapper/DOMPromise.
Once promise is resolved/rejected, DeferredWrapper::m_globalObject is cleared.

> LayoutTests/ChangeLog:9
> +        * fast/text/font-face-set-javascript.html: Added tests for handling of null, also added tests for

There are some IDL tests in web-platform-tests, see for instance LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces.html
It seems that idlharness.js/WebIDLParser.js does not test passing null to not-nullable parameters.
Maybe it can be improved to cover that. That would handle already tested interfaces like Node.

[Myles C. Maxfield]

Comment on attachment 275489 [details]
Patch

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

This looks good. Thanks. r=me

>> Source/WebCore/css/FontFaceSet.cpp:135
>> +    ec = 0;
> 
> We might want to remove RaisesException from load FontFaceSet::IDL method declaration.
> Instead, in line 138, we can directly reject the promise with ec.
> 
> That said, we might want to modify DOMPromise to type the resolve value but not the reject value.
> That would be closer to IDL descriptions (like Promise<FontFace>) and remove the need to have RaisesException with DOMPromise.

The spec states that a syntax error synchronously throws an exception (rather than rejecting a promise).

> Source/WebCore/css/FontFaceSet.idl:-34
> -    UsePointersEvenForNonNullableObjectArguments,

👍

[youenn fablet]

> > We might want to remove RaisesException from load FontFaceSet::IDL method declaration.
> > Instead, in line 138, we can directly reject the promise with ec.
> > 
> > That said, we might want to modify DOMPromise to type the resolve value but not the reject value.
> > That would be closer to IDL descriptions (like Promise<FontFace>) and remove the need to have RaisesException with DOMPromise.
> 
> The spec states that a syntax error synchronously throws an exception
> (rather than rejecting a promise).

That seems inconsistent with the other promise APIs I know.
Is there a rationale or should the spec be updated to reject the promise?

Also, the binding generator is generating code that is checking for exceptions and is rejecting the promise accordingly. It might be good to add a test case.

[WebKit Commit Bot]

Comment on attachment 275489 [details]
Patch

Clearing flags on attachment: 275489

Committed r199216: <http://trac.webkit.org/changeset/199216>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 275489 [details]
Patch

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

> Source/WebCore/css/FontFaceSet.h:102
> +    void startedLoading() final;
> +    void completedLoading() final;
> +    void faceFinished(CSSFontFace&, CSSFontFace::Status) final;

The class itself is marked final! So we could drop all of these inner finals. Not sure if there is a style preference for that.

[Darin Adler]

Comment on attachment 275489 [details]
Patch

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

>>> Source/WebCore/css/FontFaceSet.cpp:135
>>> +    ec = 0;
>> 
>> We might want to remove RaisesException from load FontFaceSet::IDL method declaration.
>> Instead, in line 138, we can directly reject the promise with ec.
>> 
>> That said, we might want to modify DOMPromise to type the resolve value but not the reject value.
>> That would be closer to IDL descriptions (like Promise<FontFace>) and remove the need to have RaisesException with DOMPromise.
> 
> The spec states that a syntax error synchronously throws an exception (rather than rejecting a promise).

Whatever behavior we chose, this should be covered with test cases, to make sure behavior is consistent across browsers.

>> Source/WebCore/css/FontFaceSet.cpp:233
>> +        if (pendingPromise->hasReachedTerminalState)
> 
> hasReachedTerminalState is probably not useful.
> It can be computed from DeferredWrapper/DOMPromise.
> Once promise is resolved/rejected, DeferredWrapper::m_globalObject is cleared.

Sounds like nice idea. If we want to do that optimization, then we need to plumb through some functions to expose that state. DeferredWrapper itself will have to publicize something (not sure what to name it), and DOMPromise will probably also have to do the same.

>> Source/WebCore/css/FontFaceSet.h:102
>> +    void faceFinished(CSSFontFace&, CSSFontFace::Status) final;
> 
> The class itself is marked final! So we could drop all of these inner finals. Not sure if there is a style preference for that.

We did discuss this on webkit-dev, and I specifically proposed that we use final on each function even when the class is final. Not everyone loved the idea, but I think we landed on it and even possibly added it to a coding style document?

I prefer that rule because when refactoring to add a derived class, we don’t end up taking final off all the functions in one fell swoop. On the other hand, marking the class final optimizes calls to all the other non-overrider virtual functions as well, and there is no way to preserve some of that optimization when refactoring to add a derived class.

On the other hand, some people don’t think that final is clear enough; doesn’t imply override to them.

>> LayoutTests/ChangeLog:9
>> +        * fast/text/font-face-set-javascript.html: Added tests for handling of null, also added tests for
> 
> There are some IDL tests in web-platform-tests, see for instance LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces.html
> It seems that idlharness.js/WebIDLParser.js does not test passing null to not-nullable parameters.
> Maybe it can be improved to cover that. That would handle already tested interfaces like Node.

I don’t really understand what you are saying.

My intention is not to to test that the IDL generator works properly. I wanted to test this particular class’s DOM binding.

On the other hand, adding more tests is fine with me!

I was adding a test about the particular thing we had wrong that this patch fixes. I didn’t cover, however, the many other cases that also changed behavior, like passing a non-object or an object for the wrong class. I was sort of using "null" to stand in for those.

[youenn fablet]

> > There are some IDL tests in web-platform-tests, see for instance LayoutTests/imported/w3c/web-platform-tests/html/dom/interfaces.html
> > It seems that idlharness.js/WebIDLParser.js does not test passing null to not-nullable parameters.
> > Maybe it can be improved to cover that. That would handle already tested interfaces like Node.
> 
> I don’t really understand what you are saying.
> 
> My intention is not to to test that the IDL generator works properly. I
> wanted to test this particular class’s DOM binding.
> 
> On the other hand, adding more tests is fine with me!
> 
> I was adding a test about the particular thing we had wrong that this patch
> fixes. I didn’t cover, however, the many other cases that also changed
> behavior, like passing a non-object or an object for the wrong class. I was
> sort of using "null" to stand in for those.

The current added tests are just fine I think.

The IDL tests I mentionned above are regular testharness.js tests.
These tests contain IDL descriptions of some interfaces.
idlharness.js/WebIDLParser.js are processing those IDLs to generate automatically a bunch of tests that run as regular layout tests.
This allows checking for instance that the prototype has the right number of keys.

I am wondering whether the "pass null to non-nullable parameters" test could be generated by idlharness.js/WebIDLParser.js scripts.

[Darin Adler]

(In reply to comment #10)
> I am wondering whether the "pass null to non-nullable parameters" test could
> be generated by idlharness.js/WebIDLParser.js scripts.

Sounds like a good idea.

We need tests for all sorts of things that we may be handling wrong such as:

- passing undefined
- passing null
- passing non-objects
- passing objects of incorrect type

For various combinations of optional/not and nullable/not parameters.