Make WebMIDI use blink Promise.

Make WebMIDI use blink Promise.

Use Blink Promise instead of MIDIAccessPromise.
BUG=320984
R=toyoshim@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169727

[yhirano, 2013-12-03 03:22:39]

ping

[eseidel, 2013-12-03 03:31:30]

I suspect there are other Promise-capable reviewers as well which might speed
your review.

[Takashi Toyoshima, 2013-12-03 10:48:50]

This review didn't appear in my incoming review list since my mail address was
wrong. Anyway, looks almost good to me.

https://codereview.chromium.org/77773003/diff/60001/LayoutTests/webmidi/reque...
File LayoutTests/webmidi/requestmidiaccess-expected.txt (right):

https://codereview.chromium.org/77773003/diff/60001/LayoutTests/webmidi/reque...
LayoutTests/webmidi/requestmidiaccess-expected.txt:13: PASS
navigator.requestMIDIAccess({sysex: true}).then(successCallback2,
errorCallback2) did not throw exception.
Hum... I'm not sure why this line is changed by this CL.

Anyway I noticed that the following line in the requestmidiaccess.html is wrong
now.
shouldNotThrow("navigator.requestMIDIAccess({sysex:
true}).then(successCallback2, errorCallback2)");

Originally, the method is throwable, but isn't now. Can you remove
'shouldNotThread' from this line?

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/MI...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/MI...
Source/modules/webmidi/MIDIAccess.cpp:151: m_navigator->didFailRequest(this,
DOMError::create("InvalidStateError"));
AbortError?

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:79: if (!frame()) {
optional: Can we have a layout test which confirm that requestMIDIAccess() works
also in iframe or somewhere out of main frame?

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:82:
resolver->reject(DOMError::create("InvalidStateError"));
This is not the case described in the spec, but I think AbortError is right
error which we should return here.

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:148:
i->value->reject(DOMError::create("InvalidStateError"),
access->executionContext());
AbortError?

[Takashi Toyoshima, 2013-12-03 11:34:48]

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:82:
resolver->reject(DOMError::create("InvalidStateError"));
Sent a pull request to use AbortError here.
https://github.com/WebAudio/web-midi-api/pull/93

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:112: resolver->resolve(access,
access->executionContext());
I proposed to remove the second argument of the MIDISuccessCallback as you
implemented.
https://github.com/WebAudio/web-midi-api/issues/90

[yhirano, 2013-12-03 12:14:15]

Sorry, I mis-wrote your mail address > toyoshim@

abarth@, can you take a look at it?
Especially, I'm wondering what I should do when requestMIDIAccess is called with
an empty frame (NavigatorMIDI.cpp:79 @PS6 and
requestmidiaccess-in-detached-frame.html).
Since the context is already stopped[2], neither callbacks will run.

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/77773003/diff/60001/LayoutTests/webmidi/reque...
File LayoutTests/webmidi/requestmidiaccess-expected.txt (right):

https://codereview.chromium.org/77773003/diff/60001/LayoutTests/webmidi/reque...
LayoutTests/webmidi/requestmidiaccess-expected.txt:13: PASS
navigator.requestMIDIAccess({sysex: true}).then(successCallback2,
errorCallback2) did not throw exception.
On 2013/12/03 10:48:50, Takashi Toyoshima (chromium) wrote:
> Hum... I'm not sure why this line is changed by this CL.
> 
> Anyway I noticed that the following line in the requestmidiaccess.html is
wrong
> now.
> shouldNotThrow("navigator.requestMIDIAccess({sysex:
> true}).then(successCallback2, errorCallback2)");
> 
> Originally, the method is throwable, but isn't now. Can you remove
> 'shouldNotThread' from this line?

Maybe it's because Blink Promise calls onFulfill / onReject asynchronously.
I modified tests and expectations.

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/MI...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/MI...
Source/modules/webmidi/MIDIAccess.cpp:151: m_navigator->didFailRequest(this,
DOMError::create("InvalidStateError"));
On 2013/12/03 10:48:50, Takashi Toyoshima (chromium) wrote:
> AbortError?

