[WebCrypto] Use ScriptPromiseResolverWithContext.

[WebCrypto] Use ScriptPromiseResolverWithContext.

Use ScriptPromiseResolverWithContext instead of a pair of
ScriptPromiseResolver and NewScriptState.
This CL doen't change the behavior in many cases: The only exception
is when the resolution / rejection happens on the suspended or stopped
ExecutionContext. In such a case, ScriptPromiseResolver suspends or stops
resolution or rejection.

R=eroman@chromium.org
BUG=352552
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171940

[eroman, 2014-04-16 02:30:00]

https://codereview.chromium.org/236673003/diff/20001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (left):

https://codereview.chromium.org/236673003/diff/20001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:91: NewScriptState::Scope
scope(m_scriptState.get());
Are you sure this is correct? Although m_promiseResolver() itself may no longer
need the scope to be entered, in many cases the arguments to resolve() also
require the context to have been entered. For instance
ScriptValue::createBoolean() I expect will fail without the script state scope.

[yhirano, 2014-04-16 11:56:16]

https://codereview.chromium.org/236673003/diff/20001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (left):

https://codereview.chromium.org/236673003/diff/20001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:91: NewScriptState::Scope
scope(m_scriptState.get());
On 2014/04/16 02:30:01, eroman wrote:
> Are you sure this is correct? Although m_promiseResolver() itself may no
longer
> need the scope to be entered, in many cases the arguments to resolve() also
> require the context to have been entered. For instance
> ScriptValue::createBoolean() I expect will fail without the script state
scope.

I've moved the conversion part inside ScriptPromiseResolverWithContext.

[eroman, 2014-04-16 17:35:05]

What is the advantage of ScriptPromiseResolverWithContext for this use case? It
seems like it adds more complexity for constructing the values to be passed to
reject/resolve.

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (left):

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:60: NewScriptState::Scope
scope(m_scriptState.get());
Does the call to:

   execuctionContext()->addConsoleMessage() below require the script context to
be active?

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:101:
m_promiseResolver->resolve(Key::create(key));
If we remove canCompletePromise(), would it be safe to call things like
Key::create()? (Apologies, not quite sure how ScriptWrappable works).

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:146: void
CryptoResultImpl::contextDestroyed()
Can this be removed?

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:155: bool
CryptoResultImpl::canCompletePromise() const
Can this be removed now since handled internally by promiseresolver?

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.h (right):

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.h:47: class CryptoResultImpl FINAL :
public CryptoResult, public ContextLifecycleObserver {
Can the ContextLifecycleObserver be removed now?

[yhirano, 2014-04-17 01:51:25]

> What is the advantage of ScriptPromiseResolverWithContext for this use case?

I'm planning to switching the Promises implementation (see [1]). With v8
Promises, I need to run V8 tasks manually when  resolve / reject is called from
a non-script execution environment.
ScriptPromiseResolverWithContext will do that and it needs
ScriptPromiseResolverWithContext::resolve to be called without a V8 context in
such a case.

1: https://codereview.chromium.org/209853010/

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (left):

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:60: NewScriptState::Scope
scope(m_scriptState.get());
On 2014/04/16 17:35:05, eroman wrote:
> Does the call to:
> 
>    execuctionContext()->addConsoleMessage() below require the script context
to
> be active?

AFAIK addConsoleMessage doesn't require to be in a V8 context.

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:101:
m_promiseResolver->resolve(Key::create(key));
On 2014/04/16 17:35:05, eroman wrote:
> If we remove canCompletePromise(), would it be safe to call things like
> Key::create()? (Apologies, not quite sure how ScriptWrappable works).

The ScriptWrappable constructor and ScriptWrappable::init can work without a V8
context.

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:146: void
CryptoResultImpl::contextDestroyed()
On 2014/04/16 17:35:05, eroman wrote:
> Can this be removed?

Basically, yes. But I think it is better to do that in a separate CL because
canCompletePromise() requires a ContextLifeCycle's method.
The change will get much easier after https://codereview.chromium.org/236713003/
is landed.

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:155: bool
CryptoResultImpl::canCompletePromise() const
On 2014/04/16 17:35:05, eroman wrote:
> Can this be removed now since handled internally by promiseresolver?

Basically, yes. But you use this function to decide decide whether to call
addConsoleMessage. Therefore I think it is better to tidy at another CL.

[yhirano, 2014-04-17 02:04:05]

+eustas because I don't know the answer for the below eroman's question.

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...

Hi, eustas, should addConsoleMessage be executed in a v8 context?

[eroman, 2014-04-17 02:25:46]

LGTM for the crypto/* changes.

You may want a better reviewer for ScriptPromiseResolverWithContext.h changes --
they looked good to me, but not familiar with it.

https://codereview.chromium.org/236673003/diff/80001/Source/bindings/v8/Scrip...
File Source/bindings/v8/ScriptPromiseResolverWithContext.h (right):

https://codereview.chromium.org/236673003/diff/80001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:45: // Anything that can
be passed to one of toV8 functions defined in this
"one of toV8" --> "one of the toV8"

https://codereview.chromium.org/236673003/diff/80001/Source/bindings/v8/Scrip...
Source/bindings/v8/ScriptPromiseResolverWithContext.h:59: // Anything that can
be passed to one of toV8 functions defined in this
"one of toV8" --> "one of the toV8"

[eustas, 2014-04-17 09:16:17]

On 2014/04/17 02:04:05, yhirano wrote:
> +eustas because I don't know the answer for the below eroman's question.
> 
>
https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
> 
> Hi, eustas, should addConsoleMessage be executed in a v8 context?

As I see, when ConsoleMessage is constructed with state, it uses current
execution context to grab stack.

I'll redirect question to yurys@

[yurys, 2014-04-17 09:27:13]

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
File Source/modules/crypto/CryptoResultImpl.cpp (left):

https://codereview.chromium.org/236673003/diff/80001/Source/modules/crypto/Cr...
Source/modules/crypto/CryptoResultImpl.cpp:60: NewScriptState::Scope
scope(m_scriptState.get());
On 2014/04/17 01:51:25, yhirano wrote:
> On 2014/04/16 17:35:05, eroman wrote:
> > Does the call to:
> > 
> >    execuctionContext()->addConsoleMessage() below require the script context
> to
> > be active?
> 
> AFAIK addConsoleMessage doesn't require to be in a V8 context.

Correct, addConsoleMessage doesn't require V8 to be in a context as the messages
may well be added from native code that doesn't enter v8 at all.

[yhirano, 2014-04-17 11:48:52]

Thanks.

Waiting for https://codereview.chromium.org/238723009/

[yhirano, 2014-04-18 09:22:37]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-04-18 09:22:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/236673003/140001

[commit-bot: I haz the power, 2014-04-18 09:56:17]

Change committed as 171940