Web MIDI: MIDIAccessor platform implementation and Blink API

Web MIDI: MIDIAccessor platform implementation and Blink API

This change implements MIDIAccessor in platform in order to access
platform dependent MIDI handling from MIDIAccess and MIDIPort.
It must be exposed as Blink API.

TEST=none
BUG=163795
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152715

R=abarth@chromium.org, crogers@google.com, tkent@chromium.org

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

[Takashi Toyoshima, 2013-06-18 13:32:02]

Hi Chris,

Can you take a look this?

[Chris Rogers, 2013-06-18 17:21:36]

Thanks Takashi - lgtm

You'll also need to get OWNERS approval for the Blink API

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
File Source/core/platform/midi/MIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
Source/core/platform/midi/MIDIAccessorClient.h:48: #endif //
MIDIAccessoClientr_h
nit: MIDIAccessoClientr -> MIDIAccessorClient

[Chris Rogers, 2013-06-18 19:54:41]

Takashi, after applying and trying your patch, I realized that you forgot the
accessApproved(bool approved) method,which is necessary so the client can call
the success or failure callback

Please also re-order the methods in both the WebKit and "normal" API as follows:

addInputPort()
addOutputPort()
accessApproved()
receiveMIDIData()

because this is the order that the "accessor" will call the client methods (it
shows the calling sequence)

Still lgtm *if* you make these changes and get Blink OWNERS approval

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
File Source/core/platform/midi/MIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
Source/core/platform/midi/MIDIAccessorClient.h:39: public:
Please add:
    virtual void accessApproved(bool approved) = 0;

https://codereview.chromium.org/17155009/diff/2001/public/platform/WebMIDIAcc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/2001/public/platform/WebMIDIAcc...
public/platform/WebMIDIAccessorClient.h:41: public:
Please add:
    virtual void accessApproved(bool approved) = 0;

[Takashi Toyoshima, 2013-06-19 05:26:23]

+tkent

Hi Kent-san,
Can you take a look this as an API owner?

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
File Source/core/platform/midi/MIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
Source/core/platform/midi/MIDIAccessorClient.h:39: public:
oh, thanks!

https://codereview.chromium.org/17155009/diff/2001/Source/core/platform/midi/...
Source/core/platform/midi/MIDIAccessorClient.h:48: #endif //
MIDIAccessoClientr_h
On 2013/06/18 17:21:36, Chris Rogers wrote:
> nit: MIDIAccessoClientr -> MIDIAccessorClient

Done.

https://codereview.chromium.org/17155009/diff/2001/public/platform/WebMIDIAcc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/2001/public/platform/WebMIDIAcc...
public/platform/WebMIDIAccessorClient.h:41: public:
On 2013/06/18 19:54:42, Chris Rogers wrote:
> Please add:
>     virtual void accessApproved(bool approved) = 0;

Done.

[tkent, 2013-06-19 05:48:44]

https://codereview.chromium.org/17155009/diff/13001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/17155009/diff/13001/public/platform/Platform....
public/platform/Platform.h:158: virtual WebMIDIAccessor*
createMIDIAccessor(WebMIDIAccessorClient*) { return 0; }
Who is responsible to delete the resultant WebMIDIAccessor object?

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessor.h (right):

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessor.h:44: virtual void requestAccess(int access) { }
What's this int argument?

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:45: virtual void accessApproved(bool
approved) = 0;
nit: we had better introduce an enum.   Bool arguments are harmful.

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:46: virtual void
receiveMIDIData(unsigned portIndex, const unsigned char* data, size_t length,
double timeStamp) = 0;
What's the specification of "timeStamp"?  seconds?  milli-seconds?  Wha time is
the epoch?  Is it ok to contain fractional part?

[Takashi Toyoshima, 2013-06-19 06:37:18]

Thanks.
I didn't change code yet, but reply to confirm directions.

https://codereview.chromium.org/17155009/diff/13001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/17155009/diff/13001/public/platform/Platform....
public/platform/Platform.h:158: virtual WebMIDIAccessor*
createMIDIAccessor(WebMIDIAccessorClient*) { return 0; }
core/platform/midi/MIDIAccessor owns this as OwnPtr.
It exists in this change;
https://codereview.chromium.org/14196004/diff/101001/Source/core/platform/mid...

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessor.h (right):

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessor.h:44: virtual void requestAccess(int access) { }
MIDIOptions.sysexEnabled is mapped to this.
I think this will be changed to enum { OptionsSysexEnabled = 1 << 0, ... } if
the option will have others in the future.
But, I guess you mean that we should make it enum from the beginning for
readability.

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/13001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:46: virtual void
receiveMIDIData(unsigned portIndex, const unsigned char* data, size_t length,
double timeStamp) = 0;
In Web MIDI, timestamp/timeStamp implies DOMHighResTimeStamp measured in
milliseconds.
Should I explicitly mention it as an argument name?

[tkent, 2013-06-19 07:03:24]

Ambiguous points should be documented or improved in the code.
WebMIDIAccessor is a public interface, and someone other than you might
implement WebMIDIAccessor for non-Chromium environment.

[Takashi Toyoshima, 2013-06-19 07:45:03]

Hi Kent-san,
As we talked offline, I added comments.

[tkent, 2013-06-19 07:59:38]

lgtm

