Have ScriptPromiseResolver on the Oilpan heap always.

Have ScriptPromiseResolver on the Oilpan heap always.

R=yhirano,haraken,jochen
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199953

[sof, 2015-07-14 21:10:13]

Patchset #2 (id:20001) has been deleted

[sof, 2015-07-14 21:10:26]

Patchset #2 (id:40001) has been deleted

[sof, 2015-07-14 21:10:39]

Patchset #2 (id:60001) has been deleted

[sof, 2015-07-14 21:10:50]

Patchset #2 (id:80001) has been deleted

[sof, 2015-07-16 06:42:37]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-07-16 06:42:38]

This is ready, but blocked on https://codereview.chromium.org/1228373006/

[haraken, 2015-07-16 06:55:47]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org, yhirano@chromium.org

[haraken, 2015-07-16 06:55:48]

+yhirano

(Let me take a look later.)

[yhirano, 2015-07-16 07:09:30]

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (left):

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:124: ref();
This ref() was called to keep the resolver alive while suspending even if
unreacheable. How do you guarantee that without this call?

[sof, 2015-07-16 07:13:55]

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (left):

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:124: ref();
On 2015/07/16 07:09:30, yhirano wrote:
> This ref() was called to keep the resolver alive while suspending even if
> unreacheable. How do you guarantee that without this call?

The object is on the stack and thereby reachable.

[yhirano, 2015-07-16 07:19:38]

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (left):

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:124: ref();
On 2015/07/16 07:13:54, sof wrote:
> On 2015/07/16 07:09:30, yhirano wrote:
> > This ref() was called to keep the resolver alive while suspending even if
> > unreacheable. How do you guarantee that without this call?
> 
> The object is on the stack and thereby reachable.

I think about the following case, for example.

// Assume the document is suspended.
// |resolver| is the only reference to the resolver.
resolver->resolve(...);
// |resolveOrRejectImmediately| is not called. it will be called when
// resumed.

resolver = nullptr;

// Here the resolver is unreacheable but should be alive until
// resume() or stop() is called.

[sof, 2015-07-16 07:33:23]

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (left):

https://codereview.chromium.org/1233173002/diff/140001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:124: ref();
On 2015/07/16 07:19:38, yhirano wrote:
> On 2015/07/16 07:13:54, sof wrote:
> > On 2015/07/16 07:09:30, yhirano wrote:
> > > This ref() was called to keep the resolver alive while suspending even if
> > > unreacheable. How do you guarantee that without this call?
> > 
> > The object is on the stack and thereby reachable.
> 
> I think about the following case, for example.
> 
> // Assume the document is suspended.
> // |resolver| is the only reference to the resolver.
> resolver->resolve(...);
> // |resolveOrRejectImmediately| is not called. it will be called when
> // resumed.
> 
> resolver = nullptr;
> 
> // Here the resolver is unreacheable but should be alive until
> // resume() or stop() is called.
> 

Thanks. Yes, we need retain a strong reference for that case now also. Updated
to do so.

[yhirano, 2015-07-16 11:14:15]

https://codereview.chromium.org/1233173002/diff/160001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolverTest.cpp (right):

https://codereview.chromium.org/1233173002/diff/160001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:207: namespace {
You're already in an unnamed namespace, so you don't need this one.

https://codereview.chromium.org/1233173002/diff/160001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:249:
EXPECT_TRUE(ScriptPromiseResolverKeepAlive::isAlive());
ASSERT_TRUE? |resolver| will be a dangling pointer when assertion fails. Ditto
for below cases.

