Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(389)

Issue 23164012: WebCrypto: Remove support for multi-part operations. (Closed)

Created:
7 years, 4 months ago by eroman
Modified:
7 years, 4 months ago
Reviewers:
abarth-chromium
CC:
blink-reviews, jamesr, eae+blinkwatch, tommyw+watchlist_chromium.org, dglazkov+blink, jeez, Bryan Eyler
Visibility:
Public.

Description

WebCrypto: Remove support for multi-part operations. This matches the recent change to the standard: https://dvcs.w3.org/hg/webcrypto-api/rev/a0dce4c0a74d In particular, CryptoOperation does not exist anymore (i.e. no CryptoOperation.process(), CryptoOperation.abort(), CryptoOperation.finish(), CryptoOperation.algorithm). BUG=245025 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156338

Patch Set 1 #

Patch Set 2 : #

Total comments: 14

Patch Set 3 : Have Chromium side own the result pointer, and create a wrapper to handle deletion #

Patch Set 4 : Use refcounting for the wrapper instead #

Patch Set 5 : Sync to tot #

Unified diffs Side-by-side diffs Delta from patch set Stats (+260 lines, -1499 lines) Patch
M LayoutTests/crypto/digest.html View 1 chunk +10 lines, -110 lines 0 comments Download
M LayoutTests/crypto/digest-expected.txt View 1 chunk +5 lines, -28 lines 0 comments Download
M LayoutTests/crypto/importKey.html View 2 chunks +1 line, -13 lines 0 comments Download
M LayoutTests/crypto/importKey-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
D LayoutTests/crypto/normalize-algorithm.html View 1 chunk +0 lines, -223 lines 0 comments Download
D LayoutTests/crypto/normalize-algorithm-expected.txt View 1 chunk +0 lines, -52 lines 0 comments Download
M LayoutTests/crypto/sign-verify.html View 1 2 3 1 chunk +9 lines, -9 lines 0 comments Download
M LayoutTests/crypto/sign-verify-expected.txt View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/platform/chromium/support/WebCrypto.cpp View 1 2 3 1 chunk +15 lines, -53 lines 0 comments Download
D Source/modules/crypto/CryptoOperation.h View 1 chunk +0 lines, -148 lines 0 comments Download
D Source/modules/crypto/CryptoOperation.cpp View 1 chunk +0 lines, -249 lines 0 comments Download
D Source/modules/crypto/CryptoOperation.idl View 1 2 3 4 1 chunk +0 lines, -44 lines 0 comments Download
D Source/modules/crypto/KeyOperation.h View 1 chunk +0 lines, -88 lines 0 comments Download
D Source/modules/crypto/KeyOperation.cpp View 1 chunk +0 lines, -127 lines 0 comments Download
M Source/modules/crypto/SubtleCrypto.h View 2 chunks +5 lines, -6 lines 0 comments Download
M Source/modules/crypto/SubtleCrypto.cpp View 1 2 3 5 chunks +110 lines, -41 lines 0 comments Download
M Source/modules/crypto/SubtleCrypto.idl View 1 2 3 4 1 chunk +5 lines, -5 lines 0 comments Download
M Source/modules/modules.gypi View 1 2 3 4 2 chunks +0 lines, -5 lines 0 comments Download
M Source/testing/runner/MockWebCrypto.h View 1 2 1 chunk +7 lines, -7 lines 0 comments Download
M Source/testing/runner/MockWebCrypto.cpp View 1 2 3 chunks +49 lines, -105 lines 0 comments Download
M public/platform/WebCrypto.h View 1 2 3 1 chunk +41 lines, -182 lines 0 comments Download

Messages

Total messages: 15 (0 generated)
eroman
Adam/James: I am hoping this will address your concerns on the ownership.
7 years, 4 months ago (2013-08-16 03:21:20 UTC) #1
abarth-chromium
https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chromium/support/WebCrypto.cpp File Source/core/platform/chromium/support/WebCrypto.cpp (right): https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chromium/support/WebCrypto.cpp#newcode31 Source/core/platform/chromium/support/WebCrypto.cpp:31: #include <string.h> // for memcpy() Please alphabetize this include ...
7 years, 4 months ago (2013-08-16 04:53:06 UTC) #2
eroman
https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/SubtleCrypto.cpp File Source/modules/crypto/SubtleCrypto.cpp (right): https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/SubtleCrypto.cpp#newcode109 Source/modules/crypto/SubtleCrypto.cpp:109: delete this; On 2013/08/16 04:53:07, abarth wrote: > :( ...
7 years, 4 months ago (2013-08-16 06:45:58 UTC) #3
abarth-chromium
On 2013/08/16 06:45:58, eroman wrote: > https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/SubtleCrypto.cpp > File Source/modules/crypto/SubtleCrypto.cpp (right): > > https://codereview.chromium.org/23164012/diff/2001/Source/modules/crypto/SubtleCrypto.cpp#newcode109 > ...
7 years, 4 months ago (2013-08-16 07:22:39 UTC) #4
abarth-chromium
Another way to frame my feedback is that the ownership model in this CL is ...
7 years, 4 months ago (2013-08-16 07:43:37 UTC) #5
eroman
Thanks Adam! I'm definitely trying to match the blink model and appreciate you spending this ...
7 years, 4 months ago (2013-08-16 16:23:24 UTC) #6
eroman
https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chromium/support/WebCrypto.cpp File Source/core/platform/chromium/support/WebCrypto.cpp (right): https://codereview.chromium.org/23164012/diff/2001/Source/core/platform/chromium/support/WebCrypto.cpp#newcode31 Source/core/platform/chromium/support/WebCrypto.cpp:31: #include <string.h> // for memcpy() On 2013/08/16 04:53:07, abarth ...
7 years, 4 months ago (2013-08-16 22:59:41 UTC) #7
abarth-chromium
On 2013/08/16 22:59:41, eroman wrote: > Note that I couldn't find other IDL files which ...
7 years, 4 months ago (2013-08-19 19:11:39 UTC) #8
abarth-chromium
lgtm
7 years, 4 months ago (2013-08-19 19:13:41 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/23164012/15001
7 years, 4 months ago (2013-08-19 19:13:52 UTC) #10
commit-bot: I haz the power
Failed to apply patch for Source/modules/crypto/SubtleCrypto.idl: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 4 months ago (2013-08-19 19:14:18 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/23164012/22001
7 years, 4 months ago (2013-08-19 19:43:44 UTC) #12
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=2377
7 years, 4 months ago (2013-08-19 20:44:53 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/23164012/22001
7 years, 4 months ago (2013-08-19 21:31:09 UTC) #14
commit-bot: I haz the power
7 years, 4 months ago (2013-08-19 21:49:02 UTC) #15
Message was sent while issue was closed.
Change committed as 156338

Powered by Google App Engine
This is Rietveld 408576698