Bug 165367 – [WebIDL] Add support for converting dictionaries to JS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebIDL] Add support for converting dictionaries to JS

[Sam Weinig]

Created attachment 296079 [details]
Patch

[Sam Weinig]

Created attachment 296085 [details]
Patch

[Sam Weinig]

Created attachment 296086 [details]
Patch

[Sam Weinig]

Created attachment 296088 [details]
Patch

[Sam Weinig]

Created attachment 296089 [details]
Patch

[Sam Weinig]

Created attachment 296095 [details]
Patch

[Sam Weinig]

Created attachment 296096 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

Created attachment 296108 [details]
Patch

[Sam Weinig]

Created attachment 296109 [details]
Patch

[Sam Weinig]

The content of attachment 296101 [details] has been deleted by
    Sam Weinig <[e-mail]>
without providing any reason.

The token used to delete this attachment was generated at 2016-12-04 15:45:25 PST.

[Sam Weinig]

Created attachment 296111 [details]
Patch

[WebKit Commit Bot]

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.

[Sam Weinig]

Created attachment 296113 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Sam Weinig]

Created attachment 296200 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Sam Weinig]

Created attachment 296219 [details]
Patch

[Sam Weinig]

Created attachment 296220 [details]
Patch

[Sam Weinig]

Created attachment 296277 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 296277 [details]
Patch

Clearing flags on attachment: 296277

Committed r209390: <http://trac.webkit.org/changeset/209390>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jiewen Tan]

*** Bug 163711 has been marked as a duplicate of this bug. ***

[Jiewen Tan]

*** Bug 164868 has been marked as a duplicate of this bug. ***

[Darin Adler]

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.