Bug 160067 - Rename SubtleCrypto to WebKitSubtleCrypto
Summary: Rename SubtleCrypto to WebKitSubtleCrypto
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks: 160880
  Show dependency treegraph
 
Reported: 2016-07-21 17:16 PDT by Jiewen Tan
Modified: 2016-08-15 17:07 PDT (History)
5 users (show)

See Also:


Attachments
Patch (568.38 KB, patch)
2016-07-21 17:45 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (569.54 KB, patch)
2016-07-21 19:09 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch (113.18 KB, patch)
2016-07-22 11:48 PDT, Jiewen Tan
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (113.21 KB, patch)
2016-07-25 10:46 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2016-07-21 17:16:58 PDT
Rename SubtleCrypto to WebKitSubtleCrypto such that we are backward compatible and meanwhile allow the new implementation to use the name SubtleCrypto.
Comment 1 Radar WebKit Bug Importer 2016-07-21 17:17:59 PDT
<rdar://problem/27483617>
Comment 2 Jiewen Tan 2016-07-21 17:45:58 PDT
Created attachment 284292 [details]
Patch
Comment 3 WebKit Commit Bot 2016-07-21 17:51:59 PDT
Attachment 284292 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:61:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
WARNING: Not running on native Windows.
Total errors found: 3 in 141 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Jiewen Tan 2016-07-21 19:09:48 PDT
Created attachment 284298 [details]
Patch
Comment 5 WebKit Commit Bot 2016-07-21 19:15:39 PDT
Attachment 284298 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:61:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
WARNING: Not running on native Windows.
Total errors found: 3 in 142 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Brent Fulgham 2016-07-21 20:24:25 PDT
Comment on attachment 284298 [details]
Patch

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

This looks great, but I disagree with moving all of the crypto things into the new "webkitCrypto" folder. Can you please only move the things actually using the "Crypto.webkitSubtle" JS API?

R- to correct that problem, but otherwise this looks great.

> Source/WebCore/ChangeLog:67
> +               crypto/webkitSubtle/wrapKey-check-usage.html

I don't think you need to move all of these to a new directory. Many of these tests invoke "Crypto.subtle" or AES/RSA stuff directly. Those should stay in Crypo since they are unchanged.

Only the things invoking "Crypto.webkitSubtle" should be in a crypto/webkitSubtle directory.

> Source/WebCore/ChangeLog:71
> +        current JSBindings use, and therefore should not introduce any change of behavoir. 

Behavoir -> behavior.

> Source/WebCore/ChangeLog:73
> +        * CMakeLists.txt:

Add comment: "Revise project files for for new file names"

> Source/WebCore/ChangeLog:95
> +        (WebCore::JSWebKitSubtleCrypto::unwrapKey):

Delete lines 81-95. We don't need to see all of the renamed things here.

> Source/WebCore/crypto/WebKitSubtleCrypto.h:27
> +#define WebKitSubtleCrypto_h

#pragma once

> Source/WebCore/page/Crypto.h:67
> +    RefPtr<WebKitSubtleCrypto> m_webkitSubtle;

You don't really need this rename, but I assume you will be adding a "RefPtr<SubtleCrypto> m_subtle;" in a future patch.
Comment 7 Jiewen Tan 2016-07-22 11:33:03 PDT
Comment on attachment 284298 [details]
Patch

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

Thanks Brent for reviewing my patch.

>> Source/WebCore/ChangeLog:67
>> +               crypto/webkitSubtle/wrapKey-check-usage.html
> 
> I don't think you need to move all of these to a new directory. Many of these tests invoke "Crypto.subtle" or AES/RSA stuff directly. Those should stay in Crypo since they are unchanged.
> 
> Only the things invoking "Crypto.webkitSubtle" should be in a crypto/webkitSubtle directory.

Hmmm, I found out that codes in resources/common.js:105:
      if (!crypto.subtle)
           crypto.subtle = crypto.webkitSubtle;
enforce all the tests running against crypto.webkitSubtle. That's why even though we haven't implement the interface crypto.subtle, tests can still use that interface.

Despite this little finding, I agree with you that we should keep those tests in the original folder.

>> Source/WebCore/ChangeLog:71
>> +        current JSBindings use, and therefore should not introduce any change of behavoir. 
> 
> Behavoir -> behavior.

Fixed.

>> Source/WebCore/ChangeLog:73
>> +        * CMakeLists.txt:
> 
> Add comment: "Revise project files for for new file names"

Added.

>> Source/WebCore/ChangeLog:95
>> +        (WebCore::JSWebKitSubtleCrypto::unwrapKey):
> 
> Delete lines 81-95. We don't need to see all of the renamed things here.

Deleted.

>> Source/WebCore/crypto/WebKitSubtleCrypto.h:27
>> +#define WebKitSubtleCrypto_h
> 
> #pragma once

Added.

>> Source/WebCore/page/Crypto.h:67
>> +    RefPtr<WebKitSubtleCrypto> m_webkitSubtle;
> 
> You don't really need this rename, but I assume you will be adding a "RefPtr<SubtleCrypto> m_subtle;" in a future patch.

Yes, I will add it in the future.
Comment 8 Jiewen Tan 2016-07-22 11:48:45 PDT
Created attachment 284353 [details]
Patch
Comment 9 WebKit Commit Bot 2016-07-22 11:50:45 PDT
Attachment 284353 [details] did not pass style-queue:


WARNING: Not running on native Windows.
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:61:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Brent Fulgham 2016-07-25 10:20:45 PDT
Comment on attachment 284353 [details]
Patch

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

r=me.

> LayoutTests/crypto/webkitSubtle/gc-2-expected.txt:1
> +Test that window.crypto.subtle wrapper preserves custom properties.

Please change this to say "Test that window.crypto.webkitSubtle wrapper preserves custom properties"

> LayoutTests/crypto/webkitSubtle/gc-3-expected.txt:1
> +Test that window.crypto wrapper protects all dependencies, so it can always be used to create crypto.subtle.

Please change this to say "Test that window.crypto.webkitSubtle wrapper protects all ..."

> LayoutTests/crypto/webkitSubtle/gc-expected.txt:1
> +Test that window.crypto.subtle wrapper preserves custom properties.

Please change this to say "Test that window.crypto.webkitSubtle wrapper preserves custom properties"
Comment 11 Jiewen Tan 2016-07-25 10:46:33 PDT
Created attachment 284500 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2016-07-25 10:49:21 PDT
Attachment 284500 [details] did not pass style-queue:


WARNING: Not running on native Windows.
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:55:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:58:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/bindings/js/JSWebKitSubtleCryptoCustom.cpp:61:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Commit Bot 2016-07-25 13:36:22 PDT
Comment on attachment 284500 [details]
Patch for landing

Clearing flags on attachment: 284500

Committed r203696: <http://trac.webkit.org/changeset/203696>