Bug 165367

Summary: [WebIDL] Add support for converting dictionaries to JS
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews106 for mac-yosemite-wk2
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews104 for mac-yosemite-wk2
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch none

Description Sam Weinig 2016-12-03 21:02:06 PST
[WebIDL] Add support for converting dictionaries to JS
Comment 1 Sam Weinig 2016-12-03 21:04:05 PST
Created attachment 296079 [details]
Patch
Comment 2 Sam Weinig 2016-12-04 07:46:54 PST
Created attachment 296085 [details]
Patch
Comment 3 Sam Weinig 2016-12-04 08:21:19 PST
Created attachment 296086 [details]
Patch
Comment 4 Sam Weinig 2016-12-04 10:13:45 PST
Created attachment 296088 [details]
Patch
Comment 5 Sam Weinig 2016-12-04 10:43:46 PST
Created attachment 296089 [details]
Patch
Comment 6 Sam Weinig 2016-12-04 12:05:50 PST
Created attachment 296095 [details]
Patch
Comment 7 Sam Weinig 2016-12-04 12:08:19 PST
Created attachment 296096 [details]
Patch
Comment 8 Build Bot 2016-12-04 13:16:03 PST
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
Comment 9 Build Bot 2016-12-04 13:16:07 PST
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 10 Build Bot 2016-12-04 13:21:37 PST
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
Comment 11 Build Bot 2016-12-04 13:21:40 PST
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 12 Build Bot 2016-12-04 13:30:37 PST
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
Comment 13 Build Bot 2016-12-04 13:30:41 PST
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
Comment 14 Sam Weinig 2016-12-04 15:38:40 PST
Created attachment 296108 [details]
Patch
Comment 15 Sam Weinig 2016-12-04 15:45:00 PST
Created attachment 296109 [details]
Patch
Comment 16 Sam Weinig 2016-12-04 15:45:27 PST
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.
Comment 17 Sam Weinig 2016-12-04 16:44:12 PST
Created attachment 296111 [details]
Patch
Comment 18 WebKit Commit Bot 2016-12-04 16:45:15 PST
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.
Comment 19 Sam Weinig 2016-12-04 17:24:54 PST
Created attachment 296113 [details]
Patch
Comment 20 WebKit Commit Bot 2016-12-04 17:26:45 PST
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 21 Darin Adler 2016-12-05 09:28:59 PST
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.
Comment 22 Sam Weinig 2016-12-05 15:20:48 PST
Created attachment 296200 [details]
Patch
Comment 23 WebKit Commit Bot 2016-12-05 15:23:40 PST
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 24 Build Bot 2016-12-05 16:28:34 PST
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
Comment 25 Build Bot 2016-12-05 16:28:38 PST
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 26 Build Bot 2016-12-05 16:32:36 PST
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
Comment 27 Build Bot 2016-12-05 16:32:40 PST
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 28 Build Bot 2016-12-05 16:46:06 PST
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
Comment 29 Build Bot 2016-12-05 16:46:11 PST
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
Comment 30 Sam Weinig 2016-12-05 16:51:40 PST
Created attachment 296219 [details]
Patch
Comment 31 Sam Weinig 2016-12-05 16:53:15 PST
Created attachment 296220 [details]
Patch
Comment 32 Sam Weinig 2016-12-05 23:07:07 PST
Created attachment 296277 [details]
Patch
Comment 33 WebKit Commit Bot 2016-12-05 23:59:30 PST
Comment on attachment 296277 [details]
Patch

Clearing flags on attachment: 296277

Committed r209390: <http://trac.webkit.org/changeset/209390>
Comment 34 WebKit Commit Bot 2016-12-05 23:59:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 35 Jiewen Tan 2016-12-07 11:40:59 PST
*** Bug 163711 has been marked as a duplicate of this bug. ***
Comment 36 Jiewen Tan 2016-12-07 11:41:27 PST
*** Bug 164868 has been marked as a duplicate of this bug. ***
Comment 37 Darin Adler 2016-12-10 17:28:16 PST
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.