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

Issue 15995010: Web MIDI: implement MIDIAccessPromise (Closed)

Created:
7 years, 6 months ago by Takashi Toyoshima
Modified:
7 years, 6 months ago
CC:
blink-reviews, eae+blinkwatch
Visibility:
Public.

Description

Web 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+252 lines, -5 lines) Patch
M Source/modules/modules.gypi View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download
M Source/modules/webmidi/MIDIAccess.h View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -2 lines 0 comments Download
M Source/modules/webmidi/MIDIAccess.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -3 lines 0 comments Download
A Source/modules/webmidi/MIDIAccessPromise.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +84 lines, -0 lines 0 comments Download
A Source/modules/webmidi/MIDIAccessPromise.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +118 lines, -0 lines 0 comments Download
A Source/modules/webmidi/MIDIAccessPromise.idl View 1 2 3 4 5 6 1 chunk +39 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
Takashi Toyoshima
Hi Chris, As you suggested, I tentatively use MIDIAccessFuture instead of DOM standard Future since ...
7 years, 6 months ago (2013-06-03 14:49:10 UTC) #1
Chris Rogers
Takashi, thanks. I'm adding +dglazkov +cwilso for guidance on Futures API Dimitri, Chris Wilson's example ...
7 years, 6 months ago (2013-06-04 04:28:58 UTC) #2
Takashi Toyoshima
http://dom.spec.whatwg.org/#dom-future-then Here is an actual Future spec. done() is just a method to set callbacks. ...
7 years, 6 months ago (2013-06-04 04:46:50 UTC) #3
Chris Rogers
On 2013/06/04 04:46:50, Takashi Toyoshima (chromium) wrote: > http://dom.spec.whatwg.org/#dom-future-then > Here is an actual Future ...
7 years, 6 months ago (2013-06-04 05:30:25 UTC) #4
dglazkov
https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MIDIAccessFuture.cpp File Source/modules/webmidi/MIDIAccessFuture.cpp (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MIDIAccessFuture.cpp#newcode77 Source/modules/webmidi/MIDIAccessFuture.cpp:77: if (m_state != Pending || !m_successCallback) Should you clear ...
7 years, 6 months ago (2013-06-04 08:08:03 UTC) #5
Takashi Toyoshima
Thanks. I also reflected MIDISuccessCallback changes introduced by https://codereview.chromium.org/16331005/ https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MIDIAccessFuture.cpp File Source/modules/webmidi/MIDIAccessFuture.cpp (right): https://codereview.chromium.org/15995010/diff/2001/Source/modules/webmidi/MIDIAccessFuture.cpp#newcode61 Source/modules/webmidi/MIDIAccessFuture.cpp:61: ...
7 years, 6 months ago (2013-06-04 11:44:36 UTC) #6
cwilso
Update the example to do what? Takashi is completely right that you *could* use then() ...
7 years, 6 months ago (2013-06-04 16:16:42 UTC) #7
Chris Rogers
On Tue, Jun 4, 2013 at 9:16 AM, Chris Wilson <cwilso@google.com> wrote: > Update the ...
7 years, 6 months ago (2013-06-04 17:27:11 UTC) #8
cwilso
Hmm. I thought (based on discussion with Alex) that done() and then(), for this use ...
7 years, 6 months ago (2013-06-04 17:47:20 UTC) #9
cwilso
Alex, can you comment on the difference between done() and then()? Or can Dimitri and ...
7 years, 6 months ago (2013-06-05 13:48:25 UTC) #10
Takashi Toyoshima
Hi Chris (Wilson), Current MIDIAcceeeFuture is a temporally implementation to make Web MIDI implementation go ...
7 years, 6 months ago (2013-06-05 14:06:05 UTC) #11
slightlyoff1
We clobbered .done() based on feedback at the last TC39 meeting where we renamed them ...
7 years, 6 months ago (2013-06-05 15:14:51 UTC) #12
cwilso
So it should be "then()", then. Is there a more official place to point to ...
7 years, 6 months ago (2013-06-05 16:51:59 UTC) #13
Takashi Toyoshima
OK, I rename it to MIDIAccessPromise and make it ActiveDOMObject.
7 years, 6 months ago (2013-06-06 12:56:14 UTC) #14
Takashi Toyoshima
Also rename "done" to "then", but it never returns another Promise here. Because this is ...
7 years, 6 months ago (2013-06-06 14:28:37 UTC) #15
Chris Rogers
https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MIDIAccessPromise.cpp File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://codereview.chromium.org/15995010/diff/35001/Source/modules/webmidi/MIDIAccessPromise.cpp#newcode76 Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback = errorCallback; This is where we need to ...
7 years, 6 months ago (2013-06-06 17:02:24 UTC) #16
Takashi Toyoshima
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/MIDIAccessPromise.cpp ...
7 years, 6 months ago (2013-06-07 02:06:56 UTC) #17
Takashi Toyoshima
Done!
7 years, 6 months ago (2013-06-07 02:34:19 UTC) #18
Chris Rogers
Thanks Takashi https://codereview.chromium.org/15995010/diff/31005/Source/modules/webmidi/MIDIAccessPromise.cpp File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://codereview.chromium.org/15995010/diff/31005/Source/modules/webmidi/MIDIAccessPromise.cpp#newcode83 Source/modules/webmidi/MIDIAccessPromise.cpp:83: MIDIAccessPromise::MIDIAccessPromise(ScriptExecutionContext* context, const Dictionary& options) nit: constructor ...
7 years, 6 months ago (2013-06-07 02:50:08 UTC) #19
Takashi Toyoshima
ok, I removed comments and added the second parameter for |this| to MIDIAccess. https://chromiumcodereview.appspot.com/15995010/diff/31005/Source/modules/webmidi/MIDIAccessPromise.cpp File ...
7 years, 6 months ago (2013-06-07 03:55:37 UTC) #20
Chris Rogers
This looks almost ready https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/webmidi/MIDIAccessPromise.h File Source/modules/webmidi/MIDIAccessPromise.h (right): https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/webmidi/MIDIAccessPromise.h#newcode59 Source/modules/webmidi/MIDIAccessPromise.h:59: virtual bool canSuspend() const OVERRIDE ...
7 years, 6 months ago (2013-06-07 04:27:25 UTC) #21
Takashi Toyoshima
Thank you so much! https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/webmidi/MIDIAccessPromise.h File Source/modules/webmidi/MIDIAccessPromise.h (right): https://chromiumcodereview.appspot.com/15995010/diff/55001/Source/modules/webmidi/MIDIAccessPromise.h#newcode59 Source/modules/webmidi/MIDIAccessPromise.h:59: virtual bool canSuspend() const OVERRIDE ...
7 years, 6 months ago (2013-06-07 04:55:17 UTC) #22
Chris Rogers
https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/webmidi/MIDIAccessPromise.cpp File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/webmidi/MIDIAccessPromise.cpp#newcode76 Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback.clear(); It seems that the m_successCallback cannot be cleared ...
7 years, 6 months ago (2013-06-07 19:19:45 UTC) #23
slightlyoff1
On 5 Jun 2013 17:51, "Chris Wilson" <cwilso@google.com> wrote: > > So it should be ...
7 years, 6 months ago (2013-06-07 21:17:45 UTC) #24
Takashi Toyoshima
https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/webmidi/MIDIAccessPromise.cpp File Source/modules/webmidi/MIDIAccessPromise.cpp (right): https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/webmidi/MIDIAccessPromise.cpp#newcode76 Source/modules/webmidi/MIDIAccessPromise.cpp:76: m_errorCallback.clear(); When then() is called later on, |successCallback| of ...
7 years, 6 months ago (2013-06-10 13:56:03 UTC) #25
Chris Rogers
On 2013/06/10 13:56:03, Takashi Toyoshima (chromium) wrote: > https://chromiumcodereview.appspot.com/15995010/diff/58001/Source/modules/webmidi/MIDIAccessPromise.cpp > File Source/modules/webmidi/MIDIAccessPromise.cpp (right): > > ...
7 years, 6 months ago (2013-06-10 20:59:18 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15995010/67001
7 years, 6 months ago (2013-06-11 01:23:34 UTC) #27
commit-bot: I haz the power
7 years, 6 months ago (2013-06-11 06:04:20 UTC) #28
Message was sent while issue was closed.
Change committed as 152189

Powered by Google App Engine
This is Rietveld 408576698