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

Sam Weinig
Reported 2017-08-13 19:35:03 PDT
[WebIDL] Remove the need for JSSubtleCryptoCustom.cpp
Attachments
Patch (250.27 KB, patch)
2017-08-13 19:44 PDT, Sam Weinig
no flags
Patch (250.19 KB, patch)
2017-08-13 20:39 PDT, Sam Weinig
no flags
Patch (251.69 KB, patch)
2017-08-13 21:54 PDT, Sam Weinig
no flags
Patch (252.00 KB, patch)
2017-08-14 19:30 PDT, Sam Weinig
no flags
Patch (241.35 KB, patch)
2017-08-14 19:39 PDT, Sam Weinig
no flags
Patch (213.25 KB, patch)
2017-08-14 19:55 PDT, Sam Weinig
no flags
Patch (238.94 KB, patch)
2017-08-14 21:05 PDT, Sam Weinig
no flags
Patch (239.51 KB, patch)
2017-08-15 08:57 PDT, Sam Weinig
no flags
Patch (239.52 KB, patch)
2017-08-15 09:39 PDT, Sam Weinig
no flags
Patch (240.24 KB, patch)
2017-08-15 10:24 PDT, Sam Weinig
no flags
Patch (240.95 KB, patch)
2017-08-15 14:26 PDT, Sam Weinig
no flags
Patch (237.89 KB, patch)
2017-08-15 16:03 PDT, Sam Weinig
no flags
Patch (237.89 KB, patch)
2017-08-15 16:18 PDT, Sam Weinig
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (1.54 MB, application/zip)
2017-08-15 17:23 PDT, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-elcapitan (2.11 MB, application/zip)
2017-08-15 17:49 PDT, Build Bot
no flags
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.22 MB, application/zip)
2017-08-15 17:52 PDT, Build Bot
no flags
Patch (240.60 KB, patch)
2017-08-15 18:26 PDT, Sam Weinig
no flags
Patch (238.75 KB, patch)
2017-08-15 21:30 PDT, Sam Weinig
no flags
Archive of layout-test-results from ews114 for mac-elcapitan (2.91 MB, application/zip)
2017-08-15 23:32 PDT, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-elcapitan (2.90 MB, application/zip)
2017-08-16 01:34 PDT, Build Bot
no flags
Patch (240.45 KB, patch)
2017-08-16 09:32 PDT, Sam Weinig
no flags
Sam Weinig
Comment 1 2017-08-13 19:44:33 PDT Comment hidden (obsolete)
Build Bot
Comment 2 2017-08-13 19:47:21 PDT Comment hidden (obsolete)
Sam Weinig
Comment 3 2017-08-13 20:39:45 PDT Comment hidden (obsolete)
Build Bot
Comment 4 2017-08-13 20:41:40 PDT Comment hidden (obsolete)
Sam Weinig
Comment 5 2017-08-13 21:54:10 PDT Comment hidden (obsolete)
Build Bot
Comment 6 2017-08-13 21:56:53 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 7 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.
Sam Weinig
Comment 8 2017-08-14 19:30:29 PDT Comment hidden (obsolete)
Sam Weinig
Comment 9 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.
Sam Weinig
Comment 10 2017-08-14 19:39:03 PDT Comment hidden (obsolete)
Build Bot
Comment 11 2017-08-14 19:41:20 PDT Comment hidden (obsolete)
Sam Weinig
Comment 12 2017-08-14 19:55:52 PDT Comment hidden (obsolete)
Build Bot
Comment 13 2017-08-14 19:58:44 PDT Comment hidden (obsolete)
Sam Weinig
Comment 14 2017-08-14 21:05:33 PDT Comment hidden (obsolete)
Build Bot
Comment 15 2017-08-14 21:08:14 PDT Comment hidden (obsolete)
Sam Weinig
Comment 16 2017-08-15 08:57:41 PDT Comment hidden (obsolete)
Build Bot
Comment 17 2017-08-15 09:00:47 PDT Comment hidden (obsolete)
Sam Weinig
Comment 18 2017-08-15 09:39:00 PDT Comment hidden (obsolete)
Build Bot
Comment 19 2017-08-15 09:42:02 PDT Comment hidden (obsolete)
Sam Weinig
Comment 20 2017-08-15 10:24:19 PDT Comment hidden (obsolete)
Build Bot
Comment 21 2017-08-15 10:27:55 PDT Comment hidden (obsolete)
Jiewen Tan
Comment 22 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.
Sam Weinig
Comment 23 2017-08-15 14:26:01 PDT Comment hidden (obsolete)
Build Bot
Comment 24 2017-08-15 14:30:56 PDT Comment hidden (obsolete)
Sam Weinig
Comment 25 2017-08-15 16:03:54 PDT Comment hidden (obsolete)
Build Bot
Comment 26 2017-08-15 16:09:31 PDT Comment hidden (obsolete)
Sam Weinig
Comment 27 2017-08-15 16:18:23 PDT Comment hidden (obsolete)
Build Bot
Comment 28 2017-08-15 16:21:16 PDT Comment hidden (obsolete)
Build Bot
Comment 29 2017-08-15 17:23:27 PDT Comment hidden (obsolete)
Build Bot
Comment 30 2017-08-15 17:23:28 PDT Comment hidden (obsolete)
Build Bot
Comment 31 2017-08-15 17:49:06 PDT Comment hidden (obsolete)
Build Bot
Comment 32 2017-08-15 17:49:07 PDT Comment hidden (obsolete)
Build Bot
Comment 33 2017-08-15 17:52:18 PDT Comment hidden (obsolete)
Build Bot
Comment 34 2017-08-15 17:52:20 PDT Comment hidden (obsolete)
Sam Weinig
Comment 35 2017-08-15 18:26:55 PDT Comment hidden (obsolete)
Build Bot
Comment 36 2017-08-15 18:29:46 PDT Comment hidden (obsolete)
Sam Weinig
Comment 37 2017-08-15 21:30:35 PDT Comment hidden (obsolete)
Build Bot
Comment 38 2017-08-15 21:33:14 PDT Comment hidden (obsolete)
Build Bot
Comment 39 2017-08-15 23:32:34 PDT Comment hidden (obsolete)
Build Bot
Comment 40 2017-08-15 23:32:35 PDT Comment hidden (obsolete)
Build Bot
Comment 41 2017-08-16 01:34:23 PDT Comment hidden (obsolete)
Build Bot
Comment 42 2017-08-16 01:34:25 PDT Comment hidden (obsolete)
Sam Weinig
Comment 43 2017-08-16 09:32:20 PDT
Build Bot
Comment 44 2017-08-16 09:35:24 PDT Comment hidden (obsolete)
Chris Dumez
Comment 45 2017-08-16 10:28:47 PDT
Comment on attachment 318265 [details] Patch r=me, nice.
Jiewen Tan
Comment 46 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?
WebKit Commit Bot
Comment 47 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>
WebKit Commit Bot
Comment 48 2017-08-16 15:11:26 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 49 2017-08-16 15:12:06 PDT
Sam Weinig
Comment 50 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.
Note You need to log in before you can comment on or make changes to this bug.