https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest {
+final

[sof, 2015-07-16 11:18:44]

https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest {
On 2015/07/16 11:14:15, yhirano wrote:
> +final

Do we use that on public/ exported types?

[yhirano, 2015-07-16 11:30:58]

https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
File public/web/WebMIDIPermissionRequest.h (right):

https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest {
On 2015/07/16 11:18:44, sof wrote:
> On 2015/07/16 11:14:15, yhirano wrote:
> > +final
> 
> Do we use that on public/ exported types?

Is there a rule forbidding final? If you're not comfortable with final here,
it's OK to leave it be.

[sof, 2015-07-16 11:33:45]

On 2015/07/16 11:30:58, yhirano wrote:
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> File public/web/WebMIDIPermissionRequest.h (right):
> 
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest {
> On 2015/07/16 11:18:44, sof wrote:
> > On 2015/07/16 11:14:15, yhirano wrote:
> > > +final
> > 
> > Do we use that on public/ exported types?
> 
> Is there a rule forbidding final? If you're not comfortable with final here,
> it's OK to leave it be.

I don't think it makes sense to do piecemeal annotations here; why would we want
to prevent extension of this particular class?

[yhirano, 2015-07-16 11:35:58]

On 2015/07/16 11:33:45, sof wrote:
> On 2015/07/16 11:30:58, yhirano wrote:
> >
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> > File public/web/WebMIDIPermissionRequest.h (right):
> > 
> >
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> > public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest {
> > On 2015/07/16 11:18:44, sof wrote:
> > > On 2015/07/16 11:14:15, yhirano wrote:
> > > > +final
> > > 
> > > Do we use that on public/ exported types?
> > 
> > Is there a rule forbidding final? If you're not comfortable with final here,
> > it's OK to leave it be.
> 
> I don't think it makes sense to do piecemeal annotations here; why would we
want
> to prevent extension of this particular class?

If we are open to subclass it, shouldn't we make the destructor virtual?

[sof, 2015-07-16 11:42:25]

On 2015/07/16 11:35:58, yhirano wrote:
> On 2015/07/16 11:33:45, sof wrote:
> > On 2015/07/16 11:30:58, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> > > File public/web/WebMIDIPermissionRequest.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> > > public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest {
> > > On 2015/07/16 11:18:44, sof wrote:
> > > > On 2015/07/16 11:14:15, yhirano wrote:
> > > > > +final
> > > > 
> > > > Do we use that on public/ exported types?
> > > 
> > > Is there a rule forbidding final? If you're not comfortable with final
here,
> > > it's OK to leave it be.
> > 
> > I don't think it makes sense to do piecemeal annotations here; why would we
> want
> > to prevent extension of this particular class?
> 
> If we are open to subclass it, shouldn't we make the destructor virtual?

You could argue either way, a blanket rule for all Blink exported classes seems
hard to come up with.

[yhirano, 2015-07-16 11:54:24]

On 2015/07/16 11:42:25, sof wrote:
> On 2015/07/16 11:35:58, yhirano wrote:
> > On 2015/07/16 11:33:45, sof wrote:
> > > On 2015/07/16 11:30:58, yhirano wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> > > > File public/web/WebMIDIPermissionRequest.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1233173002/diff/160001/public/web/WebMIDIPerm...
> > > > public/web/WebMIDIPermissionRequest.h:47: class WebMIDIPermissionRequest
{
> > > > On 2015/07/16 11:18:44, sof wrote:
> > > > > On 2015/07/16 11:14:15, yhirano wrote:
> > > > > > +final
> > > > > 
> > > > > Do we use that on public/ exported types?
> > > > 
> > > > Is there a rule forbidding final? If you're not comfortable with final
> here,
> > > > it's OK to leave it be.
> > > 
> > > I don't think it makes sense to do piecemeal annotations here; why would
we
> > want
> > > to prevent extension of this particular class?
> > 
> > If we are open to subclass it, shouldn't we make the destructor virtual?
> 
> You could argue either way, a blanket rule for all Blink exported classes
seems
> hard to come up with.

OK, I retract the comment.

[sof, 2015-07-16 11:57:43]

https://codereview.chromium.org/1233173002/diff/160001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolverTest.cpp (right):

https://codereview.chromium.org/1233173002/diff/160001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:207: namespace {
On 2015/07/16 11:14:14, yhirano wrote:
> You're already in an unnamed namespace, so you don't need this one.

Dropped.

https://codereview.chromium.org/1233173002/diff/160001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:249:
EXPECT_TRUE(ScriptPromiseResolverKeepAlive::isAlive());
On 2015/07/16 11:14:14, yhirano wrote:
> ASSERT_TRUE? |resolver| will be a dangling pointer when assertion fails. Ditto
> for below cases.

Good idea, done (here & elsewhere.)

[yhirano, 2015-07-16 12:09:00]

lgtm

[haraken, 2015-07-17 01:40:36]

Mostly looks good.

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (left):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.cpp:90: deref();

So my proposal is to keep this deref() (which is realized by
RefCountedGarbageCollected)...

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.cpp:97: deref();

but remove this deref().

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is needed.

Can we use RefCountedGarbageCollected instead of manually creating a keep alive
perssitent? RefCountedGarbageCollected is the way to create the keep alive
persistent in a safe manner.

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();

Not related to this CL, we should have:

  if (!scriptState->contextIsValid())
    return false;

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachest...
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachest...
Source/modules/cachestorage/Cache.cpp:33:
CacheMatchCallbacks(ScriptPromiseResolver* resolver)

Add explicit. Same comment for other constructors in this file.

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachest...
Source/modules/cachestorage/Cache.cpp:39: m_resolver.clear();

It would be worth having:

  ASSERT(!m_resolver);

in the destructor. Same comment for other m_resolvers in this file.

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
Source/modules/credentialmanager/CredentialsContainer.cpp:47:
~NotificationCallbacks() override { }

Add ASSERT(!m_resolver).

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
Source/modules/credentialmanager/CredentialsContainer.cpp:51:
m_resolver->resolve();

Shall we clear m_resolver?

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
Source/modules/credentialmanager/CredentialsContainer.cpp:72:
m_resolver->resolve();

Ditto. Same comment for other places.

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:121: if
(scriptState->executionContext()->activeDOMObjectsAreStopped()) {

Not related to this CL, why do we need this special logic only in
CryptoResultImpl? If we need this here, I guess we need it for all Resolver
owners.

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/mediast...
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/mediast...
Source/modules/mediastream/MediaDevices.cpp:41: 

I'm just curious, but is it valid that we reach the destructor without
m_resolver getting resolved? I guess it's not guaranteed that handleEvent is
always called.

[yhirano, 2015-07-17 02:11:32]

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
Source/modules/credentialmanager/CredentialsContainer.cpp:51:
m_resolver->resolve();
On 2015/07/17 01:40:36, haraken wrote:
> 
> Shall we clear m_resolver?

I'm not a great fan of the pattern. resolve() and reject() clear internal
resources, why is the reference cleaning needed?

[haraken, 2015-07-17 02:18:36]

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
Source/modules/credentialmanager/CredentialsContainer.cpp:51:
m_resolver->resolve();
On 2015/07/17 02:11:31, yhirano wrote:
> On 2015/07/17 01:40:36, haraken wrote:
> > 
> > Shall we clear m_resolver?
> 
> I'm not a great fan of the pattern. resolve() and reject() clear internal
> resources, why is the reference cleaning needed?

I'm just curious about the programming rule. Some clears m_resolver in onSuccess
& onError, others don't.

So your proposal is:

- Don't call m_resolver.clear() in onSuccess & onError.
- Thus the owner of the m_resolver don't need to check ASSERT(!m_resolver).
- m_resolver's destructor has an asssert to verify that resolve or reject has
been called.

?

[yhirano, 2015-07-17 03:18:40]

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
File Source/modules/credentialmanager/CredentialsContainer.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
Source/modules/credentialmanager/CredentialsContainer.cpp:51:
m_resolver->resolve();
On 2015/07/17 02:18:35, haraken wrote:
> On 2015/07/17 02:11:31, yhirano wrote:
> > On 2015/07/17 01:40:36, haraken wrote:
> > > 
> > > Shall we clear m_resolver?
> > 
> > I'm not a great fan of the pattern. resolve() and reject() clear internal
> > resources, why is the reference cleaning needed?
> 
> I'm just curious about the programming rule. Some clears m_resolver in
onSuccess
> & onError, others don't.
> 
> So your proposal is:
> 
> - Don't call m_resolver.clear() in onSuccess & onError.
> - Thus the owner of the m_resolver don't need to check ASSERT(!m_resolver).
> - m_resolver's destructor has an asssert to verify that resolve or reject has
> been called.
> 
> ?

As a user I generally don't like manual clearing which may be missing easily.
Additionally in this case the manual clearing does not have a merit in terms of
ScriptPromiseResolver, as
 - it clears the internal resource when resolved / rejected and hence retaining
a resolved / rejected resolver does not lead to a leak and
 - resolve / reject will be ignored when it is already resolved or rejected.

If a user wants to clear the resolver reference from a reason that is specific
to the user, it is OK, but I think it is not good to mandate the manual
reference clearing to all users.

[haraken, 2015-07-17 03:26:47]

On 2015/07/17 03:18:40, yhirano wrote:
>
https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
> File Source/modules/credentialmanager/CredentialsContainer.cpp (right):
> 
>
https://codereview.chromium.org/1233173002/diff/180001/Source/modules/credent...
> Source/modules/credentialmanager/CredentialsContainer.cpp:51:
> m_resolver->resolve();
> On 2015/07/17 02:18:35, haraken wrote:
> > On 2015/07/17 02:11:31, yhirano wrote:
> > > On 2015/07/17 01:40:36, haraken wrote:
> > > > 
> > > > Shall we clear m_resolver?
> > > 
> > > I'm not a great fan of the pattern. resolve() and reject() clear internal
> > > resources, why is the reference cleaning needed?
> > 
> > I'm just curious about the programming rule. Some clears m_resolver in
> onSuccess
> > & onError, others don't.
> > 
> > So your proposal is:
> > 
> > - Don't call m_resolver.clear() in onSuccess & onError.
> > - Thus the owner of the m_resolver don't need to check ASSERT(!m_resolver).
> > - m_resolver's destructor has an asssert to verify that resolve or reject
has
> > been called.
> > 
> > ?
> 
> As a user I generally don't like manual clearing which may be missing easily.
> Additionally in this case the manual clearing does not have a merit in terms
of
> ScriptPromiseResolver, as
>  - it clears the internal resource when resolved / rejected and hence
retaining
> a resolved / rejected resolver does not lead to a leak and
>  - resolve / reject will be ignored when it is already resolved or rejected.
> 
> If a user wants to clear the resolver reference from a reason that is specific
> to the user, it is OK, but I think it is not good to mandate the manual
> reference clearing to all users.

Sounds reasonable to me. Maybe we want to remove unnecessary m_resolver.clear()
from the code base.

[sof, 2015-07-17 05:34:30]

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is needed.
On 2015/07/17 01:40:35, haraken wrote:
> 
> Can we use RefCountedGarbageCollected instead of manually creating a keep
alive
> perssitent? RefCountedGarbageCollected is the way to create the keep alive
> persistent in a safe manner.
> 

I don't agree with that suggestion. Two reasons:

 - there's no counting required here.
 - it exposes reference counting as part of the public interface of these
objects; it's an internal implementation detail.

You'll find instances of local Persistent<> self keep alives elsewhere.

[haraken, 2015-07-17 06:52:52]

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is needed.
On 2015/07/17 05:34:30, sof wrote:
> On 2015/07/17 01:40:35, haraken wrote:
> > 
> > Can we use RefCountedGarbageCollected instead of manually creating a keep
> alive
> > perssitent? RefCountedGarbageCollected is the way to create the keep alive
> > persistent in a safe manner.
> > 
> 
> I don't agree with that suggestion. Two reasons:
> 
>  - there's no counting required here.
>  - it exposes reference counting as part of the public interface of these
> objects; it's an internal implementation detail.
> 
> You'll find instances of local Persistent<> self keep alives elsewhere.

That is something we need to fix. I'm ok with either plan (i.e., using a self
keep alive handle or using RefCountedGarbageCollected) but we should avoid
mixing the two syntax in the code base.

a) Stop using RefCountedGarbageCollected for the self-keep-alive purpose. Use an
explicit persistent handle.

b) Use RefCountedGarbageCollected for the self-keep-alive purpose. Rename
ref/deref to a better name.

If your plan is to apply a) for all the code base, I'm fine with it :)

[sof, 2015-07-17 07:02:04]

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is needed.
On 2015/07/17 06:52:52, haraken wrote:
> On 2015/07/17 05:34:30, sof wrote:
> > On 2015/07/17 01:40:35, haraken wrote:
> > > 
> > > Can we use RefCountedGarbageCollected instead of manually creating a keep
> > alive
> > > perssitent? RefCountedGarbageCollected is the way to create the keep alive
> > > persistent in a safe manner.
> > > 
> > 
> > I don't agree with that suggestion. Two reasons:
> > 
> >  - there's no counting required here.
> >  - it exposes reference counting as part of the public interface of these
> > objects; it's an internal implementation detail.
> > 
> > You'll find instances of local Persistent<> self keep alives elsewhere.
> 
> That is something we need to fix. I'm ok with either plan (i.e., using a self
> keep alive handle or using RefCountedGarbageCollected) but we should avoid
> mixing the two syntax in the code base.
> 
> a) Stop using RefCountedGarbageCollected for the self-keep-alive purpose. Use
an
> explicit persistent handle.
> 
> b) Use RefCountedGarbageCollected for the self-keep-alive purpose. Rename
> ref/deref to a better name.
> 
> If your plan is to apply a) for all the code base, I'm fine with it :)