Done.

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:79: if (!frame()) {
On 2013/12/03 10:48:50, Takashi Toyoshima (chromium) wrote:
> optional: Can we have a layout test which confirm that requestMIDIAccess()
works
> also in iframe or somewhere out of main frame?
I found that this rejection is currently not working.
Added a test and a FIXME comment.

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:82:
resolver->reject(DOMError::create("InvalidStateError"));
On 2013/12/03 10:48:50, Takashi Toyoshima (chromium) wrote:
> This is not the case described in the spec, but I think AbortError is right
> error which we should return here.

Done.

https://codereview.chromium.org/77773003/diff/60001/Source/modules/webmidi/Na...
Source/modules/webmidi/NavigatorWebMIDI.cpp:148:
i->value->reject(DOMError::create("InvalidStateError"),
access->executionContext());
On 2013/12/03 10:48:50, Takashi Toyoshima (chromium) wrote:
> AbortError?

Done.

[Takashi Toyoshima, 2013-12-03 12:44:45]

Yep, LGTM for Web MIDI.
Please wait a review by abarth@ on Promise.

[abarth-chromium, 2013-12-03 16:57:56]

not lgtm

This CL contains several serious bugs.

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:89: NavigatorWebMIDI* m_navigator;
This raw pointer is concerning.  This object looks like it can live for an
arbitrary length of time yet it has a raw pointer to another object that can
live for an arbitrary amount of time.

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
Source/modules/webmidi/NavigatorWebMIDI.cpp:83:
resolver->reject(DOMError::create("AbortError"));
Maybe we should throw an exception instead?

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
Source/modules/webmidi/NavigatorWebMIDI.cpp:90: m_requestState = adoptPtr(new
DOMRequestState(context));
Why is there only one m_requestState?  When is m_requestState cleared? 
DOMRequestState holds a DOMWrapperWorld.  What if this function is called from
multiple worlds?  You'll call the callback in the wrong world.

[yhirano, 2013-12-04 05:20:23]

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:89: NavigatorWebMIDI* m_navigator;
On 2013/12/03 16:57:57, abarth wrote:
> This raw pointer is concerning.  This object looks like it can live for an
> arbitrary length of time yet it has a raw pointer to another object that can
> live for an arbitrary amount of time.

We need a holder that holds pending MIDIAccessors and I think NavigatorWebMIDI
is appropriate.
I simplified the interaction between NavigatorWebMIDI and MIDIAccessor and added
comments.
m_navigator will be cleared when it goes invalid.

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
Source/modules/webmidi/NavigatorWebMIDI.cpp:83:
resolver->reject(DOMError::create("AbortError"));
On 2013/12/03 16:57:57, abarth wrote:
> Maybe we should throw an exception instead?

I think the spec doesn't want to throw an error except TypeError. toyoshim@,
what do you think?
http://webaudio.github.io/web-midi-api/#requestmidiaccess

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
Source/modules/webmidi/NavigatorWebMIDI.cpp:90: m_requestState = adoptPtr(new
DOMRequestState(context));
On 2013/12/03 16:57:57, abarth wrote:
> Why is there only one m_requestState?  When is m_requestState cleared? 
> DOMRequestState holds a DOMWrapperWorld.  What if this function is called from
> multiple worlds?  You'll call the callback in the wrong world.
Done.
Now each MIDIAccessor has its DOMRequestState.

[abarth-chromium, 2013-12-04 06:05:13]

I'm sorry, but I don't have the bandwidth to help you fix this CL.  Hopefully
you can find another reviewer who can help you sort this out.

https://codereview.chromium.org/77773003/diff/180001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/180001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:107: DOMRequestState m_requestState;
DOMRequestState isn't designed to be held in the heap.  It's meant to just be
used on the stack.  For example, it holds a raw pointer to an ExecutionContext.

[Takashi Toyoshima, 2013-12-04 06:48:37]

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
Source/modules/webmidi/NavigatorWebMIDI.cpp:83:
resolver->reject(DOMError::create("AbortError"));
How about other Promise or asynchronous API usages?
I guess handling all kind of errors in the error callback is easy for JavaScript
developer to handle them when they use Promise or asynchronous APIs.

[yhirano, 2013-12-05 08:19:11]

haraken@, can you take a look at this CL?

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/100001/Source/modules/webmidi/N...
Source/modules/webmidi/NavigatorWebMIDI.cpp:83:
resolver->reject(DOMError::create("AbortError"));
On 2013/12/04 06:48:37, Takashi Toyoshima (chromium) wrote:
> How about other Promise or asynchronous API usages?
> I guess handling all kind of errors in the error callback is easy for
JavaScript
> developer to handle them when they use Promise or asynchronous APIs.

I think so as well.
Anyway, this may be a Promise bug. I will try to fix it...

https://codereview.chromium.org/77773003/diff/180001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/180001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:107: DOMRequestState m_requestState;
On 2013/12/04 06:05:14, abarth wrote:
> DOMRequestState isn't designed to be held in the heap.  It's meant to just be
> used on the stack.  For example, it holds a raw pointer to an
ExecutionContext.
Done.
MIDIAccessor now stores a DOMWrapperWorld.

[haraken, 2013-12-06 00:42:28]

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:82: // |this| can be deleted here.

This sounds ugly. How about putting 'RefPtr<MIDIAccess> protect(this)' at the
beginning of this method?

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:109: // |this| can be deleted here.

Ditto. The same comment for other unprotected |this| objects.

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:206: DOMRequestState
state(executionContext(), m_world);

I don't fully understand why you need to store m_world in startRequest() and
pass the m_world explicitly to DOMRequestState here.

Is it possible that resolve() is called from a different isolated world that
called startRequest()? In other words, is it possible that m_world is different
from DOMWrapperWorld::current()?

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:235: navigator->unregisterPending(this);

The lifetime management of m_navigator looks fragile. Can you use wtf/WeakPtr?

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:98: // deregister itself from the navigator
object.

deregister => unregister

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:100: // and then |m_navigator| will be
cleard.

cleared

[yhirano, 2014-02-21 10:52:29]

PTAL? > haraken

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:82: // |this| can be deleted here.
On 2013/12/06 00:42:29, haraken wrote:
> 
> This sounds ugly. How about putting 'RefPtr<MIDIAccess> protect(this)' at the
> beginning of this method?

Done.

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:109: // |this| can be deleted here.
On 2013/12/06 00:42:29, haraken wrote:
> 
> Ditto. The same comment for other unprotected |this| objects.

Done.

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:206: DOMRequestState
state(executionContext(), m_world);
On 2013/12/06 00:42:29, haraken wrote:
> 
> I don't fully understand why you need to store m_world in startRequest() and
> pass the m_world explicitly to DOMRequestState here.
> 
> Is it possible that resolve() is called from a different isolated world that
> called startRequest()? In other words, is it possible that m_world is
different
> from DOMWrapperWorld::current()?

This function can be called from IPC handlers. In such a case,
DOMWrapperWorld::current() returns null.

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:235: navigator->unregisterPending(this);
On 2013/12/06 00:42:29, haraken wrote:
> 
> The lifetime management of m_navigator looks fragile. Can you use wtf/WeakPtr?

This class now uses [un]setPendingActivity for the purpose.

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:98: // deregister itself from the navigator
object.
On 2013/12/06 00:42:29, haraken wrote:
> 
> deregister => unregister
deleted

https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.h:100: // and then |m_navigator| will be
cleard.
On 2013/12/06 00:42:29, haraken wrote:
> 
> cleared

deleted

[haraken, 2014-02-21 11:11:39]

I'm afraid that this CL is still a bit too complicated.

MIDIAccess has a lot of states (m_requesting, m_world, DOMRequestState, pending
activity etc). We need to invent a better way to manage the states. I understand
that using Promise needs somewhat complicated implementation, but we need to
reduce the complexity.

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:178: RefPtr<MIDIAccess> protect(this);

Why do you need protection? Is this "just in case"?

It's true that |this| object can be destructed in resolve()/reject(), but
resolve()/reject() are executed at the very end of this method. Since we're not
touching |this| object after resolve()/reject(), I don't think we need the
protection. (In any event, if we're touching |this| object after
resolve()/reject(), we're doing something wrong.)

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:183: setPendingActivity(this);

m_requesting and setPendingActivity should be synchronized. Can you add a helper
method that updates the two variables?

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:205: DOMRequestState::Scope scope(state);

I don't understand why you need DOMRequestState here.

DOMRequestState holds a world. MIDIAccess also holds a world. This implies that
something is mis-designed...

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:207: m_hasAccess = true;

What is m_hasAccess for?

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:210: // FIXME: Care about suspend / stop.

I think you need to fix this in this CL.

[yhirano, 2014-02-21 13:52:35]

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:178: RefPtr<MIDIAccess> protect(this);
On 2014/02/21 11:11:40, haraken wrote:
> 
> Why do you need protection? Is this "just in case"?
> 
> It's true that |this| object can be destructed in resolve()/reject(), but
> resolve()/reject() are executed at the very end of this method. Since we're
not
> touching |this| object after resolve()/reject(), I don't think we need the
> protection. (In any event, if we're touching |this| object after
> resolve()/reject(), we're doing something wrong.)

I think they were introduced by your suggestion.
https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:183: setPendingActivity(this);
On 2014/02/21 11:11:40, haraken wrote:
> 
> m_requesting and setPendingActivity should be synchronized. Can you add a
helper
> method that updates the two variables?

Done.

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:205: DOMRequestState::Scope scope(state);
On 2014/02/21 11:11:40, haraken wrote:
> 
> I don't understand why you need DOMRequestState here.
> 
> DOMRequestState holds a world. MIDIAccess also holds a world. This implies
that
> something is mis-designed...
I introduced MIDIAccessResolver.

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:207: m_hasAccess = true;
On 2014/02/21 11:11:40, haraken wrote:
> 
> What is m_hasAccess for?

m_hasAccess is used in didReceiveMIDIData and sendMIDIData.
This class blocks input / output operations when it is false.

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:210: // FIXME: Care about suspend / stop.
On 2014/02/21 11:11:40, haraken wrote:
> 
> I think you need to fix this in this CL.

Done.

[yhirano, 2014-02-26 09:18:07]

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/760002/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccess.cpp:178: RefPtr<MIDIAccess> protect(this);
On 2014/02/21 13:52:35, yhirano wrote:
> On 2014/02/21 11:11:40, haraken wrote:
> > 
> > Why do you need protection? Is this "just in case"?
> > 
> > It's true that |this| object can be destructed in resolve()/reject(), but
> > resolve()/reject() are executed at the very end of this method. Since we're
> not
> > touching |this| object after resolve()/reject(), I don't think we need the
> > protection. (In any event, if we're touching |this| object after
> > resolve()/reject(), we're doing something wrong.)
> 
> I think they were introduced by your suggestion.
>
https://codereview.chromium.org/77773003/diff/230001/Source/modules/webmidi/M...

I deleted many of protect(this).

[haraken, 2014-02-26 11:30:38]

not LGTM because of the complicated state management.

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.h (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:49: static
PassOwnPtr<MIDIAccessResolver> create(PassRefPtr<ScriptPromiseResolver>
resolver, ExecutionContext* executionContext) { return adoptPtr(new
MIDIAccessResolver(resolver, executionContext)); }

Nit: Split into multiple lines.

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:52: void resolve(MIDIAccess*);

PassRefPtr<MIDIAccess> ?

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:59: ExecutionContext*
m_executionContext;

Why do you need to store m_executionContext to MIDIAccessResolver? I guess you
can get the executionContext when you're about to call resolve()/reject() ?

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;

Could you avoid using setPendingActivity/unsetPendingActivity by overriding
hasPendingActivity?

  bool hasPendingActivity() {
    return m_requesting;
  }

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.h (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.h:101: };

This kind of state management looks too complicated and fragile.

The fact that you're implementing the state management in MIDIAccess implis that
we have to implement the state management in each feature (e.g., MIDIAccess)
that want to use Promise. This will definitely not scale.

Unfortunately we'll need a fundamental redesign about this CL.

[yhirano, 2014-02-26 11:55:14]

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;
On 2014/02/26 11:30:39, haraken wrote:
> 
> Could you avoid using setPendingActivity/unsetPendingActivity by overriding
> hasPendingActivity?
> 
>   bool hasPendingActivity() {
>     return m_requesting;
>   }
Because MIDIAccess is not exposed to the V8 GC at the creation time, setting
hasPendingActivity don't take effect.

[haraken, 2014-02-26 12:09:51]

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;
On 2014/02/26 11:55:15, yhirano wrote:
> On 2014/02/26 11:30:39, haraken wrote:
> > 
> > Could you avoid using setPendingActivity/unsetPendingActivity by overriding
> > hasPendingActivity?
> > 
> >   bool hasPendingActivity() {
> >     return m_requesting;
> >   }
> Because MIDIAccess is not exposed to the V8 GC at the creation time, setting
> hasPendingActivity don't take effect.

Then probably should we use ContextLifecycleObserver instead of ActiveDOMObject?
ActiveDOMObject is designed for managing lifetime of wrappers (but people are
misusing it in a couple of places:-).

[yhirano, 2014-02-26 12:16:36]

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;
On 2014/02/26 12:09:52, haraken wrote:
> On 2014/02/26 11:55:15, yhirano wrote:
> > On 2014/02/26 11:30:39, haraken wrote:
> > > 
> > > Could you avoid using setPendingActivity/unsetPendingActivity by
overriding
> > > hasPendingActivity?
> > > 
> > >   bool hasPendingActivity() {
> > >     return m_requesting;
> > >   }
> > Because MIDIAccess is not exposed to the V8 GC at the creation time, setting
> > hasPendingActivity don't take effect.
> 
> Then probably should we use ContextLifecycleObserver instead of
ActiveDOMObject?
> ActiveDOMObject is designed for managing lifetime of wrappers (but people are
> misusing it in a couple of places:-).
MIDIAccess will be exposed to the GC, but it is not at the creation time.
The wrapper will be created when Promise onFulfilled callback is called.

[yhirano, 2014-02-26 12:37:11]

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;
On 2014/02/26 12:16:37, yhirano wrote:
> On 2014/02/26 12:09:52, haraken wrote:
> > On 2014/02/26 11:55:15, yhirano wrote:
> > > On 2014/02/26 11:30:39, haraken wrote:
> > > > 
> > > > Could you avoid using setPendingActivity/unsetPendingActivity by
> overriding
> > > > hasPendingActivity?
> > > > 
> > > >   bool hasPendingActivity() {
> > > >     return m_requesting;
> > > >   }
> > > Because MIDIAccess is not exposed to the V8 GC at the creation time,
setting
> > > hasPendingActivity don't take effect.
> > 
> > Then probably should we use ContextLifecycleObserver instead of
> ActiveDOMObject?
> > ActiveDOMObject is designed for managing lifetime of wrappers (but people
are
> > misusing it in a couple of places:-).
> MIDIAccess will be exposed to the GC, but it is not at the creation time.
> The wrapper will be created when Promise onFulfilled callback is called.
> 
Sorry, I was wrong. The wrapper will be created when the promise is resolved
with this object.

[haraken, 2014-02-26 12:52:39]

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;
On 2014/02/26 12:37:12, yhirano wrote:
> On 2014/02/26 12:16:37, yhirano wrote:
> > On 2014/02/26 12:09:52, haraken wrote:
> > > On 2014/02/26 11:55:15, yhirano wrote:
> > > > On 2014/02/26 11:30:39, haraken wrote:
> > > > > 
> > > > > Could you avoid using setPendingActivity/unsetPendingActivity by
> > overriding
> > > > > hasPendingActivity?
> > > > > 
> > > > >   bool hasPendingActivity() {
> > > > >     return m_requesting;
> > > > >   }
> > > > Because MIDIAccess is not exposed to the V8 GC at the creation time,
> setting
> > > > hasPendingActivity don't take effect.
> > > 
> > > Then probably should we use ContextLifecycleObserver instead of
> > ActiveDOMObject?
> > > ActiveDOMObject is designed for managing lifetime of wrappers (but people
> are
> > > misusing it in a couple of places:-).
> > MIDIAccess will be exposed to the GC, but it is not at the creation time.
> > The wrapper will be created when Promise onFulfilled callback is called.
> > 
> Sorry, I was wrong. The wrapper will be created when the promise is resolved
> with this object.

Let's discuss this issue offline tomorrow. I think we need to improve an
infrastructure of ActiveDOMObject and ContextLifecycleObserver so that they can
handle cases like MIDIAccess.

[yhirano, 2014-02-27 11:37:49]

Discussed with haraken@, kouhei@

1. Having intrinsic states is no problem, MIDIAccess can (and should) have three
states, Requesting, Resolved and Stopped.
   m_requesting and m_hasAccess can be unified to the state.
2. creating a wrapper by using v8NoInline and setting hasPendingActivity is the
best option to keep a MIDIAccess object at least in the short term.
3. MIDIAccessResolver introduced in this CL will be replaced by some bindings
classes currently being developed.
4. Decoupling the initialization process could improve the design. I will try it
in a future CL.

Is there anything missing?
I will propose another PS next week.

[yhirano, 2014-03-04 17:52:59]

PTAL

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.h (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:49: static
PassOwnPtr<MIDIAccessResolver> create(PassRefPtr<ScriptPromiseResolver>
resolver, ExecutionContext* executionContext) { return adoptPtr(new
MIDIAccessResolver(resolver, executionContext)); }
On 2014/02/26 11:30:39, haraken wrote:
> 
> Nit: Split into multiple lines.

Done.

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:52: void resolve(MIDIAccess*);
On 2014/02/26 11:30:39, haraken wrote:
> 
> PassRefPtr<MIDIAccess> ?

No, to avoid modules/webmidi inclusion in MIDIAccessResolver.cpp.
I modified the reject parameter type for consistency.

https://codereview.chromium.org/77773003/diff/1160001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:59: ExecutionContext*
m_executionContext;
On 2014/02/26 11:30:39, haraken wrote:
> 
> Why do you need to store m_executionContext to MIDIAccessResolver? I guess you
> can get the executionContext when you're about to call resolve()/reject() ?

Done.

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1160001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:199: m_requesting = true;
On 2014/02/26 12:52:40, haraken wrote:
> On 2014/02/26 12:37:12, yhirano wrote:
> > On 2014/02/26 12:16:37, yhirano wrote:
> > > On 2014/02/26 12:09:52, haraken wrote:
> > > > On 2014/02/26 11:55:15, yhirano wrote:
> > > > > On 2014/02/26 11:30:39, haraken wrote:
> > > > > > 
> > > > > > Could you avoid using setPendingActivity/unsetPendingActivity by
> > > overriding
> > > > > > hasPendingActivity?
> > > > > > 
> > > > > >   bool hasPendingActivity() {
> > > > > >     return m_requesting;
> > > > > >   }
> > > > > Because MIDIAccess is not exposed to the V8 GC at the creation time,
> > setting
> > > > > hasPendingActivity don't take effect.
> > > > 
> > > > Then probably should we use ContextLifecycleObserver instead of
> > > ActiveDOMObject?
> > > > ActiveDOMObject is designed for managing lifetime of wrappers (but
people
> > are
> > > > misusing it in a couple of places:-).
> > > MIDIAccess will be exposed to the GC, but it is not at the creation time.
> > > The wrapper will be created when Promise onFulfilled callback is called.
> > > 
> > Sorry, I was wrong. The wrapper will be created when the promise is resolved
> > with this object.
> 
> Let's discuss this issue offline tomorrow. I think we need to improve an
> infrastructure of ActiveDOMObject and ContextLifecycleObserver so that they
can
> handle cases like MIDIAccess.

Done.

[yhirano, 2014-03-10 01:35:40]

ping

[haraken, 2014-03-13 02:22:32]

Sorry for the review delay.

This CL looks much better than before. Although I'm still not really happy about
the complicated state management, it looks like it's as complex as necessary.
I'm OK with this CL with some comments.

abarth@: Would you just take a quick look at the overall approach of this CL?

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.cpp (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.cpp:41:
MIDIAccessResolver::MIDIAccessResolver(PassRefPtr<ScriptPromiseResolver>
resolver, ExecutionContext* context)

Nit: How about passing Isolate* instead of ExecutionContext* ?

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.cpp:42: : m_resolver(resolver),
m_world(DOMWrapperWorld::current(toIsolate(context))) { }

Nit:

   : m_xxx()
    , m _yyy()
{
}

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.cpp:44:
MIDIAccessResolver::~MIDIAccessResolver() { }

Nit:

XXX::~XXX()
{
}

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:89: toV8NoInline(this,
executionContext());

You might want to implement a helper method that does this hack.

Shall we create ActiveDOMObject::makeThisObjectAsynchronous() and add a good
comment into the method?

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:210: ScriptPromise promise =
ScriptPromise::createPending();

Shall we add ASSERT(frame()) ?

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/NavigatorWebMIDI.cpp:34: #include
"bindings/v8/DOMRequestState.h"

Nit: This won't be necessary. Please clean up #includes in this CL.

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/NavigatorWebMIDI.cpp:79: ScriptPromise promise =
ScriptPromise::createPending();

At this point, Frame is dead. On which context is the ScriptPromise created?

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
File Source/modules/webmidi/NavigatorWebMIDI.h (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/NavigatorWebMIDI.h:41: class ExecutionContext;

Nit: This won't be necessary.

[yhirano, 2014-03-13 05:43:21]

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.cpp (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.cpp:41:
MIDIAccessResolver::MIDIAccessResolver(PassRefPtr<ScriptPromiseResolver>
resolver, ExecutionContext* context)
On 2014/03/13 02:22:33, haraken wrote:
> 
> Nit: How about passing Isolate* instead of ExecutionContext* ?

Done.

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.cpp:42: : m_resolver(resolver),
m_world(DOMWrapperWorld::current(toIsolate(context))) { }
On 2014/03/13 02:22:33, haraken wrote:
> 
> Nit:
> 
>    : m_xxx()
>     , m _yyy()
> {
> }

Done.

https://codereview.chromium.org/77773003/diff/1200001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.cpp:44:
MIDIAccessResolver::~MIDIAccessResolver() { }
On 2014/03/13 02:22:33, haraken wrote:
> 
> Nit:
> 
> XXX::~XXX()
> {
> }

Done.

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:89: toV8NoInline(this,
executionContext());
On 2014/03/13 02:22:33, haraken wrote:
> 
> You might want to implement a helper method that does this hack.
> 
> Shall we create ActiveDOMObject::makeThisObjectAsynchronous() and add a good
> comment into the method?

It doesn't work because toV8NoInline can't be applied for ActiveDOMObject.

I'm planning to factor out this async-initialization operation to a class which
each Promise resolution target DOMObject can use, in the folowup CL.
The class will hide toV8NoInline from the customers.
So, I would keep it be.

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:210: ScriptPromise promise =
ScriptPromise::createPending();
On 2014/03/13 02:22:33, haraken wrote:
> 
> Shall we add ASSERT(frame()) ?

Done.

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
File Source/modules/webmidi/NavigatorWebMIDI.cpp (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/NavigatorWebMIDI.cpp:34: #include
"bindings/v8/DOMRequestState.h"
On 2014/03/13 02:22:33, haraken wrote:
> 
> Nit: This won't be necessary. Please clean up #includes in this CL.

Done.

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/NavigatorWebMIDI.cpp:79: ScriptPromise promise =
ScriptPromise::createPending();
On 2014/03/13 02:22:33, haraken wrote:
> 
> At this point, Frame is dead. On which context is the ScriptPromise created?

It's the calling context.
requestmidiaccess-in-detached-frame.html[1] which is added in this CL covers
this code block.
Please note that the coming V8 Promise[2] doesn't have a wrapper and we won't
need creationContext.

[1]
https://codereview.chromium.org/77773003/diff/1200001/LayoutTests/webmidi/req...
[2] https://codereview.chromium.org/194663003/diff/20001/include/v8.h

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
File Source/modules/webmidi/NavigatorWebMIDI.h (right):

https://codereview.chromium.org/77773003/diff/1200001/Source/modules/webmidi/...
Source/modules/webmidi/NavigatorWebMIDI.h:41: class ExecutionContext;
On 2014/03/13 02:22:33, haraken wrote:
> 
> Nit: This won't be necessary.

Done.

[haraken, 2014-03-13 05:48:45]

ok, LGTM from my side.

Let's give abarth a chance to take another look.

[yhirano, 2014-03-17 08:57:22]

abarth, do you have any comments?

[abarth-chromium, 2014-03-20 00:43:46]

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.h (right):

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:30: // OF THIS SOFTWARE, EVEN IF ADVISED
OF THE POSSIBILITY OF SUCH DAMAGE.
Please use new-style copyright blocks:

http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:40: class isolate;
This must not be needed because the class is named Isolate...

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:66: RefPtr<DOMWrapperWorld> m_world;
Is RefPtr<DOMWrapperWorld> allowed anymore?  haraken would know.  You should
double-check with him before landing.

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:63: MIDIAccess* m_owner;
This raw pointer is scary...

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:89: toV8NoInline(this,
executionContext());
This seems like a bit of a hack.  We don't use this pattern elsewhere...  How
does other code solve this problem?

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:194: m_state = Stopped;
How does this clear out the m_owner pointer in PostAction?  At this point |this|
can be GCed...

[yhirano, 2014-03-20 02:40:50]

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.h (right):

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:30: // OF THIS SOFTWARE, EVEN IF ADVISED
OF THE POSSIBILITY OF SUCH DAMAGE.
On 2014/03/20 00:43:46, abarth wrote:
> Please use new-style copyright blocks:
> 
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:40: class isolate;
On 2014/03/20 00:43:46, abarth wrote:
> This must not be needed because the class is named Isolate...

Done.

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:66: RefPtr<DOMWrapperWorld> m_world;
On 2014/03/20 00:43:46, abarth wrote:
> Is RefPtr<DOMWrapperWorld> allowed anymore?  haraken would know.  You should
> double-check with him before landing.
Currently FrameLoader and ErrorEvent use RefPtr<DOMWrapperWorld> in
/Source/core. 

haraken said "we should use RefPtr<DOMWrapper> when we store DOMWrapperWorld in
heap allocated objects..." in his CL[1] landed this month.

[1]: https://src.chromium.org/viewvc/blink?revision=168874&view=revision

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:63: MIDIAccess* m_owner;
On 2014/03/20 00:43:46, abarth wrote:
> This raw pointer is scary...

Done.

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:89: toV8NoInline(this,
executionContext());
On 2014/03/20 00:43:46, abarth wrote:
> This seems like a bit of a hack.  We don't use this pattern elsewhere...  How
> does other code solve this problem?
Another CL[1] being reviewed is using this pattern.
Note that I will generalize this "Async initialization of Promise resolution
target DOM objects" pattern and hide this hack in the common base class in the
followup CL.

Another solution is retaining objects in a global variable explicitly.
modules/quota does this. The QuotaDispatcher singleton object has pending
callbacks[2].

In the latter pattern, we have to pass the initialization context ownership
around pending operations if we have multiple initialization steps.

The former pattern is more natural to me, because the initializing context holds
all.

[1] https://codereview.chromium.org/104363002/
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/quot...

https://codereview.chromium.org/77773003/diff/1220001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:194: m_state = Stopped;
On 2014/03/20 00:43:46, abarth wrote:
> How does this clear out the m_owner pointer in PostAction?  At this point
|this|
> can be GCed...

Done.

[haraken, 2014-03-20 04:37:15]

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
File Source/bindings/v8/MIDIAccessResolver.h (right):

https://codereview.chromium.org/77773003/diff/1220001/Source/bindings/v8/MIDI...
Source/bindings/v8/MIDIAccessResolver.h:66: RefPtr<DOMWrapperWorld> m_world;
On 2014/03/20 02:40:51, yhirano wrote:
> On 2014/03/20 00:43:46, abarth wrote:
> > Is RefPtr<DOMWrapperWorld> allowed anymore?  haraken would know.  You should
> > double-check with him before landing.
> Currently FrameLoader and ErrorEvent use RefPtr<DOMWrapperWorld> in
> /Source/core. 
> 
> haraken said "we should use RefPtr<DOMWrapper> when we store DOMWrapperWorld
in
> heap allocated objects..." in his CL[1] landed this month.
> 
> [1]: https://src.chromium.org/viewvc/blink?revision=168874&view=revision

RefPtr<DOMWrapperWorld> is ok here. Dan and I discussed whether this should be a
RefPtr or a raw pointer and concluded that a RefPtr is slightly better
(https://codereview.chromium.org/186633002/).

In the future, I'll implement a better mechanism to wrap DOMWrapperWorld and
ExecutionContext.

[abarth-chromium, 2014-03-20 16:44:24]

Ok, sounds like you and haraken have things under control.  Please feel free to
land this CL once haraken is happy.

[haraken, 2014-03-20 19:17:21]

On 2014/03/20 16:44:24, abarth wrote:
> Ok, sounds like you and haraken have things under control.  Please feel free
to
> land this CL once haraken is happy.

I re-scaned the CL. LGTM.

[yhirano, 2014-03-20 19:25:46]

Thank you!

[yhirano, 2014-03-20 19:25:58]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-20 19:26:07]

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

[commit-bot: I haz the power, 2014-03-20 19:26:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 19:26:46]

Failed to apply patch for Source/modules/webmidi/NavigatorWebMIDI.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/webmidi/NavigatorWebMIDI.h
  Hunk #1 FAILED at 31.
  Hunk #2 succeeded at 47 with fuzz 2 (offset 2 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
Source/modules/webmidi/NavigatorWebMIDI.h.rej

Patch:       Source/modules/webmidi/NavigatorWebMIDI.h
Index: Source/modules/webmidi/NavigatorWebMIDI.h
diff --git a/Source/modules/webmidi/NavigatorWebMIDI.h
b/Source/modules/webmidi/NavigatorWebMIDI.h
index
11b7f6984d40c9b520c36f6e92b58a8975d8d582..cc11f1c1f3ce371f17c7c4eb18c3494b08e2cf68
100644
--- a/Source/modules/webmidi/NavigatorWebMIDI.h
+++ b/Source/modules/webmidi/NavigatorWebMIDI.h
@@ -31,13 +31,13 @@
 #ifndef NavigatorWebMIDI_h
 #define NavigatorWebMIDI_h
 
+#include "bindings/v8/ScriptPromise.h"
 #include "core/frame/DOMWindowProperty.h"
 #include "modules/webmidi/MIDIOptions.h"
 #include "platform/Supplementable.h"
 
 namespace WebCore {
 
-class MIDIAccessPromise;
 class Navigator;
 
 class NavigatorWebMIDI FINAL : public Supplement<Navigator>, public
DOMWindowProperty {
@@ -45,8 +45,8 @@ public:
     virtual ~NavigatorWebMIDI();
     static NavigatorWebMIDI& from(Navigator&);
 
-    static PassRefPtrWillBeRawPtr<MIDIAccessPromise>
requestMIDIAccess(Navigator&, const Dictionary&);
-    PassRefPtrWillBeRawPtr<MIDIAccessPromise> requestMIDIAccess(const
Dictionary&);
+    static ScriptPromise requestMIDIAccess(Navigator&, const Dictionary&);
+    ScriptPromise requestMIDIAccess(const Dictionary&);
 
 private:
     NavigatorWebMIDI(LocalFrame*);

[yhirano, 2014-03-20 20:03:29]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:03:40]

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

[commit-bot: I haz the power, 2014-03-20 20:16:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:16:51]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[yhirano, 2014-03-20 20:30:54]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:30:59]

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

[commit-bot: I haz the power, 2014-03-20 20:34:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:34:22]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[yhirano, 2014-03-20 21:09:32]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:09:42]

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

[commit-bot: I haz the power, 2014-03-20 21:21:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 21:21:14]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[yhirano, 2014-03-21 00:36:53]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:37:01]

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

[commit-bot: I haz the power, 2014-03-21 00:39:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 00:39:53]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[yhirano, 2014-03-21 01:09:27]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-21 01:09:52]

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

[commit-bot: I haz the power, 2014-03-21 02:22:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 02:22:23]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[yhirano, 2014-03-21 03:11:59]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-21 03:12:08]

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

[commit-bot: I haz the power, 2014-03-21 04:25:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 04:25:33]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[yhirano, 2014-03-21 06:40:50]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-21 06:41:02]

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

[commit-bot: I haz the power, 2014-03-21 07:14:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 07:14:06]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[yhirano, 2014-03-21 07:56:55]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-03-21 07:57:07]

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

[commit-bot: I haz the power, 2014-03-21 07:57:38]

Change committed as 169727

[vsevik, 2014-03-21 08:48:44]

A revert of this CL has been created in
https://codereview.chromium.org/207583005/ by vsevik@chromium.org.

The reason for reverting is: This patch has broken oilpan compilation.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Oilpan/bu....