Bug 175526

Summary: [WebIDL] Remove the need for JSSubtleCryptoCustom.cpp
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, darin, dbates, esprehn+autocc, jiewen_tan, kangil.han, kondapallykalyan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch none

Description Sam Weinig 2017-08-13 19:35:03 PDT
[WebIDL] Remove the need for JSSubtleCryptoCustom.cpp
Comment 1 Sam Weinig 2017-08-13 19:44:33 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2017-08-13 19:47:21 PDT Comment hidden (obsolete)
Comment 3 Sam Weinig 2017-08-13 20:39:45 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-08-13 20:41:40 PDT Comment hidden (obsolete)
Comment 5 Sam Weinig 2017-08-13 21:54:10 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-08-13 21:56:53 PDT Comment hidden (obsolete)
Comment 7 Jiewen Tan 2017-08-14 12:01:00 PDT
Comment on attachment 318024 [details]
Patch

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

Looks good to me for the crypto parts. Someone else needs to take care of the binding parts. Please make the patch build on all platforms. : ) Thanks!

> Source/WebCore/ChangeLog:3
> +        [WebIDL] Remove the need for JSSubtleCryptoCustom.cpp

I am guessing this is an effort towards eliminating all the custom binding codes?

> Source/WebCore/crypto/SubtleCrypto.cpp:529
> +    // FIXME: Is this check necessisary? normalizeCryptoAlgorithmParameters should be throwing for any unsupported

This check is not necessary now. Before the feature is completed, we have some extra algorithms listed in the CryptoAlgorithmIdentifier. Therefore, it could be possible that normalizeCryptoAlgorithmParameters don't throw but we actually don't have the implementation. Now, I have removed those extra algorithms. Hence, it should not be a problem anymore.