What/where are they? (I don't know of any, off-hand.) We already have self-keep
alives using Persistent<> in places.

[haraken, 2015-07-17 07:15:56]

On 2015/07/17 07:02:04, sof wrote:
>
https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
> File Source/bindings/core/v8/ScriptPromiseResolver.h (right):
> 
>
https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
> Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is
needed.
> On 2015/07/17 06:52:52, haraken wrote:
> > On 2015/07/17 05:34:30, sof wrote:
> > > On 2015/07/17 01:40:35, haraken wrote:
> > > > 
> > > > Can we use RefCountedGarbageCollected instead of manually creating a
keep
> > > alive
> > > > perssitent? RefCountedGarbageCollected is the way to create the keep
alive
> > > > persistent in a safe manner.
> > > > 
> > > 
> > > I don't agree with that suggestion. Two reasons:
> > > 
> > >  - there's no counting required here.
> > >  - it exposes reference counting as part of the public interface of these
> > > objects; it's an internal implementation detail.
> > > 
> > > You'll find instances of local Persistent<> self keep alives elsewhere.
> > 
> > That is something we need to fix. I'm ok with either plan (i.e., using a
self
> > keep alive handle or using RefCountedGarbageCollected) but we should avoid
> > mixing the two syntax in the code base.
> > 
> > a) Stop using RefCountedGarbageCollected for the self-keep-alive purpose.
Use
> an
> > explicit persistent handle.
> > 
> > b) Use RefCountedGarbageCollected for the self-keep-alive purpose. Rename
> > ref/deref to a better name.
> > 
> > If your plan is to apply a) for all the code base, I'm fine with it :)
> 
> What/where are they? (I don't know of any, off-hand.) We already have
self-keep
> alives using Persistent<> in places.

SuspendableScriptExecutor is one example (since I was thinking that b) is a
plan). I'm fine with a).

