Bug 157451 – Next step on dictionary bindings, along with other bindings refinements

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Next step on dictionary bindings, along with other bindings refinements

[Darin Adler]

Created attachment 278326 [details]
Patch

[Chris Dumez]

Comment on attachment 278326 [details]
Patch

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

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:968
> +            if ($needExceptionCheck) {

I find it odd we don't do this after the convert<>() call below. It looks like like you want to avoid the exception check for the last dictionary member but I would personally do the exception check after the last member conversion as well.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:969
> +                $result .= "    if (state.hadException())\n";

UNLIKELY()

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:973
> +            my $function = $member->isOptional ? "convertOptional" : "convert";

We will also need to deal with nullable types at some point.

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:509
> +    auto featureSettings = convertOptional<String>(state, propertyValue(state, value, "featureSettings"), "normal");

Aren't we missing an exception check after this one?

> Source/WebCore/css/FontFace.idl:33
> +dictionary FontFaceDescriptors {

Looks just like the spec, nice!

[Chris Dumez]

Comment on attachment 278326 [details]
Patch

Note that there are test failures that look related to your change though:
Regressions: Unexpected text-only failures (2)
  fast/text/font-face-set-document.html [ Failure ]
  fast/text/font-face-set-javascript.html [ Failure ]

Regressions: Unexpected timeouts (1)
  fast/text/font-loading-csp-block-all.html [ Timeout ]

[Build Bot]

Comment on attachment 278326 [details]
Patch

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

New failing tests:
fast/text/font-loading-csp-block-all.html
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278328 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Build Bot]

Comment on attachment 278326 [details]
Patch

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

New failing tests:
fast/text/font-loading-csp-block-all.html
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

Created attachment 278329 [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 278326 [details]
Patch

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

New failing tests:
fast/text/font-loading-csp-block-all.html
fast/text/font-face-set-document.html
fast/text/font-face-set-javascript.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 278326 [details]
Patch

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

New failing tests:
fast/text/font-loading-csp-block-all.html
fast/text/font-face-set-javascript.html
fast/text/font-face-set-document.html

[Build Bot]

Created attachment 278331 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Darin Adler]

Comment on attachment 278326 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:968
>> +            if ($needExceptionCheck) {
> 
> I find it odd we don't do this after the convert<>() call below. It looks like like you want to avoid the exception check for the last dictionary member but I would personally do the exception check after the last member conversion as well.

We don’t need to do an exception check before building the structure. It slightly optimizes the exception case while slowing down the success case. I would like to leave it this way, at least for now.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:969
>> +                $result .= "    if (state.hadException())\n";
> 
> UNLIKELY()

Good catch, I will fix that (while I fix the failing tests).

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:973
>> +            my $function = $member->isOptional ? "convertOptional" : "convert";
> 
> We will also need to deal with nullable types at some point.

Absolutely. There’s a lot more to deal with, and I want to share much more of the code with the code that sets up function call arguments!

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:509
>> +    auto featureSettings = convertOptional<String>(state, propertyValue(state, value, "featureSettings"), "normal");
> 
> Aren't we missing an exception check after this one?

No, that’s what I was talking about above. The caller of this function is going to do an exception check. We don’t need an extra exception check before building the structure. It wouldn’t do major harm, but it would add an extra branch.

On the other hand, I think I realized that I will need to add exception checks after calling propertyValue, but *before* calling convert.

[Darin Adler]

Committed r200547: <http://trac.webkit.org/changeset/200547>