Next step on dictionary bindings, along with other bindings refinements
Created attachment 278326 [details] Patch
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!
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 ]
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
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
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
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
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
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
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
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
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.
Committed r200547: <http://trac.webkit.org/changeset/200547>