[sof, 2015-07-17 08:21:07]

On 2015/07/17 07:15:56, haraken wrote:
> On 2015/07/17 07:02:04, sof wrote:
> >
>
https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
> > File Source/bindings/core/v8/ScriptPromiseResolver.h (right):
> > 
> >
>
https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
> > Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is
> needed.
> > On 2015/07/17 06:52:52, haraken wrote:
> > > On 2015/07/17 05:34:30, sof wrote:
> > > > On 2015/07/17 01:40:35, haraken wrote:
> > > > > 
> > > > > Can we use RefCountedGarbageCollected instead of manually creating a
> keep
> > > > alive
> > > > > perssitent? RefCountedGarbageCollected is the way to create the keep
> alive
> > > > > persistent in a safe manner.
> > > > > 
> > > > 
> > > > I don't agree with that suggestion. Two reasons:
> > > > 
> > > >  - there's no counting required here.
> > > >  - it exposes reference counting as part of the public interface of
these
> > > > objects; it's an internal implementation detail.
> > > > 
> > > > You'll find instances of local Persistent<> self keep alives elsewhere.
> > > 
> > > That is something we need to fix. I'm ok with either plan (i.e., using a
> self
> > > keep alive handle or using RefCountedGarbageCollected) but we should avoid
> > > mixing the two syntax in the code base.
> > > 
> > > a) Stop using RefCountedGarbageCollected for the self-keep-alive purpose.
> Use
> > an
> > > explicit persistent handle.
> > > 
> > > b) Use RefCountedGarbageCollected for the self-keep-alive purpose. Rename
> > > ref/deref to a better name.
> > > 
> > > If your plan is to apply a) for all the code base, I'm fine with it :)
> > 
> > What/where are they? (I don't know of any, off-hand.) We already have
> self-keep
> > alives using Persistent<> in places.
> 
> SuspendableScriptExecutor is one example (since I was thinking that b) is a
> plan). I'm fine with a).

