Summary: | [WebIDL] Add support for converting dictionaries to JS | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||||||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, darin, esprehn+autocc, jason.mei, jiewen_tan, kondapallykalyan, rniwa | ||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 165736 | ||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Sam Weinig
2016-12-03 21:02:06 PST
Created attachment 296079 [details]
Patch
Created attachment 296085 [details]
Patch
Created attachment 296086 [details]
Patch
Created attachment 296088 [details]
Patch
Created attachment 296089 [details]
Patch
Created attachment 296095 [details]
Patch
Created attachment 296096 [details]
Patch
Comment on attachment 296096 [details] Patch Attachment 296096 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2622463 New failing tests: crypto/webkitSubtle/rsassa-pkcs1-v1_5-generate-key-with-leading-zeroes-in-exponent.html crypto/webkitSubtle/rsassa-pkcs1-v1_5-generate-key.html crypto/webkitSubtle/rsa-oaep-key-manipulation.html crypto/workers/subtle/rsa-generate-key.html crypto/subtle/rsa-oaep-generate-key.html crypto/subtle/rsaes-pkcs1-v1_5-generate-key.html crypto/webkitSubtle/rsa-oaep-generate-non-extractable-key.html crypto/subtle/rsaes-pkcs1-v1_5-generate-key-extractable.html crypto/subtle/rsassa-pkcs1-v1_5-generate-key.html Created attachment 296098 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 296096 [details] Patch Attachment 296096 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2622473 New failing tests: crypto/webkitSubtle/rsassa-pkcs1-v1_5-generate-key-with-leading-zeroes-in-exponent.html crypto/webkitSubtle/rsassa-pkcs1-v1_5-generate-key.html crypto/webkitSubtle/rsa-oaep-key-manipulation.html crypto/workers/subtle/rsa-generate-key.html crypto/subtle/rsa-oaep-generate-key.html crypto/subtle/rsaes-pkcs1-v1_5-generate-key.html crypto/webkitSubtle/rsa-oaep-generate-non-extractable-key.html crypto/subtle/rsaes-pkcs1-v1_5-generate-key-extractable.html crypto/subtle/rsassa-pkcs1-v1_5-generate-key.html Created attachment 296099 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 296096 [details] Patch Attachment 296096 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2622475 New failing tests: crypto/webkitSubtle/rsassa-pkcs1-v1_5-generate-key-with-leading-zeroes-in-exponent.html crypto/webkitSubtle/rsassa-pkcs1-v1_5-generate-key.html crypto/webkitSubtle/rsa-oaep-key-manipulation.html crypto/workers/subtle/rsa-generate-key.html crypto/subtle/rsa-oaep-generate-key.html crypto/subtle/rsaes-pkcs1-v1_5-generate-key.html crypto/webkitSubtle/rsa-oaep-generate-non-extractable-key.html crypto/subtle/rsaes-pkcs1-v1_5-generate-key-extractable.html crypto/subtle/rsassa-pkcs1-v1_5-generate-key.html Created attachment 296101 [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.6
Created attachment 296108 [details]
Patch
Created attachment 296109 [details]
Patch
The content of attachment 296101 [details] has been deleted by Sam Weinig <sam@webkit.org> without providing any reason. The token used to delete this attachment was generated at 2016-12-04 15:45:25 PST. Created attachment 296111 [details]
Patch
Attachment 296111 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:62: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
Total errors found: 1 in 50 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 296113 [details]
Patch
Attachment 296113 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:62: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
Total errors found: 1 in 52 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 296113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296113&action=review Not compiling yet on GTK/EFL, maybe because of missing include of JSDOMConvert.h somewhere? r=me assuming you get it to compile there > Source/WebCore/bindings/generic/IDLTypes.h:43 > +template<typename Value> class DOMPromise; I suggest omitting the identifier "Value" here. > Source/WebCore/bindings/js/JSDOMPromise.h:63 > +#if ENABLE(SUBTLE_CRYPTO) > + || std::is_same<DOMClass, CryptoKeyPair>::value; > +#else > + ; > +#endif I’d put the semicolon outside the #if/#endif > Source/WebCore/bindings/js/JSDOMPromise.h:366 > +#if ENABLE(SUBTLE_CRYPTO) I’d put a blank line after this rather than having the #if/endif in the same paragraph as the code inside. > Source/WebCore/bindings/js/JSDOMPromise.h:368 > +template<> > +inline void DeferredPromise::resolve(const CryptoKeyPair& result) I’d merge this onto a single line. > Source/WebCore/bindings/js/JSDOMPromise.h:374 > + JSC::ExecState* exec = m_globalObject->globalExec(); I’d write: auto& state = *m_globalObject->globalExec(); > Source/WebCore/bindings/js/JSDOMPromise.h:376 > + resolve(*exec, toJS<IDLDictionary<CryptoKeyPair>>(*exec, *m_globalObject.get(), result)); Does *m_globalObject, need the ".get()"? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1211 > + if ($dictionary->extendedAttributes->{JSGenerateToJSObject}) { > + $result .= "template<> JSC::JSObject* convertDictionaryToJS(JSC::ExecState&, JSDOMGlobalObject&, const ${className}&);\n\n"; > + } Later I’d like us to make this be automatic based on the use of this dictionary as a return value rather than based on an extended attribute. We could still keep the extended attribute if it was needed, say, for custom bindings. > Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:106 > + callback(KeyOrKeyPair(result)); Can this use { } syntax instead of ()? Can we save a little churn by using WTFMove(result)? Same questions for all the other cases below. > Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:244 > + capturedCallback(KeyOrKeyPair(WTFMove(pair))); Can we use { } syntax here instead of ()? > Source/WebCore/crypto/mac/CryptoKeyRSAMac.cpp:275 > + CryptoKeyPair pair { WTFMove(publicKey), WTFMove(privateKey) }; > + (*localCallback)(WTFMove(pair)); Would read better as a one-liner, I think. (*localCallback)(CryptoKeyPair { WTFMove(publicKey), WTFMove(privateKey) }); > Source/WebCore/html/HTMLMediaElement.h:90 > +template<typename Value> class DOMPromise; I would omit the identifier Value here. Created attachment 296200 [details]
Patch
Attachment 296200 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSDOMPromise.cpp:140: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 75 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 296200 [details] Patch Attachment 296200 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2629564 New failing tests: imported/w3c/web-platform-tests/fetch/api/request/request-init-002.html imported/w3c/web-platform-tests/fetch/api/response/response-consume-empty.html imported/w3c/web-platform-tests/fetch/api/request/request-consume-empty.html webaudio/audiocontext-state.html imported/w3c/web-platform-tests/fetch/api/response/response-init-002.html Created attachment 296213 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 296200 [details] Patch Attachment 296200 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2629573 New failing tests: imported/w3c/web-platform-tests/fetch/api/request/request-init-002.html imported/w3c/web-platform-tests/fetch/api/response/response-init-002.html imported/w3c/web-platform-tests/fetch/api/request/request-consume-empty.html webaudio/audiocontext-state.html imported/w3c/web-platform-tests/fetch/api/response/response-consume-empty.html Created attachment 296214 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 296200 [details] Patch Attachment 296200 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2629629 New failing tests: imported/w3c/web-platform-tests/fetch/api/request/request-init-002.html imported/w3c/web-platform-tests/fetch/api/response/response-consume-empty.html imported/w3c/web-platform-tests/fetch/api/request/request-consume-empty.html imported/w3c/web-platform-tests/fetch/api/response/response-init-002.html Created attachment 296218 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 296219 [details]
Patch
Created attachment 296220 [details]
Patch
Created attachment 296277 [details]
Patch
Comment on attachment 296277 [details] Patch Clearing flags on attachment: 296277 Committed r209390: <http://trac.webkit.org/changeset/209390> All reviewed patches have been landed. Closing bug. *** Bug 163711 has been marked as a duplicate of this bug. *** *** Bug 164868 has been marked as a duplicate of this bug. *** Comment on attachment 296277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=296277&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1359 > + $result .= " if (dictionary.${key}) {\n"; > + $result .= " auto ${key}Value = toJS<$IDLType>(state, globalObject, dictionary.${key});\n"; > + $result .= " result->putDirect(vm, JSC::Identifier::fromString(&vm, \"${key}\"), ${key}Value);\n"; > + $result .= " }\n"; While working on bug 165736 I realized that this works for optional values represented by data members of type RefPtr<>, but doesn’t work with data members of the type std::optional<> or WTF::String. My patch attached to that bug makes it work for those additional types but does not deploy a clean strategy to work across all types. |