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

Issue 133253002: [webcrypto] Fix TODO regarding asynchronous completion of crypto operations. (Closed)

Created:
6 years, 11 months ago by eroman
Modified:
6 years, 11 months ago
Reviewers:
abarth-chromium
CC:
blink-reviews, jamesr, dglazkov+blink
Visibility:
Public.

Description

[webcrypto] Fix TODO regarding asynchronous completion of crypto operations. Also expand the documentation of blink API for WebCrypto to describe thread safety and guarantees on inputs. BUG=245025 R=abarth@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165506

Patch Set 1 #

Total comments: 5

Patch Set 2 : Dont use ScriptState - Instead use ContextLifecycleObserver and save the DomWrapperWorld #

Total comments: 2

Patch Set 3 : Use DOMRequestState for handle/context scope #

Total comments: 1

Patch Set 4 : Rebase #

Patch Set 5 : Remove unused forward declaration of ScriptState #

Unified diffs Side-by-side diffs Delta from patch set Stats (+172 lines, -32 lines) Patch
M Source/core/platform/CryptoResult.h View 1 chunk +2 lines, -2 lines 0 comments Download
M Source/modules/crypto/CryptoResultImpl.h View 1 2 3 4 3 chunks +20 lines, -2 lines 0 comments Download
M Source/modules/crypto/CryptoResultImpl.cpp View 1 2 2 chunks +80 lines, -10 lines 0 comments Download
M Source/modules/crypto/SubtleCrypto.cpp View 1 chunk +0 lines, -3 lines 0 comments Download
M public/platform/WebCrypto.h View 2 chunks +70 lines, -15 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
eroman
Testing: I did some brief manual tests. Layout tests will exercise the asynchronous completion codepaths ...
6 years, 11 months ago (2014-01-10 05:21:12 UTC) #1
abarth-chromium
https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h#newcode67 Source/modules/crypto/CryptoResultImpl.h:67: // This ScriptState should outlive m_promiseResolver, as it is ...
6 years, 11 months ago (2014-01-10 15:25:03 UTC) #2
eroman
https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h#newcode67 Source/modules/crypto/CryptoResultImpl.h:67: // This ScriptState should outlive m_promiseResolver, as it is ...
6 years, 11 months ago (2014-01-10 19:10:25 UTC) #3
eroman
Gentle ping. Just in case it fell off your stack, since you are generally super ...
6 years, 11 months ago (2014-01-14 01:41:57 UTC) #4
abarth-chromium
https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h#newcode67 Source/modules/crypto/CryptoResultImpl.h:67: // This ScriptState should outlive m_promiseResolver, as it is ...
6 years, 11 months ago (2014-01-14 04:01:05 UTC) #5
eroman
https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/133253002/diff/1/Source/modules/crypto/CryptoResultImpl.h#newcode67 Source/modules/crypto/CryptoResultImpl.h:67: // This ScriptState should outlive m_promiseResolver, as it is ...
6 years, 11 months ago (2014-01-14 20:40:09 UTC) #6
abarth-chromium
On 2014/01/14 20:40:09, eroman wrote: > Would it be appropriate to make CryptoResultImpl inherit from ...
6 years, 11 months ago (2014-01-15 00:04:57 UTC) #7
eroman
PTAL -> switched to using ContextLifecycleObserver instead of ScriptState. I found myself having to duplicate ...
6 years, 11 months ago (2014-01-15 21:18:41 UTC) #8
abarth-chromium
https://codereview.chromium.org/133253002/diff/150001/Source/modules/crypto/CryptoResultImpl.cpp File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/133253002/diff/150001/Source/modules/crypto/CryptoResultImpl.cpp#newcode70 Source/modules/crypto/CryptoResultImpl.cpp:70: v8::Context::Scope scope(v8Context); Maybe DOMRequestState would be helpful here? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/v8/DOMRequestState.h ...
6 years, 11 months ago (2014-01-16 04:41:07 UTC) #9
eroman
https://codereview.chromium.org/133253002/diff/150001/Source/modules/crypto/CryptoResultImpl.cpp File Source/modules/crypto/CryptoResultImpl.cpp (right): https://codereview.chromium.org/133253002/diff/150001/Source/modules/crypto/CryptoResultImpl.cpp#newcode70 Source/modules/crypto/CryptoResultImpl.cpp:70: v8::Context::Scope scope(v8Context); On 2014/01/16 04:41:08, abarth wrote: > Maybe ...
6 years, 11 months ago (2014-01-16 20:24:15 UTC) #10
abarth-chromium
LGTM. This looks great. https://codereview.chromium.org/133253002/diff/270001/Source/modules/crypto/CryptoResultImpl.h File Source/modules/crypto/CryptoResultImpl.h (right): https://codereview.chromium.org/133253002/diff/270001/Source/modules/crypto/CryptoResultImpl.h#newcode46 Source/modules/crypto/CryptoResultImpl.h:46: class ScriptState; This looks unused ...
6 years, 11 months ago (2014-01-18 09:03:48 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/133253002/270001
6 years, 11 months ago (2014-01-18 09:03:59 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/133253002/270001
6 years, 11 months ago (2014-01-21 20:09:43 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/133253002/400001
6 years, 11 months ago (2014-01-21 20:10:21 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/133253002/460001
6 years, 11 months ago (2014-01-21 20:11:20 UTC) #15
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=18362
6 years, 11 months ago (2014-01-21 21:00:09 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/133253002/460001
6 years, 11 months ago (2014-01-21 21:04:35 UTC) #17
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=18388
6 years, 11 months ago (2014-01-21 21:47:04 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/133253002/460001
6 years, 11 months ago (2014-01-21 21:56:53 UTC) #19
commit-bot: I haz the power
Retried try job too often on linux_blink_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blink_rel&number=18408
6 years, 11 months ago (2014-01-21 23:04:23 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/133253002/460001
6 years, 11 months ago (2014-01-22 01:11:27 UTC) #21
eroman
6 years, 11 months ago (2014-01-22 03:15:59 UTC) #22
Message was sent while issue was closed.
Committed patchset #5 manually as r165506 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698