Add support for ImplementedAs, Clamp, EnforceRange, TreatNullAs for dictionary members
Created attachment 304600 [details] Patch
Comment on attachment 304600 [details] Patch Attachment 304600 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3335541 New failing tests: crypto/subtle/aes-gcm-encrypt-malformed-parameters.html crypto/subtle/hmac-generate-key-malformed-parameters.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html crypto/subtle/aes-generate-key-malformed-parameters.html
Created attachment 304604 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 304600 [details] Patch Attachment 304600 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3335546 New failing tests: crypto/subtle/aes-gcm-encrypt-malformed-parameters.html crypto/subtle/hmac-generate-key-malformed-parameters.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html crypto/subtle/aes-generate-key-malformed-parameters.html
Created attachment 304605 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 304600 [details] Patch Attachment 304600 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3335552 New failing tests: crypto/subtle/aes-gcm-encrypt-malformed-parameters.html crypto/subtle/hmac-generate-key-malformed-parameters.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html crypto/subtle/aes-generate-key-malformed-parameters.html
Created attachment 304606 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 304600 [details] Patch Attachment 304600 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3335572 New failing tests: crypto/subtle/aes-gcm-encrypt-malformed-parameters.html crypto/subtle/hmac-generate-key-malformed-parameters.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html crypto/subtle/aes-generate-key-malformed-parameters.html
Created attachment 304610 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Jiewen and Brent, I think this patch is exposing bugs in subtle crypto. [EnforceRange] in AesGcmParams and AesKeyParams now throw TypeErrors. Can you help me determine whether the tests need to be rewritten, or rebaselined?
Comment on attachment 304600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304600&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1570 > + # 5.1.1. Set a different key in case it is aliased with ImplementedAs. > + my $resultKey = $key; > + if ($member->extendedAttributes->{"ImplementedAs"}) { > + $resultKey = $member->extendedAttributes->{"ImplementedAs"}; > + } The 5.1 business is spec step language. Since this implementation specific, it should not have a number. Probably just a comment explaining why it's needed here. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1586 > - $result .= " result.$key = convert<${IDLType}>(state, ${key}Value);\n"; > + > + my @conversionArguments = (); > + push(@conversionArguments, "state"); > + push(@conversionArguments, "${key}Value"); > + push(@conversionArguments, GetIntegerConversionConfiguration($member)) if $codeGenerator->IsIntegerType($type); > + push(@conversionArguments, GetStringConversionConfiguration($member)) if $codeGenerator->IsStringType($type); > + $result .= " result.$resultKey = convert<${IDLType}>(" . join(", ", @conversionArguments) . ");\n"; > $result .= " RETURN_IF_EXCEPTION(throwScope, { });\n"; Ideally we would be able to use JSValueToNative, rather than repeating things here. Probably need to update the list of contexts to include IDLDictionaryMember. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1632 > + # 1.1. Set a different key in case it is aliased with ImplementedAs. > + my $resultKey = $key; > + if ($member->extendedAttributes->{"ImplementedAs"}) { > + $resultKey = $member->extendedAttributes->{"ImplementedAs"}; > + } Again, spec language.
*** Bug 169333 has been marked as a duplicate of this bug. ***
(In reply to comment #10) > Jiewen and Brent, I think this patch is exposing bugs in subtle crypto. > [EnforceRange] in AesGcmParams and AesKeyParams now throw TypeErrors. > > Can you help me determine whether the tests need to be rewritten, or > rebaselined? Ha, I didn't realize that we didn't support EnforceRange before. For tests cases under crypto/, rebaseline should be fine. However, I need to investigate why the w3c test cases fail.
(In reply to comment #13) > (In reply to comment #10) > > Jiewen and Brent, I think this patch is exposing bugs in subtle crypto. > > [EnforceRange] in AesGcmParams and AesKeyParams now throw TypeErrors. > > > > Can you help me determine whether the tests need to be rewritten, or > > rebaselined? > > Ha, I didn't realize that we didn't support EnforceRange before. For tests > cases under crypto/, rebaseline should be fine. However, I need to > investigate why the w3c test cases fail. We can rebaseline the w3c tests as well. In my opinion, the expected results are wrong. We are doing the right thing. I will file bug reports against the w3c tests later on.
Sam, I took a stab at switching to JSValueToNative, and the way enumerations were written out changed. Is this a better way to convert enumerations, or is it wrong? If it's the latter, then I will not attempt to switch, and will add a FIXME and file a new bug to do the conversion. Here's an example of the change: --- WebCore/bindings/scripts/test/JS/JSTestObj.cpp 2017-03-16 10:43:26.000000000 -0700 +++ /var/folders/sf/90xm7_w50ld3z7dmg4lm01ph0000gn/T/tmppGuUgJ/JSTestObj.cpp 2017-03-16 12:43:53.000000000 -0700 @@ -517,19 +517,19 @@ } JSValue enumerationValueWithDefaultValue = isNullOrUndefined ? jsUndefined() : object->get(&state, Identifier::fromString(&state, "enumerationValueWithDefault")); if (!enumerationValueWithDefaultValue.isUndefined()) { - result.enumerationValueWithDefault = convert<IDLEnumeration<TestObj::EnumType>>(state, enumerationValueWithDefaultValue); + result.enumerationValueWithDefault = parseEnumeration<TestObj::EnumType>(state, enumerationValueWithDefaultValue); RETURN_IF_EXCEPTION(throwScope, { }); } else result.enumerationValueWithDefault = TestObj::EnumType::EnumValue1; JSValue enumerationValueWithEmptyStringDefaultValue = isNullOrUndefined ? jsUndefined() : object->get(&state, Identifier::fromString(&state, "enumerationValueWithEmptyStringDefa
Created attachment 304765 [details] Patch
Comment on attachment 304765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304765&action=review > Source/WebCore/ChangeLog:13 > + (JSValueToNative): Bypass parseEnumeration serialization for enums if the context is an This does not explain why we should do this. It is not obvious to me at least. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1566 > + my $implementedAsKey = $key; nit: Could be on one line: my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key; > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1621 > + my $implementedAsKey = $key; nit: Could be on one line: my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key;
Comment on attachment 304765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304765&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5550 > + if ($codeGenerator->IsEnumType($type) && ref($context) ne "IDLDictionaryMember") { Yep, explain this. We might be able to do it a better way in the future.
Comment on attachment 304765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304765&action=review >> Source/WebCore/ChangeLog:13 >> + (JSValueToNative): Bypass parseEnumeration serialization for enums if the context is an > > This does not explain why we should do this. It is not obvious to me at least. I'll add an explanation here and a brief one in the code below. parseEnumeration returns std::optional, where we need it to throw a TypeError instead. To do this, we pass through so that it write out convert<IDLEnumeration<T>>. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1566 >> + my $implementedAsKey = $key; > > nit: Could be on one line: > my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key; Done. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1621 >> + my $implementedAsKey = $key; > > nit: Could be on one line: > my $implementedAsKey = $member->extendedAttributes->{"ImplementedAs"} ? $member->extendedAttributes->{"ImplementedAs"} : $key; Done. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:5550 >> + if ($codeGenerator->IsEnumType($type) && ref($context) ne "IDLDictionaryMember") { > > Yep, explain this. We might be able to do it a better way in the future. Done.
Created attachment 304846 [details] Patch for landing
Comment on attachment 304846 [details] Patch for landing Attachment 304846 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3353570 New failing tests: crypto/subtle/aes-gcm-encrypt-malformed-parameters.html crypto/subtle/hmac-generate-key-malformed-parameters.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm.html imported/w3c/web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker.html crypto/subtle/aes-generate-key-malformed-parameters.html
Created attachment 304854 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 304855 [details] Patch for landing
Comment on attachment 304855 [details] Patch for landing Clearing flags on attachment: 304855 Committed r214139: <http://trac.webkit.org/changeset/214139>
Comment on attachment 304765 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304765&action=review > LayoutTests/imported/w3c/ChangeLog:10 > + Rebaseline tests. The results seem to show a problem in the original tests. > + * web-platform-tests/WebCryptoAPI/encrypt_decrypt/aes_gcm.worker-expected.txt: > + * web-platform-tests/WebCryptoAPI/encrypt_decrypt/test_aes_gcm-expected.txt: Do these failures occur in other browsers?