> Source/WebCore/crypto/SubtleCrypto.cpp:572
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:612
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:653
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:683
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:713
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:806
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:857
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:894
> +    if (UNLIKELY(!algorithm)) {

Ditto.

> Source/WebCore/crypto/SubtleCrypto.cpp:932
> +    if (UNLIKELY(!algorithm)) {

Ditto.
Comment 8 Sam Weinig 2017-08-14 19:30:29 PDT Comment hidden (obsolete)
Comment 9 Sam Weinig 2017-08-14 19:34:58 PDT
(In reply to Jiewen Tan from comment #7)
> Comment on attachment 318024 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318024&action=review
> 
> Looks good to me for the crypto parts. Someone else needs to take care of
> the binding parts. Please make the patch build on all platforms. : ) Thanks!

Yeah, it's not even building on the Mac/iOS bots yet for some reason. Wasn't ready for review but thanks for looking at it :).

> 
> > Source/WebCore/ChangeLog:3
> > +        [WebIDL] Remove the need for JSSubtleCryptoCustom.cpp
> 
> I am guessing this is an effort towards eliminating all the custom binding
> codes?

Yep.

Do you happen to know what is going on with 'webkitSubtleCrypto'? Do we need to keep that around of any reason? For instance, are there known clients that would break if we remove it?

> 
> > Source/WebCore/crypto/SubtleCrypto.cpp:529
> > +    // FIXME: Is this check necessisary? normalizeCryptoAlgorithmParameters should be throwing for any unsupported
> 
> This check is not necessary now. Before the feature is completed, we have
> some extra algorithms listed in the CryptoAlgorithmIdentifier. Therefore, it
> could be possible that normalizeCryptoAlgorithmParameters don't throw but we
> actually don't have the implementation. Now, I have removed those extra
> algorithms. Hence, it should not be a problem anymore.

Cool. Will remove.
Comment 10 Sam Weinig 2017-08-14 19:39:03 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-08-14 19:41:20 PDT Comment hidden (obsolete)
Comment 12 Sam Weinig 2017-08-14 19:55:52 PDT Comment hidden (obsolete)
Comment 13 Build Bot 2017-08-14 19:58:44 PDT Comment hidden (obsolete)
Comment 14 Sam Weinig 2017-08-14 21:05:33 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-08-14 21:08:14 PDT Comment hidden (obsolete)
Comment 16 Sam Weinig 2017-08-15 08:57:41 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-08-15 09:00:47 PDT Comment hidden (obsolete)
Comment 18 Sam Weinig 2017-08-15 09:39:00 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-08-15 09:42:02 PDT Comment hidden (obsolete)
Comment 20 Sam Weinig 2017-08-15 10:24:19 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2017-08-15 10:27:55 PDT Comment hidden (obsolete)
Comment 22 Jiewen Tan 2017-08-15 11:33:34 PDT
(In reply to Sam Weinig from comment #9)
> (In reply to Jiewen Tan from comment #7)
> > Comment on attachment 318024 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318024&action=review
> > 
> > Looks good to me for the crypto parts. Someone else needs to take care of
> > the binding parts. Please make the patch build on all platforms. : ) Thanks!
> 
> Yeah, it's not even building on the Mac/iOS bots yet for some reason. Wasn't
> ready for review but thanks for looking at it :).
> 
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        [WebIDL] Remove the need for JSSubtleCryptoCustom.cpp
> > 
> > I am guessing this is an effort towards eliminating all the custom binding
> > codes?
> 
> Yep.
> 
> Do you happen to know what is going on with 'webkitSubtleCrypto'? Do we need
> to keep that around of any reason? For instance, are there known clients
> that would break if we remove it?

So far I don't know any websites could potentially break because the missing of webkitSubtleCrypto. Since we are the only vendor that ships webkitSubtleCrypto, I bet all the websites using the WebCrypto API have a fallback to SubtleCrypto. Therefore, there shouldn't be any problems of removing the legacy API. I will be glad to remove it.
Comment 23 Sam Weinig 2017-08-15 14:26:01 PDT Comment hidden (obsolete)
Comment 24 Build Bot 2017-08-15 14:30:56 PDT Comment hidden (obsolete)
Comment 25 Sam Weinig 2017-08-15 16:03:54 PDT Comment hidden (obsolete)
Comment 26 Build Bot 2017-08-15 16:09:31 PDT Comment hidden (obsolete)
Comment 27 Sam Weinig 2017-08-15 16:18:23 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2017-08-15 16:21:16 PDT Comment hidden (obsolete)
Comment 29 Build Bot 2017-08-15 17:23:27 PDT Comment hidden (obsolete)
Comment 30 Build Bot 2017-08-15 17:23:28 PDT Comment hidden (obsolete)
Comment 31 Build Bot 2017-08-15 17:49:06 PDT Comment hidden (obsolete)
Comment 32 Build Bot 2017-08-15 17:49:07 PDT Comment hidden (obsolete)
Comment 33 Build Bot 2017-08-15 17:52:18 PDT Comment hidden (obsolete)
Comment 34 Build Bot 2017-08-15 17:52:20 PDT Comment hidden (obsolete)
Comment 35 Sam Weinig 2017-08-15 18:26:55 PDT Comment hidden (obsolete)
Comment 36 Build Bot 2017-08-15 18:29:46 PDT Comment hidden (obsolete)
Comment 37 Sam Weinig 2017-08-15 21:30:35 PDT Comment hidden (obsolete)
Comment 38 Build Bot 2017-08-15 21:33:14 PDT Comment hidden (obsolete)
Comment 39 Build Bot 2017-08-15 23:32:34 PDT Comment hidden (obsolete)
Comment 40 Build Bot 2017-08-15 23:32:35 PDT Comment hidden (obsolete)
Comment 41 Build Bot 2017-08-16 01:34:23 PDT Comment hidden (obsolete)
Comment 42 Build Bot 2017-08-16 01:34:25 PDT Comment hidden (obsolete)
Comment 43 Sam Weinig 2017-08-16 09:32:20 PDT
Created attachment 318265 [details]
Patch
Comment 44 Build Bot 2017-08-16 09:35:24 PDT Comment hidden (obsolete)
Comment 45 Chris Dumez 2017-08-16 10:28:47 PDT
Comment on attachment 318265 [details]
Patch

r=me, nice.
Comment 46 Jiewen Tan 2017-08-16 12:15:14 PDT
Comment on attachment 318265 [details]
Patch

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

r=me for the Crypto parts.

> Source/WebCore/bindings/js/JSDOMConvertUnion.h:214
>          // FIXME: Add support for steps 5 & 6.

Is this comment still needed?
Comment 47 WebKit Commit Bot 2017-08-16 15:11:23 PDT
Comment on attachment 318265 [details]
Patch

Clearing flags on attachment: 318265

Committed r220811: <http://trac.webkit.org/changeset/220811>
Comment 48 WebKit Commit Bot 2017-08-16 15:11:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 49 Radar WebKit Bug Importer 2017-08-16 15:12:06 PDT
<rdar://problem/33927861>
Comment 50 Sam Weinig 2017-08-16 15:23:44 PDT
(In reply to Jiewen Tan from comment #46)
> Comment on attachment 318265 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318265&action=review
> 
> r=me for the Crypto parts.
> 
> > Source/WebCore/bindings/js/JSDOMConvertUnion.h:214
> >          // FIXME: Add support for steps 5 & 6.
> 
> Is this comment still needed?

Yep.