i see, thanks. Added a light abstraction SelfKeepAlive<T> to handle the details.
(Will adopt its use more fully in a follow-up, if (it&) that's acceptable.)

[haraken, 2015-07-17 08:53:24]

On 2015/07/17 08:21:07, sof wrote:
> On 2015/07/17 07:15:56, haraken wrote:
> > On 2015/07/17 07:02:04, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
> > > File Source/bindings/core/v8/ScriptPromiseResolver.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1233173002/diff/180001/Source/bindings/core/v...
> > > Source/bindings/core/v8/ScriptPromiseResolver.h:147: // Persistent<> is
> > needed.
> > > On 2015/07/17 06:52:52, haraken wrote:
> > > > On 2015/07/17 05:34:30, sof wrote:
> > > > > On 2015/07/17 01:40:35, haraken wrote:
> > > > > > 
> > > > > > Can we use RefCountedGarbageCollected instead of manually creating a
> > keep
> > > > > alive
> > > > > > perssitent? RefCountedGarbageCollected is the way to create the keep
> > alive
> > > > > > persistent in a safe manner.
> > > > > > 
> > > > > 
> > > > > I don't agree with that suggestion. Two reasons:
> > > > > 
> > > > >  - there's no counting required here.
> > > > >  - it exposes reference counting as part of the public interface of
> these
> > > > > objects; it's an internal implementation detail.
> > > > > 
> > > > > You'll find instances of local Persistent<> self keep alives
elsewhere.
> > > > 
> > > > That is something we need to fix. I'm ok with either plan (i.e., using a
> > self
> > > > keep alive handle or using RefCountedGarbageCollected) but we should
avoid
> > > > mixing the two syntax in the code base.
> > > > 
> > > > a) Stop using RefCountedGarbageCollected for the self-keep-alive
purpose.
> > Use
> > > an
> > > > explicit persistent handle.
> > > > 
> > > > b) Use RefCountedGarbageCollected for the self-keep-alive purpose.
Rename
> > > > ref/deref to a better name.
> > > > 
> > > > If your plan is to apply a) for all the code base, I'm fine with it :)
> > > 
> > > What/where are they? (I don't know of any, off-hand.) We already have
> > self-keep
> > > alives using Persistent<> in places.
> > 
> > SuspendableScriptExecutor is one example (since I was thinking that b) is a
> > plan). I'm fine with a).
> 
> i see, thanks. Added a light abstraction SelfKeepAlive<T> to handle the
details.
> (Will adopt its use more fully in a follow-up, if (it&) that's acceptable.)

Looks good! (Let me take a look at the rest of the CL later.)

[sof, 2015-07-17 09:43:26]

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();
On 2015/07/17 01:40:35, haraken wrote:
> 
> Not related to this CL, we should have:
> 
>   if (!scriptState->contextIsValid())
>     return false;

Done, here & in resolveAllPendingReadsAsDone()

yhirano@: ptal ?

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachest...
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/cachest...
Source/modules/cachestorage/Cache.cpp:33:
CacheMatchCallbacks(ScriptPromiseResolver* resolver)
On 2015/07/17 01:40:35, haraken wrote:
> 
> Add explicit. Same comment for other constructors in this file.
> 

Done.

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:121: if
(scriptState->executionContext()->activeDOMObjectsAreStopped()) {
On 2015/07/17 01:40:36, haraken wrote:
> 
> Not related to this CL, why do we need this special logic only in
> CryptoResultImpl? If we need this here, I guess we need it for all Resolver
> owners.

It's due to
https://codereview.chromium.org/780793002/diff/60001/Source/modules/crypto/Cr...

As CryptoResultImpl now keeps a Member<>, we no longer this special case. (I
thought it was there as a leak-preventing measure, but it was needed for weak
pointer safety.)

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/mediast...
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/modules/mediast...
Source/modules/mediastream/MediaDevices.cpp:41: 
On 2015/07/17 01:40:36, haraken wrote:
> 
> I'm just curious, but is it valid that we reach the destructor without
> m_resolver getting resolved? I guess it's not guaranteed that handleEvent is
> always called.

But then handleEvent() would be called for the paired PromiseErrorCallback
instead?

[yhirano, 2015-07-17 13:53:41]

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();
On 2015/07/17 09:43:26, sof wrote:
> On 2015/07/17 01:40:35, haraken wrote:
> > 
> > Not related to this CL, we should have:
> > 
> >   if (!scriptState->contextIsValid())
> >     return false;
> 
> Done, here & in resolveAllPendingReadsAsDone()
> 
> yhirano@: ptal ?

haraken@: Is it guaranteed that the document is stopped when
scriptState->contextIsValid() returns false? Otherwise an assertion in the
ScriptPromiseResolver fails and we should modify there as well.

[haraken, 2015-07-18 07:10:27]

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
File Source/core/streams/ReadableStreamImpl.h (right):

https://codereview.chromium.org/1233173002/diff/180001/Source/core/streams/Re...
Source/core/streams/ReadableStreamImpl.h:178: ScriptState* scriptState =
resolver->scriptState();
On 2015/07/17 13:53:41, yhirano wrote:
> On 2015/07/17 09:43:26, sof wrote:
> > On 2015/07/17 01:40:35, haraken wrote:
> > > 
> > > Not related to this CL, we should have:
> > > 
> > >   if (!scriptState->contextIsValid())
> > >     return false;
> > 
> > Done, here & in resolveAllPendingReadsAsDone()
> > 
> > yhirano@: ptal ?
> 
> haraken@: Is it guaranteed that the document is stopped when
> scriptState->contextIsValid() returns false? Otherwise an assertion in the
> ScriptPromiseResolver fails and we should modify there as well.

It is guaranteed for a document that is associated with a frame. It is not
guaranteed for some documents that don't have a frame, but that's an edge case
(e.g., XSLT documents).

[haraken, 2015-07-18 08:01:24]

LGTM

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)

Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolverTest.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:227:
ScriptPromiseResolverKeepAlive(ScriptState* scriptState)

Add explicit.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachest...
File Source/modules/cachestorage/Cache.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachest...
Source/modules/cachestorage/Cache.cpp:70: m_resolver.clear();

Nit: So let's remove these .clear() in follow-up CLs.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachest...
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachest...
Source/modules/cachestorage/CacheStorage.cpp:100:
MatchCallbacks(ScriptPromiseResolver* resolver)

Add explicit.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:120:
ASSERT(scriptState->contextIsValid());

Nit: It would be nice to move this assert to Resolver::create.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediast...
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediast...
Source/modules/mediastream/MediaDevices.cpp:37:
PromiseSuccessCallback(ScriptPromiseResolver* resolver)

Add explicit.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediast...
Source/modules/mediastream/MediaDevices.cpp:63:
PromiseErrorCallback(ScriptPromiseResolver* resolver)

Add explicit.

[sof, 2015-07-18 20:46:08]

Thanks for the (weekend) review.

Until the Blink thread that initiates a crypto operation also take care of
releasing its CryptoResultImpl (heap) result (
https://codereview.chromium.org/1228373006/ ), this CL cannot go ahead. Will
park it until then.

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)
On 2015/07/18 08:01:23, haraken wrote:
> 
> Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?

I don't think that's accurate - how do you reason to suggest that here?

i.e., consider MIDIAccessInitializer::start() being called with an execution
context in a suspended state. It'll set m_keepAlive. start() may then progress
and try to call reject() (say), where ScriptPromiseResolver will call
keepAliveWhilePending() again (via resolveOrReject()). m_keepAlive will then be
set.

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolverTest.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolverTest.cpp:227:
ScriptPromiseResolverKeepAlive(ScriptState* scriptState)
On 2015/07/18 08:01:23, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachest...
File Source/modules/cachestorage/CacheStorage.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/cachest...
Source/modules/cachestorage/CacheStorage.cpp:100:
MatchCallbacks(ScriptPromiseResolver* resolver)
On 2015/07/18 08:01:24, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/...
File Source/modules/crypto/CryptoResultImpl.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/crypto/...
Source/modules/crypto/CryptoResultImpl.cpp:120:
ASSERT(scriptState->contextIsValid());
On 2015/07/18 08:01:24, haraken wrote:
> 
> Nit: It would be nice to move this assert to Resolver::create.

Minor nit addressed :)

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediast...
File Source/modules/mediastream/MediaDevices.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediast...
Source/modules/mediastream/MediaDevices.cpp:37:
PromiseSuccessCallback(ScriptPromiseResolver* resolver)
On 2015/07/18 08:01:24, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/1233173002/diff/220001/Source/modules/mediast...
Source/modules/mediastream/MediaDevices.cpp:63:
PromiseErrorCallback(ScriptPromiseResolver* resolver)
On 2015/07/18 08:01:24, haraken wrote:
> 
> Add explicit.