[commit-bot: I haz the power, 2013-06-19 09:52:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/17155009/22001

[commit-bot: I haz the power, 2013-06-19 11:56:07]

Change committed as 152715

[abarth-chromium, 2013-06-19 17:08:58]

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/Pl...
File public/platform/Platform.h (right):

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/Pl...
public/platform/Platform.h:161: 
Please keep two blank lines between each section.

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
File public/platform/WebMIDIAccessor.h (right):

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
public/platform/WebMIDIAccessor.h:38: // Abstract interface to the Chromium MIDI
system.
This comment isn't needed.

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
public/platform/WebMIDIAccessor.h:45: virtual void requestAccess(int access) { }
Why not use a bool or an enum for |access| ?  Using an int seems pretty opaque.

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
File public/platform/WebMIDIAccessorClient.h (right):

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
public/platform/WebMIDIAccessorClient.h:38: // Abstract client interface to the
Chromium MIDI system.
This comment isn't necessary.

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
public/platform/WebMIDIAccessorClient.h:43: virtual void addOutputPort(const
WebString& id, const WebString& manufacturer, const WebString& name, const
WebString& version) = 0;
Do these functions instruct the callee to add these ports?  If they're
notifications, they should be named in a notification style (e.g.,
didAddInputPort / didAddOutputPort).

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
public/platform/WebMIDIAccessorClient.h:45: virtual void accessApproved(bool
approved) = 0;
What does it mean to call accessApproved(false) ?  Does that mean the access
wasn't approved?  Maybe didAllowAccess and didBlockAccess?

https://chromiumcodereview.appspot.com/17155009/diff/22001/public/platform/We...
public/platform/WebMIDIAccessorClient.h:46: virtual void
receiveMIDIData(unsigned portIndex, const unsigned char* data, size_t length,
double timeStamp) = 0;
didReceiveMIDIData ?

Should we have a comment here saying that |timeStamp| is in milliseconds also?

[Chris Rogers, 2013-06-19 19:43:31]

abarth: PTAL - I hope to have addressed your comments

https://codereview.chromium.org/17155009/diff/22001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/17155009/diff/22001/public/platform/Platform....
public/platform/Platform.h:161: 
On 2013/06/19 17:08:59, abarth wrote:
> Please keep two blank lines between each section.

Done.

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessor.h (right):

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessor.h:38: // Abstract interface to the Chromium MIDI
system.
On 2013/06/19 17:08:59, abarth wrote:
> This comment isn't needed.

Done.

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessor.h:45: virtual void requestAccess(int access) { }
On 2013/06/19 17:08:59, abarth wrote:
> Why not use a bool or an enum for |access| ?  Using an int seems pretty
opaque.

changing to bool

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
File public/platform/WebMIDIAccessorClient.h (right):

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:38: // Abstract client interface to the
Chromium MIDI system.
On 2013/06/19 17:08:59, abarth wrote:
> This comment isn't necessary.

Done.

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:43: virtual void addOutputPort(const
WebString& id, const WebString& manufacturer, const WebString& name, const
WebString& version) = 0;
On 2013/06/19 17:08:59, abarth wrote:
> Do these functions instruct the callee to add these ports?  If they're
> notifications, they should be named in a notification style (e.g.,
> didAddInputPort / didAddOutputPort).

Done.

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:45: virtual void accessApproved(bool
approved) = 0;
On 2013/06/19 17:08:59, abarth wrote:
> What does it mean to call accessApproved(false) ?  Does that mean the access
> wasn't approved?  Maybe didAllowAccess and didBlockAccess?

Done.

https://codereview.chromium.org/17155009/diff/22001/public/platform/WebMIDIAc...
public/platform/WebMIDIAccessorClient.h:46: virtual void
receiveMIDIData(unsigned portIndex, const unsigned char* data, size_t length,
double timeStamp) = 0;
On 2013/06/19 17:08:59, abarth wrote:
> didReceiveMIDIData ?
> 
> Should we have a comment here saying that |timeStamp| is in milliseconds also?

Done.

[abarth-chromium, 2013-06-19 20:13:42]

LGTM.  Thanks!

[Chris Rogers, 2013-06-19 21:51:18]

Committed patchset #6 manually as r152732.

[jamesr, 2013-06-19 22:20:18]

On 2013/06/19 21:51:18, Chris Rogers wrote:
> Committed patchset #6 manually as r152732.

Was there a pressing need to land this on a closed tree?

[Chris Rogers, 2013-06-19 22:33:40]

On Wed, Jun 19, 2013 at 3:20 PM, <jamesr@chromium.org> wrote:

> On 2013/06/19 21:51:18, Chris Rogers wrote:
>
>> Committed patchset #6 manually as r152732.
>>
>
> Was there a pressing need to land this on a closed tree?
>

Sorry, my bad...


>
>
https://codereview.chromium.**org/17155009/<https://codereview.chromium.org/1...
>

[tkent, 2013-06-20 01:39:37]

On 2013/06/19 22:33:40, Chris Rogers wrote:
> On Wed, Jun 19, 2013 at 3:20 PM, <mailto:jamesr@chromium.org> wrote:
> 
> > On 2013/06/19 21:51:18, Chris Rogers wrote:
> >
> >> Committed patchset #6 manually as r152732.

I reverted Chris's changes because of build breakage.