Bug 163541 - [WebIDL] Support BufferSource
Summary: [WebIDL] Support BufferSource
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-17 05:32 PDT by Zan Dobersek
Modified: 2016-10-18 11:01 PDT (History)
4 users (show)

See Also:


Attachments
WIP (101.21 KB, patch)
2016-10-17 05:34 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP (108.49 KB, patch)
2016-10-17 09:37 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP (116.77 KB, patch)
2016-10-17 10:57 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
WIP (16.71 KB, patch)
2016-10-17 11:25 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (22.99 KB, patch)
2016-10-17 23:22 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (22.19 KB, patch)
2016-10-18 03:39 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2016-10-17 05:32:58 PDT
[WebIDL] Support BufferSource
Comment 1 Zan Dobersek 2016-10-17 05:34:29 PDT
Created attachment 291812 [details]
WIP
Comment 2 WebKit Commit Bot 2016-10-17 05:39:12 PDT
Attachment 291812 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:72:  CryptoAlgorithmRSA_OAEP::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:84:  CryptoAlgorithmRSA_OAEP::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:38:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:50:  CryptoAlgorithmAES_CBC::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:62:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:99:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp:38:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp:50:  CryptoAlgorithmRSA_OAEP::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:38:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:59:  CryptoAlgorithmRSA_OAEP::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:38:  transformAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:85:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:90:  CryptoAlgorithmAES_CBC::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_KWGnuTLS.cpp:37:  CryptoAlgorithmAES_KW::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_KWGnuTLS.cpp:48:  CryptoAlgorithmAES_KW::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:68:  CryptoAlgorithmAES_CBC::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:80:  CryptoAlgorithmAES_CBC::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:36:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:50:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:67:  CryptoAlgorithmAES_KW::encryptForWrapKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:77:  CryptoAlgorithmAES_KW::decryptForUnwrapKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:72:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:84:  CryptoAlgorithmRSASSA_PKCS1_v1_5::verify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:68:  CryptoAlgorithmRSAES_PKCS1_v1_5::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:78:  CryptoAlgorithmRSAES_PKCS1_v1_5::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:50:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSAES_PKCS1_v1_5GnuTLS.cpp:37:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSAES_PKCS1_v1_5GnuTLS.cpp:48:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_KWMac.cpp:37:  CryptoAlgorithmAES_KW::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_KWMac.cpp:57:  CryptoAlgorithmAES_KW::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:673:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:774:  Missing space before {  [whitespace/braces] [5]
Total errors found: 34 in 50 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 youenn fablet 2016-10-17 07:02:17 PDT
Comment on attachment 291812 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=291812&action=review

> Source/WebCore/bindings/js/JSDOMConvert.h:657
> +        throwTypeError(&state, scope);

I would add an error message explaining the reason of the error.

> Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:-169
> -    auto success = cryptoOperationDataFromJSValue(&state, state.uncheckedArgument(2), data);

Can we remove Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp as well?

> Source/WebCore/crypto/WebKitSubtleCrypto.idl:-35
> -    [Custom] Promise digest(AlgorithmIdentifier algorithm, sequence<CryptoOperationData> data);

This is a legacy API and is not supposed to be changed.
But at the same time, all of these methods are custom so it is probably fine.
Comment 4 youenn fablet 2016-10-17 07:03:29 PDT
> Can we remove Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp as
> well?
Would bad copy-paste..
I was meaning "Can we remove cryptoOperationDataFromJSValue function"...
Comment 5 Zan Dobersek 2016-10-17 08:21:33 PDT
(In reply to comment #4)
> > Can we remove Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp as
> > well?
> Would bad copy-paste..
> I was meaning "Can we remove cryptoOperationDataFromJSValue function"...

The two uses of cryptoOperationDataFromJSValue() in JSCryptoAlgorithmDictionary.cpp can be replaced with BufferSource as well. That would enable removing the JSCryptoOperationData files.
Comment 6 Zan Dobersek 2016-10-17 09:37:43 PDT
Created attachment 291824 [details]
WIP

Need to fix xcode builds.
Comment 7 Jiewen Tan 2016-10-17 10:43:33 PDT
Hi Youenn and Zan,

Thank you for making the support for BufferSource. I would definitely like to use the BufferSource in my new WebCrypto API once it is ready. However, my opinion of the old interface remains the same: keep it intact. I am not quite sure why there is a must to change the old CryptoOperationData to BufferSource. Even though this change probably wouldn't break anything, I just don't see a must for replacing CryptoOperationData with BufferSource.
Comment 8 youenn fablet 2016-10-17 10:54:59 PDT
(In reply to comment #7)
> Hi Youenn and Zan,
> 
> Thank you for making the support for BufferSource. I would definitely like
> to use the BufferSource in my new WebCrypto API once it is ready. However,
> my opinion of the old interface remains the same: keep it intact. I am not
> quite sure why there is a must to change the old CryptoOperationData to
> BufferSource. Even though this change probably wouldn't break anything, I
> just don't see a must for replacing CryptoOperationData with BufferSource.

From the legacy crypto API point of view, AFAICS, it is just renaming the cryptoOperationDataFromJSValue function. I think it is good to do that since the function is more generic than crypto and we do not want twice the same method in our code base.

The change to WebKit legacy crypto IDL is not actually needed, it does not change anything since all methods are custom I think. But it clarifies things since the custom binding is not supporting sequence<> parameters.
Comment 9 Zan Dobersek 2016-10-17 10:57:07 PDT
Created attachment 291833 [details]
WIP
Comment 10 WebKit Commit Bot 2016-10-17 11:01:07 PDT
Attachment 291833 [details] did not pass style-queue:


ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:72:  CryptoAlgorithmRSA_OAEP::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSA_OAEP.cpp:84:  CryptoAlgorithmRSA_OAEP::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:38:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_CBCGnuTLS.cpp:50:  CryptoAlgorithmAES_CBC::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:62:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSASSA_PKCS1_v1_5Mac.cpp:99:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:38:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSA_OAEPMac.cpp:59:  CryptoAlgorithmRSA_OAEP::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:38:  transformAES_CBC is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:85:  CryptoAlgorithmAES_CBC::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_CBCMac.cpp:90:  CryptoAlgorithmAES_CBC::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_KWGnuTLS.cpp:37:  CryptoAlgorithmAES_KW::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmAES_KWGnuTLS.cpp:48:  CryptoAlgorithmAES_KW::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:68:  CryptoAlgorithmAES_CBC::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_CBC.cpp:80:  CryptoAlgorithmAES_CBC::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:36:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmRSAES_PKCS1_v1_5Mac.cpp:50:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:67:  CryptoAlgorithmAES_KW::encryptForWrapKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmAES_KW.cpp:77:  CryptoAlgorithmAES_KW::decryptForUnwrapKey is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:72:  CryptoAlgorithmRSASSA_PKCS1_v1_5::sign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSASSA_PKCS1_v1_5.cpp:84:  CryptoAlgorithmRSASSA_PKCS1_v1_5::verify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp:38:  CryptoAlgorithmRSA_OAEP::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp:50:  CryptoAlgorithmRSA_OAEP::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:68:  CryptoAlgorithmRSAES_PKCS1_v1_5::encrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/algorithms/CryptoAlgorithmRSAES_PKCS1_v1_5.cpp:78:  CryptoAlgorithmRSAES_PKCS1_v1_5::decrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:38:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformSign is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp:50:  CryptoAlgorithmRSASSA_PKCS1_v1_5::platformVerify is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSAES_PKCS1_v1_5GnuTLS.cpp:37:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/gnutls/CryptoAlgorithmRSAES_PKCS1_v1_5GnuTLS.cpp:48:  CryptoAlgorithmRSAES_PKCS1_v1_5::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_KWMac.cpp:37:  CryptoAlgorithmAES_KW::platformEncrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/crypto/mac/CryptoAlgorithmAES_KWMac.cpp:57:  CryptoAlgorithmAES_KW::platformDecrypt is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:673:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:774:  Missing space before {  [whitespace/braces] [5]
Total errors found: 34 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Zan Dobersek 2016-10-17 11:05:43 PDT
(In reply to comment #7)
> Hi Youenn and Zan,
> 
> Thank you for making the support for BufferSource. I would definitely like
> to use the BufferSource in my new WebCrypto API once it is ready. However,
> my opinion of the old interface remains the same: keep it intact. I am not
> quite sure why there is a must to change the old CryptoOperationData to
> BufferSource. Even though this change probably wouldn't break anything, I
> just don't see a must for replacing CryptoOperationData with BufferSource.

FWIW I'm totally fine reverting any changes to the crypto interfaces. I started with those because that's where I spotted BufferSource first, but later also found it could be used on the SourceBuffer interface.
Comment 12 Zan Dobersek 2016-10-17 11:25:41 PDT
Created attachment 291840 [details]
WIP

Loses the crypto changes, should build everywhere, needs a bindings test.
Comment 13 Jiewen Tan 2016-10-17 11:27:00 PDT
(In reply to comment #11)
> (In reply to comment #7)
> > Hi Youenn and Zan,
> > 
> > Thank you for making the support for BufferSource. I would definitely like
> > to use the BufferSource in my new WebCrypto API once it is ready. However,
> > my opinion of the old interface remains the same: keep it intact. I am not
> > quite sure why there is a must to change the old CryptoOperationData to
> > BufferSource. Even though this change probably wouldn't break anything, I
> > just don't see a must for replacing CryptoOperationData with BufferSource.
> 
> FWIW I'm totally fine reverting any changes to the crypto interfaces. I
> started with those because that's where I spotted BufferSource first, but
> later also found it could be used on the SourceBuffer interface.

Thank you. Actually the old spec the old interface is built against doesn't use BufferSource. So I will really appreciate it if you can revert your changes against to the crypto interfaces.
Comment 14 WebKit Commit Bot 2016-10-17 11:30:11 PDT
Attachment 291840 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Jiewen Tan 2016-10-17 11:30:30 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Hi Youenn and Zan,
> > 
> > Thank you for making the support for BufferSource. I would definitely like
> > to use the BufferSource in my new WebCrypto API once it is ready. However,
> > my opinion of the old interface remains the same: keep it intact. I am not
> > quite sure why there is a must to change the old CryptoOperationData to
> > BufferSource. Even though this change probably wouldn't break anything, I
> > just don't see a must for replacing CryptoOperationData with BufferSource.
> 
> From the legacy crypto API point of view, AFAICS, it is just renaming the
> cryptoOperationDataFromJSValue function. I think it is good to do that since
> the function is more generic than crypto and we do not want twice the same
> method in our code base.
> 
> The change to WebKit legacy crypto IDL is not actually needed, it does not
> change anything since all methods are custom I think. But it clarifies
> things since the custom binding is not supporting sequence<> parameters.

I don't quite get why cryptoOperationDataFromJSValue is more generic than crypto. Besides the old WebCrypto interface, I don't see any other places that use it. In my new implementation, I don't have any plan to use this function at least at this point.
Comment 16 Zan Dobersek 2016-10-17 23:22:29 PDT
Created attachment 291921 [details]
Patch
Comment 17 youenn fablet 2016-10-18 00:54:24 PDT
Comment on attachment 291921 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291921&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:26
> +#include "BufferSource.h"

Can we do without this include here?

> Source/WebCore/bindings/js/JSDOMConvert.h:645
> +    using ReturnType = BufferSource;

Not sure this adds much?

> Source/WebCore/bindings/scripts/CodeGenerator.pm:575
> +sub IsBufferSourceType

Not sure this function is really needed.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:878
> +    return 0 if $object->IsTypedArrayType($type) || $object->IsBufferSourceType($type);

If we remove IsBufferSourceType, we could write return 0 if $type eq "BufferSource" instead which sounds good enough.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:322
> +    return if $codeGenerator->IsTypedArrayType($interfaceName) || $codeGenerator->IsBufferSourceType($interfaceName);

Maybe do the three checks in the same way: the first is separated from IsTypedArrayType and the next twos are combined.
Comment 18 youenn fablet 2016-10-18 01:00:20 PDT
> I don't quite get why cryptoOperationDataFromJSValue is more generic than
> crypto. Besides the old WebCrypto interface, I don't see any other places
> that use it. In my new implementation, I don't have any plan to use this
> function at least at this point.

There is no crypto-specific bits in cryptoOperationDataFromJSValue, it is just converting a BufferSource in a struct that can be used more easily by WebCore.

In general, the fact that the Crypto API is legacy and its functionality should be kept the same does not prevent us to do some refactoring to improve the code base (removing duplicate code).

I agree this could be fixed as a follow-up patch.
Comment 19 Zan Dobersek 2016-10-18 03:39:16 PDT
Created attachment 291935 [details]
Patch for landing
Comment 20 Zan Dobersek 2016-10-18 06:18:25 PDT
Comment on attachment 291935 [details]
Patch for landing

Clearing flags on attachment: 291935

Committed r207462: <http://trac.webkit.org/changeset/207462>
Comment 21 Zan Dobersek 2016-10-18 06:18:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Jiewen Tan 2016-10-18 11:01:25 PDT
(In reply to comment #18)
> > I don't quite get why cryptoOperationDataFromJSValue is more generic than
> > crypto. Besides the old WebCrypto interface, I don't see any other places
> > that use it. In my new implementation, I don't have any plan to use this
> > function at least at this point.
> 
> There is no crypto-specific bits in cryptoOperationDataFromJSValue, it is
> just converting a BufferSource in a struct that can be used more easily by
> WebCore.
> 
> In general, the fact that the Crypto API is legacy and its functionality
> should be kept the same does not prevent us to do some refactoring to
> improve the code base (removing duplicate code).
> 
> I agree this could be fixed as a follow-up patch.

I see. We could remove this function or the file together when we deprecate the whole WebkitSubtleCrypto thing.