Done.

[haraken, 2015-07-19 07:25:51]

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)
On 2015/07/18 20:46:08, sof wrote:
> On 2015/07/18 08:01:23, haraken wrote:
> > 
> > Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?
> 
> I don't think that's accurate - how do you reason to suggest that here?
> 
> i.e., consider MIDIAccessInitializer::start() being called with an execution
> context in a suspended state. It'll set m_keepAlive. start() may then progress
> and try to call reject() (say), where ScriptPromiseResolver will call
> keepAliveWhilePending() again (via resolveOrReject()). m_keepAlive will then
be
> set.

Makes sense! Would be worth having a comment -- keepAliveWhilePending can be
called multiple times but those are expected to be cleared with one clear()
call.

[sof, 2015-07-20 11:01:43]

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptPromiseResolver.cpp (right):

https://codereview.chromium.org/1233173002/diff/220001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptPromiseResolver.cpp:48: if (m_state ==
ResolvedOrRejected || m_keepAlive)
On 2015/07/19 07:25:50, haraken wrote:
> On 2015/07/18 20:46:08, sof wrote:
> > On 2015/07/18 08:01:23, haraken wrote:
> > > 
> > > Instead of adding '|| m_keepAlive', can we add ASSERT(!m_keepAlive)?
> > 
> > I don't think that's accurate - how do you reason to suggest that here?
> > 
> > i.e., consider MIDIAccessInitializer::start() being called with an execution
> > context in a suspended state. It'll set m_keepAlive. start() may then
progress
> > and try to call reject() (say), where ScriptPromiseResolver will call
> > keepAliveWhilePending() again (via resolveOrReject()). m_keepAlive will then
> be
> > set.
> 
> Makes sense! Would be worth having a comment -- keepAliveWhilePending can be
> called multiple times but those are expected to be cleared with one clear()
> call.

