|
|
Created:
7 years, 6 months ago by Takashi Toyoshima Modified:
7 years, 6 months ago CC:
blink-reviews, eae+blinkwatch Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionWeb MIDI: implement MIDIAccessPromise
Since Promise is not available yet, Web MIDI tentatively uses this interface
to go implementation ahead.
BUG=163795
TEST=none
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152189
Patch Set 1 #Patch Set 2 : --similarity 90 #
Total comments: 12
Patch Set 3 : review #2 #5 #Patch Set 4 : (rebase) #Patch Set 5 : Promise/ActiveDOMObject #Patch Set 6 : update to accept options #Patch Set 7 : done -> then #
Total comments: 4
Patch Set 8 : (change base to trunk) #Patch Set 9 : review #16 #
Total comments: 4
Patch Set 10 : review #19 #
Total comments: 4
Patch Set 11 : review #21 #
Total comments: 2
Patch Set 12 : rebase #
Messages
Total messages: 28 (0 generated)
Hi Chris, As you suggested, I tentatively use MIDIAccessFuture instead of DOM standard Future since it's not available yet.
Takashi, thanks. I'm adding +dglazkov +cwilso for guidance on Futures API Dimitri, Chris Wilson's example code shows that done() is the only method that needs to be invoked: http://webaudio.github.io/web-midi-api/#examples-of-web-midi-api-usage-in-jav... But Takashi's CL suggests that the API is that accept() must then be called at a later time? FWIW, I have a hacked build where I implement the Future calling done() directly - please see: https://codereview.chromium.org/16288002/ https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... File Source/modules/webmidi/MIDIAccessFuture.cpp (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:61: successCallback->scheduleCallback(m_context); shouldn't this be errorCallback? https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... File Source/modules/webmidi/MIDIAccessFuture.h (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.h:60: }; nit: I'd add a blank line here
http://dom.spec.whatwg.org/#dom-future-then Here is an actual Future spec. done() is just a method to set callbacks. These callbacks will be invoked when FutureResolver binded to the Future calls accept(), resolve(), or reject(). It is completely asynchronous thing. In this implementation, I omit to implement FutureResolver. I'll take weak pointer of MIDIAccessFuture instances in blink C++ side, and will invoke them when an asynchronous processing is done and wek pointer is still valid. Web MIDI API spec shows only done() example, but then() and other methods should be available once we move to use proper Future implementation.
On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: > http://dom.spec.whatwg.org/#dom-future-then > Here is an actual Future spec. > > done() is just a method to set callbacks. These callbacks will be invoked when > FutureResolver binded to the Future calls accept(), resolve(), or reject(). It > is completely asynchronous thing. > In this implementation, I omit to implement FutureResolver. > I'll take weak pointer of MIDIAccessFuture instances in blink C++ side, and will > invoke them when an asynchronous processing is done and wek pointer is still > valid. > > Web MIDI API spec shows only done() example, but then() and other methods should > be available once we move to use proper Future implementation. Takashi, thanks for the clarification. Chris, you should definitely update your example in the spec. I should probably leave this particular review to Dimitri.
https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... File Source/modules/webmidi/MIDIAccessFuture.cpp (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:77: if (m_state != Pending || !m_successCallback) Should you clear error callback before returning? https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:82: if (m_errorCallback) Don't need this check, right? https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:88: if (m_state != Pending || !m_errorCallback) Should you clear success callback before returning? https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:93: if (m_successCallback) Ditto.
Thanks. I also reflected MIDISuccessCallback changes introduced by https://codereview.chromium.org/16331005/ https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... File Source/modules/webmidi/MIDIAccessFuture.cpp (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:61: successCallback->scheduleCallback(m_context); On 2013/06/04 04:28:58, Chris Rogers wrote: > shouldn't this be errorCallback? Done. https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:77: if (m_state != Pending || !m_successCallback) Thank you. I reconstruct this function to meet this and the next comment. https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:82: if (m_errorCallback) On 2013/06/04 08:08:03, Dimitri Glazkov wrote: > Don't need this check, right? Done. https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:88: if (m_state != Pending || !m_errorCallback) On 2013/06/04 08:08:03, Dimitri Glazkov wrote: > Should you clear success callback before returning? Done. https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.cpp:93: if (m_successCallback) On 2013/06/04 08:08:03, Dimitri Glazkov wrote: > Ditto. Done. https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... File Source/modules/webmidi/MIDIAccessFuture.h (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MID... Source/modules/webmidi/MIDIAccessFuture.h:60: }; On 2013/06/04 04:28:58, Chris Rogers wrote: > nit: I'd add a blank line here Done.
Update the example to do what? Takashi is completely right that you *could* use then() in addition to done() (or accept() or resolve(), or even reject(), although that's of limited use WRT this API), but I didn't want to confuse the issue of using this simple API by embedding a complete discussion of Futures. What should I change it to? Merely mention in a comment that then() is also acceptable? On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: > On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: > >> http://dom.spec.whatwg.org/#**dom-future-then<http://dom.spec.whatwg.org/#dom... >> Here is an actual Future spec. >> > > done() is just a method to set callbacks. These callbacks will be invoked >> when >> FutureResolver binded to the Future calls accept(), resolve(), or >> reject(). It >> is completely asynchronous thing. >> In this implementation, I omit to implement FutureResolver. >> I'll take weak pointer of MIDIAccessFuture instances in blink C++ side, >> and >> > will > >> invoke them when an asynchronous processing is done and wek pointer is >> still >> valid. >> > > Web MIDI API spec shows only done() example, but then() and other methods >> > should > >> be available once we move to use proper Future implementation. >> > > Takashi, thanks for the clarification. Chris, you should definitely > update your > example in the spec. > > I should probably leave this particular review to Dimitri. > > > https://codereview.chromium.**org/15995010/<https://codereview.chromium.org/1... >
On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: > Update the example to do what? Takashi is completely right that you > *could* use then() in addition to done() (or accept() or resolve(), or even > reject(), although that's of limited use WRT this API), but I didn't want > to confuse the issue of using this simple API by embedding a complete > discussion of Futures. > > What should I change it to? Merely mention in a comment that then() is > also acceptable? > As I understand it, your example is not complete because *nothing* will happen when you call done() other that the callbacks will be stored away internally. It's necessary to call another method to actual set the mechanism in motion so that either one of the callbacks will be called. In other words, it's not a complete code example, because it doesn't work as it stands, and I can't copy/paste it into my little JS demos and have my success callback fire. > > On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: > >> On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: >> >>> http://dom.spec.whatwg.org/#**dom-future-then<http://dom.spec.whatwg.org/#dom... >>> Here is an actual Future spec. >>> >> >> done() is just a method to set callbacks. These callbacks will be >>> invoked when >>> FutureResolver binded to the Future calls accept(), resolve(), or >>> reject(). It >>> is completely asynchronous thing. >>> In this implementation, I omit to implement FutureResolver. >>> I'll take weak pointer of MIDIAccessFuture instances in blink C++ side, >>> and >>> >> will >> >>> invoke them when an asynchronous processing is done and wek pointer is >>> still >>> valid. >>> >> >> Web MIDI API spec shows only done() example, but then() and other methods >>> >> should >> >>> be available once we move to use proper Future implementation. >>> >> >> Takashi, thanks for the clarification. Chris, you should definitely >> update your >> example in the spec. >> >> I should probably leave this particular review to Dimitri. >> >> >> https://codereview.chromium.**org/15995010/<https://codereview.chromium.org/1... >> > >
Hmm. I thought (based on discussion with Alex) that done() and then(), for this use case, would be identical. It's possible that this use of done() is less desirable, simply because it's not chaining (so it's not "appropriate") - but I'm not as sure it wouldn't work; the intent is that the future would have been created (and initiated) from inside the Web MIDI implementation, not by the caller to requestMIDIAccess(). I thought. Futurz iz hard. :) On Tue, Jun 4, 2013 at 10:27 AM, Chris Rogers <crogers@google.com> wrote: > > > > On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: > >> Update the example to do what? Takashi is completely right that you >> *could* use then() in addition to done() (or accept() or resolve(), or even >> reject(), although that's of limited use WRT this API), but I didn't want >> to confuse the issue of using this simple API by embedding a complete >> discussion of Futures. >> >> What should I change it to? Merely mention in a comment that then() is >> also acceptable? >> > > As I understand it, your example is not complete because *nothing* will > happen when you call done() other that the callbacks will be stored away > internally. It's necessary to call another method to actual set the > mechanism in motion so that either one of the callbacks will be called. In > other words, it's not a complete code example, because it doesn't work as > it stands, and I can't copy/paste it into my little JS demos and have my > success callback fire. > > >> >> On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: >> >>> On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: >>> >>>> http://dom.spec.whatwg.org/#**dom-future-then<http://dom.spec.whatwg.org/#dom... >>>> Here is an actual Future spec. >>>> >>> >>> done() is just a method to set callbacks. These callbacks will be >>>> invoked when >>>> FutureResolver binded to the Future calls accept(), resolve(), or >>>> reject(). It >>>> is completely asynchronous thing. >>>> In this implementation, I omit to implement FutureResolver. >>>> I'll take weak pointer of MIDIAccessFuture instances in blink C++ side, >>>> and >>>> >>> will >>> >>>> invoke them when an asynchronous processing is done and wek pointer is >>>> still >>>> valid. >>>> >>> >>> Web MIDI API spec shows only done() example, but then() and other >>>> methods >>>> >>> should >>> >>>> be available once we move to use proper Future implementation. >>>> >>> >>> Takashi, thanks for the clarification. Chris, you should definitely >>> update your >>> example in the spec. >>> >>> I should probably leave this particular review to Dimitri. >>> >>> >>> https://codereview.chromium.**org/15995010/<https://codereview.chromium.org/1... >>> >> >> >
Alex, can you comment on the difference between done() and then()? Or can Dimitri and Takashi work it out, since I think Dimitri's still in Japan? On Tue, Jun 4, 2013 at 10:47 AM, Chris Wilson <cwilso@google.com> wrote: > Hmm. I thought (based on discussion with Alex) that done() and then(), > for this use case, would be identical. It's possible that this use of > done() is less desirable, simply because it's not chaining (so it's not > "appropriate") - but I'm not as sure it wouldn't work; the intent is that > the future would have been created (and initiated) from inside the Web MIDI > implementation, not by the caller to requestMIDIAccess(). > > I thought. Futurz iz hard. :) > > > On Tue, Jun 4, 2013 at 10:27 AM, Chris Rogers <crogers@google.com> wrote: > >> >> >> >> On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: >> >>> Update the example to do what? Takashi is completely right that you >>> *could* use then() in addition to done() (or accept() or resolve(), or even >>> reject(), although that's of limited use WRT this API), but I didn't want >>> to confuse the issue of using this simple API by embedding a complete >>> discussion of Futures. >>> >>> What should I change it to? Merely mention in a comment that then() is >>> also acceptable? >>> >> >> As I understand it, your example is not complete because *nothing* will >> happen when you call done() other that the callbacks will be stored away >> internally. It's necessary to call another method to actual set the >> mechanism in motion so that either one of the callbacks will be called. In >> other words, it's not a complete code example, because it doesn't work as >> it stands, and I can't copy/paste it into my little JS demos and have my >> success callback fire. >> >> >>> >>> On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: >>> >>>> On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: >>>> >>>>> http://dom.spec.whatwg.org/#**dom-future-then<http://dom.spec.whatwg.org/#dom... >>>>> Here is an actual Future spec. >>>>> >>>> >>>> done() is just a method to set callbacks. These callbacks will be >>>>> invoked when >>>>> FutureResolver binded to the Future calls accept(), resolve(), or >>>>> reject(). It >>>>> is completely asynchronous thing. >>>>> In this implementation, I omit to implement FutureResolver. >>>>> I'll take weak pointer of MIDIAccessFuture instances in blink C++ >>>>> side, and >>>>> >>>> will >>>> >>>>> invoke them when an asynchronous processing is done and wek pointer is >>>>> still >>>>> valid. >>>>> >>>> >>>> Web MIDI API spec shows only done() example, but then() and other >>>>> methods >>>>> >>>> should >>>> >>>>> be available once we move to use proper Future implementation. >>>>> >>>> >>>> Takashi, thanks for the clarification. Chris, you should definitely >>>> update your >>>> example in the spec. >>>> >>>> I should probably leave this particular review to Dimitri. >>>> >>>> >>>> https://codereview.chromium.**org/15995010/<https://codereview.chromium.org/1... >>>> >>> >>> >> >
Hi Chris (Wilson), Current MIDIAcceeeFuture is a temporally implementation to make Web MIDI implementation go ahead. The real Future interface is going to be implemented by tyoshino@, and I'll use it once it is committed. After that, all features of Future will be available with Web MIDI. For now, I'll implement only limited Future implementing only done() for testing.
We clobbered .done() based on feedback at the last TC39 meeting where we renamed them to Promise. Here's the updated IDL: https://github.com/slightlyoff/Futures/blob/master/Promise.idl Regards On Wed, Jun 5, 2013 at 2:48 PM, Chris Wilson <cwilso@google.com> wrote: > Alex, can you comment on the difference between done() and then()? Or can > Dimitri and Takashi work it out, since I think Dimitri's still in Japan? > > > On Tue, Jun 4, 2013 at 10:47 AM, Chris Wilson <cwilso@google.com> wrote: > >> Hmm. I thought (based on discussion with Alex) that done() and then(), >> for this use case, would be identical. It's possible that this use of >> done() is less desirable, simply because it's not chaining (so it's not >> "appropriate") - but I'm not as sure it wouldn't work; the intent is that >> the future would have been created (and initiated) from inside the Web MIDI >> implementation, not by the caller to requestMIDIAccess(). >> >> I thought. Futurz iz hard. :) >> >> >> On Tue, Jun 4, 2013 at 10:27 AM, Chris Rogers <crogers@google.com> wrote: >> >>> >>> >>> >>> On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: >>> >>>> Update the example to do what? Takashi is completely right that you >>>> *could* use then() in addition to done() (or accept() or resolve(), or even >>>> reject(), although that's of limited use WRT this API), but I didn't want >>>> to confuse the issue of using this simple API by embedding a complete >>>> discussion of Futures. >>>> >>>> What should I change it to? Merely mention in a comment that then() is >>>> also acceptable? >>>> >>> >>> As I understand it, your example is not complete because *nothing* will >>> happen when you call done() other that the callbacks will be stored away >>> internally. It's necessary to call another method to actual set the >>> mechanism in motion so that either one of the callbacks will be called. In >>> other words, it's not a complete code example, because it doesn't work as >>> it stands, and I can't copy/paste it into my little JS demos and have my >>> success callback fire. >>> >>> >>>> >>>> On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: >>>> >>>>> On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: >>>>> >>>>>> http://dom.spec.whatwg.org/#**dom-future-then<http://dom.spec.whatwg.org/#dom... >>>>>> Here is an actual Future spec. >>>>>> >>>>> >>>>> done() is just a method to set callbacks. These callbacks will be >>>>>> invoked when >>>>>> FutureResolver binded to the Future calls accept(), resolve(), or >>>>>> reject(). It >>>>>> is completely asynchronous thing. >>>>>> In this implementation, I omit to implement FutureResolver. >>>>>> I'll take weak pointer of MIDIAccessFuture instances in blink C++ >>>>>> side, and >>>>>> >>>>> will >>>>> >>>>>> invoke them when an asynchronous processing is done and wek pointer >>>>>> is still >>>>>> valid. >>>>>> >>>>> >>>>> Web MIDI API spec shows only done() example, but then() and other >>>>>> methods >>>>>> >>>>> should >>>>> >>>>>> be available once we move to use proper Future implementation. >>>>>> >>>>> >>>>> Takashi, thanks for the clarification. Chris, you should definitely >>>>> update your >>>>> example in the spec. >>>>> >>>>> I should probably leave this particular review to Dimitri. >>>>> >>>>> >>>>> https://codereview.chromium.**org/15995010/<https://codereview.chromium.org/1... >>>>> >>>> >>>> >>> >> >
So it should be "then()", then. Is there a more official place to point to in terms of spec than your Github repo, or is that the preferred place? And all occurrences of "Future" should be replaced with "Promise"? On Wed, Jun 5, 2013 at 8:14 AM, Alex Russell <slightlyoff@google.com> wrote: > We clobbered .done() based on feedback at the last TC39 meeting where we > renamed them to Promise. Here's the updated IDL: > > https://github.com/slightlyoff/Futures/blob/master/Promise.idl > > Regards > > > On Wed, Jun 5, 2013 at 2:48 PM, Chris Wilson <cwilso@google.com> wrote: > >> Alex, can you comment on the difference between done() and then()? Or >> can Dimitri and Takashi work it out, since I think Dimitri's still in Japan? >> >> >> On Tue, Jun 4, 2013 at 10:47 AM, Chris Wilson <cwilso@google.com> wrote: >> >>> Hmm. I thought (based on discussion with Alex) that done() and then(), >>> for this use case, would be identical. It's possible that this use of >>> done() is less desirable, simply because it's not chaining (so it's not >>> "appropriate") - but I'm not as sure it wouldn't work; the intent is that >>> the future would have been created (and initiated) from inside the Web MIDI >>> implementation, not by the caller to requestMIDIAccess(). >>> >>> I thought. Futurz iz hard. :) >>> >>> >>> On Tue, Jun 4, 2013 at 10:27 AM, Chris Rogers <crogers@google.com>wrote: >>> >>>> >>>> >>>> >>>> On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: >>>> >>>>> Update the example to do what? Takashi is completely right that you >>>>> *could* use then() in addition to done() (or accept() or resolve(), or even >>>>> reject(), although that's of limited use WRT this API), but I didn't want >>>>> to confuse the issue of using this simple API by embedding a complete >>>>> discussion of Futures. >>>>> >>>>> What should I change it to? Merely mention in a comment that then() >>>>> is also acceptable? >>>>> >>>> >>>> As I understand it, your example is not complete because *nothing* will >>>> happen when you call done() other that the callbacks will be stored away >>>> internally. It's necessary to call another method to actual set the >>>> mechanism in motion so that either one of the callbacks will be called. In >>>> other words, it's not a complete code example, because it doesn't work as >>>> it stands, and I can't copy/paste it into my little JS demos and have my >>>> success callback fire. >>>> >>>> >>>>> >>>>> On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: >>>>> >>>>>> On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: >>>>>> >>>>>>> http://dom.spec.whatwg.org/#**dom-future-then<http://dom.spec.whatwg.org/#dom... >>>>>>> Here is an actual Future spec. >>>>>>> >>>>>> >>>>>> done() is just a method to set callbacks. These callbacks will be >>>>>>> invoked when >>>>>>> FutureResolver binded to the Future calls accept(), resolve(), or >>>>>>> reject(). It >>>>>>> is completely asynchronous thing. >>>>>>> In this implementation, I omit to implement FutureResolver. >>>>>>> I'll take weak pointer of MIDIAccessFuture instances in blink C++ >>>>>>> side, and >>>>>>> >>>>>> will >>>>>> >>>>>>> invoke them when an asynchronous processing is done and wek pointer >>>>>>> is still >>>>>>> valid. >>>>>>> >>>>>> >>>>>> Web MIDI API spec shows only done() example, but then() and other >>>>>>> methods >>>>>>> >>>>>> should >>>>>> >>>>>>> be available once we move to use proper Future implementation. >>>>>>> >>>>>> >>>>>> Takashi, thanks for the clarification. Chris, you should definitely >>>>>> update your >>>>>> example in the spec. >>>>>> >>>>>> I should probably leave this particular review to Dimitri. >>>>>> >>>>>> >>>>>> https://codereview.chromium.**org/15995010/<https://codereview.chromium.org/1... >>>>>> >>>>> >>>>> >>>> >>> >> >
OK, I rename it to MIDIAccessPromise and make it ActiveDOMObject.
Also rename "done" to "then", but it never returns another Promise here. Because this is temporally interface until Promise is implemented, and it seems to be enough for testing and make our implementation go ahead.
https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback = errorCallback; This is where we need to make the request to the MIDI system to get permission. We can iterate this part in later CLs For now, we at least need to add a // FIXME comment to explain this The simplest way is to create the MIDIAccess right here: m_access = MIDIAccess::create(m_context, this); then the MIDIAccess will make the request and callback to MIDIAccessPromise::accept() ... or reject() https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... File Source/modules/webmidi/MIDIAccessPromise.h (right): https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... Source/modules/webmidi/MIDIAccessPromise.h:71: void reject(DOMError*); these will have to be made public so the MIDIAccess object can call them once it has been granted permission (or not)
I'll update CL soon. Firstly, I publish my comments to let you know directions. https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback = errorCallback; Agreed on adding comments and create the MIDIAccess right now, but I think it's not here, but in constructor, right? Asynchronous operation should start even if then() is not called. https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... File Source/modules/webmidi/MIDIAccessPromise.h (right): https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MI... Source/modules/webmidi/MIDIAccessPromise.h:71: void reject(DOMError*); ok. Also, I rename accept() to fulfill() since the DOM spec is updated like that. http://dom.spec.whatwg.org/#promises
Done!
Thanks Takashi https://codereview.chromium.org/15995010/diff/31005/Source/modules/webmidi/MI... File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://codereview.chromium.org/15995010/diff/31005/Source/modules/webmidi/MI... Source/modules/webmidi/MIDIAccessPromise.cpp:83: MIDIAccessPromise::MIDIAccessPromise(ScriptExecutionContext* context, const Dictionary& options) nit: constructor should be near top of file just below create() https://codereview.chromium.org/15995010/diff/31005/Source/modules/webmidi/MI... Source/modules/webmidi/MIDIAccessPromise.cpp:88: , m_access(MIDIAccess::create(m_context)) I think we will need to add an additional constructor argument for MIDIAccess::create() passing in |this| (this MIDIAccessPromise) so that the MIDIAccess will be able to callback to our accept() method (or reject()). Using this approach it will be very easy to get the implementation working quickly. This can be a temporary/quick way to make this work, until you can implement the PromiseResolver as you suggest in the FIXME below. I think it makes sense to do something simple and direct for now, and not worry too much about all the complicated details of Promise. Our goal is to get the MIDI API basically working, and then we can refine and make the Promise API implementation complete later.
ok, I removed comments and added the second parameter for |this| to MIDIAccess. https://chromiumcodereview.appspot.com/15995010/diff/31005/Source/modules/web... File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://chromiumcodereview.appspot.com/15995010/diff/31005/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.cpp:83: MIDIAccessPromise::MIDIAccessPromise(ScriptExecutionContext* context, const Dictionary& options) On 2013/06/07 02:50:08, Chris Rogers wrote: > nit: constructor should be near top of file just below create() Done. https://chromiumcodereview.appspot.com/15995010/diff/31005/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.cpp:88: , m_access(MIDIAccess::create(m_context)) On 2013/06/07 02:50:08, Chris Rogers wrote: > I think we will need to add an additional constructor argument for > MIDIAccess::create() passing in |this| (this MIDIAccessPromise) so that the > MIDIAccess will be able to callback to our accept() method (or reject()). Using > this approach it will be very easy to get the implementation working quickly. > > This can be a temporary/quick way to make this work, until you can implement the > PromiseResolver as you suggest in the FIXME below. I think it makes sense to do > something simple and direct for now, and not worry too much about all the > complicated details of Promise. Our goal is to get the MIDI API basically > working, and then we can refine and make the Promise API implementation complete > later. Done.
This looks almost ready https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/web... File Source/modules/webmidi/MIDIAccessPromise.h (right): https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.h:59: virtual bool canSuspend() const OVERRIDE { return true; } For ActiveDOMObject, I think you also need to implement virtual bool hasPendingActivity() const; and return |true| until either fulfill() or reject() is called. Until the async status (success/error) is determined, this must stay alive. You may want to verify this with haraken. https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.h:74: ScriptExecutionContext* m_context; Since we inherit from ActiveDOMObject, I don't think it's necessary to keep track of m_context, since scriptExecutionContext() is a method So this member can be removed
Thank you so much! https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/web... File Source/modules/webmidi/MIDIAccessPromise.h (right): https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.h:59: virtual bool canSuspend() const OVERRIDE { return true; } I see. I indirectly realize right hasPendingActivity() behavior by adding setPendingActivity()/unsetPendingActivity() at proper places. https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.h:74: ScriptExecutionContext* m_context; Thanks. I forget to remove this when I change this class to be ActiveDOMObject. Done.
https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/web... File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback.clear(); It seems that the m_successCallback cannot be cleared right here if we have just entered the Accepted state (line 71). Since when then() is called later on, it won't be able to call handleEvent to call the success callback. I think it's safer to not clear either of these callbacks, or if they are cleared to clear each one separately after the callback has actually been made
On 5 Jun 2013 17:51, "Chris Wilson" <cwilso@google.com> wrote: > > So it should be "then()", then. Yep. > Is there a more official place to point to in terms of spec than your Github repo, or is that the preferred place? And all occurrences of "Future" should be replaced with "Promise"? The WHATWG spec text has been updated. > On Wed, Jun 5, 2013 at 8:14 AM, Alex Russell <slightlyoff@google.com> wrote: >> >> We clobbered .done() based on feedback at the last TC39 meeting where we renamed them to Promise. Here's the updated IDL: >> >> https://github.com/slightlyoff/Futures/blob/master/Promise.idl >> >> Regards >> >> >> On Wed, Jun 5, 2013 at 2:48 PM, Chris Wilson <cwilso@google.com> wrote: >>> >>> Alex, can you comment on the difference between done() and then()? Or can Dimitri and Takashi work it out, since I think Dimitri's still in Japan? >>> >>> >>> On Tue, Jun 4, 2013 at 10:47 AM, Chris Wilson <cwilso@google.com> wrote: >>>> >>>> Hmm. I thought (based on discussion with Alex) that done() and then(), for this use case, would be identical. It's possible that this use of done() is less desirable, simply because it's not chaining (so it's not "appropriate") - but I'm not as sure it wouldn't work; the intent is that the future would have been created (and initiated) from inside the Web MIDI implementation, not by the caller to requestMIDIAccess(). >>>> >>>> I thought. Futurz iz hard. :) >>>> >>>> >>>> On Tue, Jun 4, 2013 at 10:27 AM, Chris Rogers <crogers@google.com> wrote: >>>>> >>>>> >>>>> >>>>> >>>>> On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: >>>>>> >>>>>> Update the example to do what? Takashi is completely right that you *could* use then() in addition to done() (or accept() or resolve(), or even reject(), although that's of limited use WRT this API), but I didn't want to confuse the issue of using this simple API by embedding a complete discussion of Futures. >>>>>> >>>>>> What should I change it to? Merely mention in a comment that then() is also acceptable? >>>>> >>>>> >>>>> As I understand it, your example is not complete because *nothing* will happen when you call done() other that the callbacks will be stored away internally. It's necessary to call another method to actual set the mechanism in motion so that either one of the callbacks will be called. In other words, it's not a complete code example, because it doesn't work as it stands, and I can't copy/paste it into my little JS demos and have my success callback fire. >>>>> >>>>>> >>>>>> >>>>>> On Mon, Jun 3, 2013 at 10:30 PM, <crogers@google.com> wrote: >>>>>>> >>>>>>> On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: >>>>>>>> >>>>>>>> http://dom.spec.whatwg.org/#dom-future-then >>>>>>>> Here is an actual Future spec. >>>>>>> >>>>>>> >>>>>>>> done() is just a method to set callbacks. These callbacks will be invoked when >>>>>>>> FutureResolver binded to the Future calls accept(), resolve(), or reject(). It >>>>>>>> is completely asynchronous thing. >>>>>>>> In this implementation, I omit to implement FutureResolver. >>>>>>>> I'll take weak pointer of MIDIAccessFuture instances in blink C++ side, and >>>>>>> >>>>>>> will >>>>>>>> >>>>>>>> invoke them when an asynchronous processing is done and wek pointer is still >>>>>>>> valid. >>>>>>> >>>>>>> >>>>>>>> Web MIDI API spec shows only done() example, but then() and other methods >>>>>>> >>>>>>> should >>>>>>>> >>>>>>>> be available once we move to use proper Future implementation. >>>>>>> >>>>>>> >>>>>>> Takashi, thanks for the clarification. Chris, you should definitely update your >>>>>>> example in the spec. >>>>>>> >>>>>>> I should probably leave this particular review to Dimitri. >>>>>>> >>>>>>> >>>>>>> https://codereview.chromium.org/15995010/ >>>>>> >>>>>> >>>>> >>>> >>> >> >
https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/web... File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/web... Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback.clear(); When then() is called later on, |successCallback| of then()'s argument should be invoked. fulfill() and reject() are only place which invoke m_successCallback or m_errorCallback. When then() invokes a callback, it uses one of arguments.
On 2013/06/10 13:56:03, Takashi Toyoshima (chromium) wrote: > https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/web... > File Source/modules/webmidi/MIDIAccessPromise.cpp (right): > > https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/web... > Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback.clear(); > When then() is called later on, |successCallback| of then()'s argument should be > invoked. > fulfill() and reject() are only place which invoke m_successCallback or > m_errorCallback. > When then() invokes a callback, it uses one of arguments. Sorry, you're right, I misread that part. lgtm - please make sure bots pass
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15995010/67001
Message was sent while issue was closed.
Change committed as 152189 |