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

Issue 14044022: Web MIDI: implement MIDIAccess (Closed)

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

Description

Web MIDI: implement MIDIAccess This change implements MIDIAccess. BUG=163795 TEST=none R=crogers@google.com Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151351

Patch Set 1 #

Total comments: 3

Patch Set 2 : haraken review #

Total comments: 4

Patch Set 3 : (rebase) #

Patch Set 4 : (rebase and style fix) #

Patch Set 5 : https://github.com/WebAudio/web-midi-api/issues/2 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+201 lines, -0 lines) Patch
M Source/core/dom/EventTarget.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/EventTargetFactory.in View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/modules.gypi View 1 2 3 2 chunks +3 lines, -0 lines 0 comments Download
A Source/modules/webmidi/MIDIAccess.h View 1 2 3 4 1 chunk +83 lines, -0 lines 0 comments Download
A Source/modules/webmidi/MIDIAccess.cpp View 1 2 3 4 1 chunk +59 lines, -0 lines 0 comments Download
A Source/modules/webmidi/MIDIAccess.idl View 1 2 3 4 1 chunk +50 lines, -0 lines 1 comment Download
M Source/modules/webmidi/MIDIInput.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M Source/modules/webmidi/MIDIOutput.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
haraken
https://codereview.chromium.org/14044022/diff/1/Source/modules/webmidi/MIDIAccess.idl File Source/modules/webmidi/MIDIAccess.idl (right): https://codereview.chromium.org/14044022/diff/1/Source/modules/webmidi/MIDIAccess.idl#newcode38 Source/modules/webmidi/MIDIAccess.idl:38: [RaisesException] MIDIInput getInput(DOMString target); Due to lack of IDL ...
7 years, 8 months ago (2013-04-26 15:15:13 UTC) #1
Takashi Toyoshima
https://codereview.chromium.org/14044022/diff/1/Source/modules/webmidi/MIDIAccess.idl File Source/modules/webmidi/MIDIAccess.idl (right): https://codereview.chromium.org/14044022/diff/1/Source/modules/webmidi/MIDIAccess.idl#newcode38 Source/modules/webmidi/MIDIAccess.idl:38: [RaisesException] MIDIInput getInput(DOMString target); Thanks. I filed a bug ...
7 years, 8 months ago (2013-04-26 17:11:13 UTC) #2
haraken
The IDL change LGTM.
7 years, 8 months ago (2013-04-26 17:33:19 UTC) #3
haraken
https://codereview.chromium.org/14044022/diff/4001/Source/modules/webmidi/MIDIAccess.h File Source/modules/webmidi/MIDIAccess.h (right): https://codereview.chromium.org/14044022/diff/4001/Source/modules/webmidi/MIDIAccess.h#newcode45 Source/modules/webmidi/MIDIAccess.h:45: class Navigator; Nit: This isn't needed.
7 years, 8 months ago (2013-04-26 17:33:25 UTC) #4
Chris Rogers
lgtm with nits https://codereview.chromium.org/14044022/diff/4001/Source/modules/webmidi/MIDIAccess.cpp File Source/modules/webmidi/MIDIAccess.cpp (right): https://codereview.chromium.org/14044022/diff/4001/Source/modules/webmidi/MIDIAccess.cpp#newcode77 Source/modules/webmidi/MIDIAccess.cpp:77: if ((*i)->id() != id) Instead of ...
7 years, 7 months ago (2013-04-29 19:22:09 UTC) #5
haraken
It looks like we should wait for landing the CL until this discussion (https://code.google.com/p/chromium/issues/detail?id=235884) settles ...
7 years, 7 months ago (2013-04-29 19:24:03 UTC) #6
Takashi Toyoshima
Yes, this CL depends on MIDIInput CL. And MIDIInput IDL has a problem haraken is ...
7 years, 7 months ago (2013-05-01 14:51:57 UTC) #7
Takashi Toyoshima
> And MIDIInput IDL has a problem haraken is mentioned. This line is wrong. s/MIDIInput/MIDIAccess/.
7 years, 7 months ago (2013-05-01 14:54:30 UTC) #8
Chris Rogers
On 2013/05/01 14:51:57, Takashi Toyoshima (chromium) wrote: > Yes, this CL depends on MIDIInput CL. ...
7 years, 7 months ago (2013-05-01 17:29:30 UTC) #9
haraken
Agreed. Anyway, implementation looks good to me.
7 years, 7 months ago (2013-05-01 18:02:07 UTC) #10
Takashi Toyoshima
Hi Chris, I update this CL to follow the latest discussion; https://github.com/WebAudio/web-midi-api/issues/2. Basically, everything becomes ...
7 years, 7 months ago (2013-05-23 12:40:35 UTC) #11
Chris Rogers
https://codereview.chromium.org/14044022/diff/31001/Source/modules/webmidi/MIDIAccess.idl File Source/modules/webmidi/MIDIAccess.idl (right): https://codereview.chromium.org/14044022/diff/31001/Source/modules/webmidi/MIDIAccess.idl#newcode37 Source/modules/webmidi/MIDIAccess.idl:37: sequence<MIDIOutput> outputs(); Chris Wilson can chime in here, but ...
7 years, 7 months ago (2013-05-23 17:53:29 UTC) #12
Chris Wilson
On 2013/05/23 17:53:29, Chris Rogers wrote: > https://codereview.chromium.org/14044022/diff/31001/Source/modules/webmidi/MIDIAccess.idl > File Source/modules/webmidi/MIDIAccess.idl (right): > > https://codereview.chromium.org/14044022/diff/31001/Source/modules/webmidi/MIDIAccess.idl#newcode37 ...
7 years, 7 months ago (2013-05-23 18:53:46 UTC) #13
Takashi Toyoshima
Also, WebIDL doesn't allow us to have sequence<> attributes.
7 years, 7 months ago (2013-05-27 11:22:18 UTC) #14
Takashi Toyoshima
So, can I commit this?
7 years, 6 months ago (2013-05-28 23:45:48 UTC) #15
Chris Rogers
lgtm - we can work out the details of outputs() versus .outputs later
7 years, 6 months ago (2013-05-29 00:12:22 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/14044022/31001
7 years, 6 months ago (2013-05-29 01:57:30 UTC) #17
commit-bot: I haz the power
Change committed as 151351
7 years, 6 months ago (2013-05-29 04:02:57 UTC) #18
cwilso
7 years, 6 months ago (2013-05-30 13:52:19 UTC) #19
Yeah, it's fine for now.  I know we can't have sequence<> attributes - is
there some way to make a defined MIDIInput array if it's an attribute, or
does it need to just be an Array?


On Tue, May 28, 2013 at 5:12 PM, <crogers@google.com> wrote:

> lgtm - we can work out the details of outputs() versus .outputs later
>
>
https://chromiumcodereview.**appspot.com/14044022/<https://chromiumcodereview...
>

Powered by Google App Engine
This is Rietveld 408576698