Added clarifying comment.

[yhirano, 2015-07-21 04:38:31]

core/streams LGTM

[haraken, 2015-07-30 11:41:21]

https://codereview.chromium.org/1233173002/diff/280001/Source/platform/heap/H...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1233173002/diff/280001/Source/platform/heap/H...
Source/platform/heap/Handle.h:1045: class SelfKeepAlive {

Can we land this (with some HeapTests) ahead of the time?

We want to use the SelfKeepAlive handle in
https://codereview.chromium.org/1257653007/ :)

[sof, 2015-07-30 11:44:25]

On 2015/07/30 11:41:21, haraken wrote:
>
https://codereview.chromium.org/1233173002/diff/280001/Source/platform/heap/H...
> File Source/platform/heap/Handle.h (right):
> 
>
https://codereview.chromium.org/1233173002/diff/280001/Source/platform/heap/H...
> Source/platform/heap/Handle.h:1045: class SelfKeepAlive {
> 
> Can we land this (with some HeapTests) ahead of the time?
> 
> We want to use the SelfKeepAlive handle in
> https://codereview.chromium.org/1257653007/ :)

I'd prefer to unblock this by landing the mazy
https://codereview.chromium.org/1228373006/ (it is way overdue.)

[sof, 2015-08-03 08:35:32]

sigbjornf@opera.com changed reviewers:
+ jochen@chromium.org

[sof, 2015-08-03 08:35:33]

With https://codereview.chromium.org/1228373006/ having landed, rebased and
updated this one. pta(quick)l?

jochen@: could you have a look at the public/ changes? tia.

[jochen (gone - plz use gerrit), 2015-08-03 08:43:23]

public lgtm

[Raymond Toy, 2015-08-03 15:46:53]

rtoy@chromium.org changed reviewers:
+ rtoy@chromium.org

[Raymond Toy, 2015-08-03 15:46:55]

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudi...
File Source/modules/webaudio/RealtimeAnalyser.cpp (right):

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudi...
Source/modules/webaudio/RealtimeAnalyser.cpp:40: const double
RealtimeAnalyser::DefaultSmoothingTimeConstant = 0.8;
Drive-by-comment:

This change seems unrelated.  Given the large size of the change, I think it
would be better not to do this here, even though it's trivial.

[sof, 2015-08-03 15:53:48]

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudi...
File Source/modules/webaudio/RealtimeAnalyser.cpp (right):

https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudi...
Source/modules/webaudio/RealtimeAnalyser.cpp:40: const double
RealtimeAnalyser::DefaultSmoothingTimeConstant = 0.8;
On 2015/08/03 15:46:54, Raymond Toy wrote:
> Drive-by-comment:
> 
> This change seems unrelated.  Given the large size of the change, I think it
> would be better not to do this here, even though it's trivial.

A whitespace change too trivial to merit a change of its own, I reasoned, after
stumbling across another one of its kind in a modules/ file.

Could you explain your reasoning for why it would be risky to include it here?

[Raymond Toy, 2015-08-03 16:19:03]

On 2015/08/03 15:53:48, sof wrote:
>
https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudi...
> File Source/modules/webaudio/RealtimeAnalyser.cpp (right):
> 
>
https://codereview.chromium.org/1233173002/diff/380001/Source/modules/webaudi...
> Source/modules/webaudio/RealtimeAnalyser.cpp:40: const double
> RealtimeAnalyser::DefaultSmoothingTimeConstant = 0.8;
> On 2015/08/03 15:46:54, Raymond Toy wrote:
> > Drive-by-comment:
> > 
> > This change seems unrelated.  Given the large size of the change, I think it
> > would be better not to do this here, even though it's trivial.
> 
> A whitespace change too trivial to merit a change of its own, I reasoned,
after
> stumbling across another one of its kind in a modules/ file.
> 
> Could you explain your reasoning for why it would be risky to include it here?

The only pain is if someone has to look through history to find how something
changed and has to examine extra files for trivial whitespace changes unrelated
to the actual change.

I won't oppose this change; I just prefer not seeing unrelated changes, even if
trivial. :-)

[sof, 2015-08-04 05:20:24]

this CL is proving hard to keep working across CLs being landed that creates
ScriptPromiseResolvers; haraken@: still content with the latest (>= ps#12) ?

[haraken, 2015-08-04 06:08:41]

On 2015/08/04 05:20:24, sof wrote:
> this CL is proving hard to keep working across CLs being landed that creates
> ScriptPromiseResolvers; haraken@: still content with the latest (>= ps#12) ?

Still LGTM

[sof, 2015-08-04 06:27:16]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-08-04 06:27:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1233173002/#ps420001
(title: "fix webusb ScriptPromiseResolver usage")

[commit-bot: I haz the power, 2015-08-04 06:27:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233173002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1233173002/420001

[commit-bot: I haz the power, 2015-08-04 07:15:27]

Committed patchset #18 (id:420001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199953