WebMIDI: Add MIDIOptions dictionary

WebMIDI: Add MIDIOptions dictionary

We have basic support for IDL dictionary so we can use it for
MIDIOptions. Renamed m_sysexEnabled and setSysexEnabled()
because their role have changed.

MIDIOptions is the first IDL dictionary in modules/ so this CL
also contains GYP/GN changes.

Existing tests (LayoutTests/webmidi) should cover this change.

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

[bashi, 2014-09-09 04:33:38]

bashi@chromium.org changed reviewers:
+ toyoshim@chromium.org

[bashi, 2014-09-09 04:33:38]

Toyoshima-san,

Could you take a look at changes in modules/webmidi? I removed m_options from
MIDIAccessInitializer and merged m_options.sysex with m_sysexEanbled.
I confirmed that layout tests are passed, but I'm not sure this is OK.

Here is a draft of how IDL dictionaries in Blink works:
https://docs.google.com/document/d/1mRB5zbfHd0lX2Y_Hr7g6grzP_L4Xc6_gBRjL-AE7s...

[Takashi Toyoshima, 2014-09-10 04:07:51]

Thank you for this improvement.
A small request is here.

https://codereview.chromium.org/547383004/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/547383004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:24: if (options->hasSysex())
hum... this change works, but now the variable name is curious.
Originally, m_options.sysex means that we are going to request a sysex
permission, and m_sysexEnabled means that the request is permitted by the
browser process.

So, maybe the better change is:

1. rename m_sysexEnabled to m_requestSysex
2. do not update the variable in setSysexEnabled(). now we do not need to hold
the result since we just reject() here.

Also, MIDIAccessInitializer::setSysexEnabled(bool enable) is still curious name
now. resolveSysexPermission(bool accept) or something like that. But this will
affect many module. So, I'm ok to keep the function name as is. I may handle it
later.

[bashi, 2014-09-10 08:04:39]

Thank you for review!

https://codereview.chromium.org/547383004/diff/40001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/547383004/diff/40001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:24: if (options->hasSysex())
On 2014/09/10 04:07:51, Takashi Toyoshima (chromium) wrote:
> hum... this change works, but now the variable name is curious.
> Originally, m_options.sysex means that we are going to request a sysex
> permission, and m_sysexEnabled means that the request is permitted by the
> browser process.
> 
> So, maybe the better change is:
> 
> 1. rename m_sysexEnabled to m_requestSysex
> 2. do not update the variable in setSysexEnabled(). now we do not need to hold
> the result since we just reject() here.
> 
> Also, MIDIAccessInitializer::setSysexEnabled(bool enable) is still curious
name
> now. resolveSysexPermission(bool accept) or something like that. But this will
> affect many module. So, I'm ok to keep the function name as is. I may handle
it
> later.

Renamed m_requestSysex and resolveSysexPermission(allowed) respectively.

[Takashi Toyoshima, 2014-09-10 08:30:28]

lgtm on webmidi

https://codereview.chromium.org/547383004/diff/80001/Source/web/WebMIDIPermis...
File Source/web/WebMIDIPermissionRequest.cpp (right):

https://codereview.chromium.org/547383004/diff/80001/Source/web/WebMIDIPermis...
Source/web/WebMIDIPermissionRequest.cpp:55: void
WebMIDIPermissionRequest::setIsAllowed(bool allowed)
Thank you for fixing function names.
Yeah, maybe Blink web API here is a good boundary :P

[haraken, 2014-09-10 08:40:44]

LGTM

[bashi, 2014-09-10 08:41:20]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, tkent@chromium.org

[bashi, 2014-09-10 08:41:21]

Haraken-san, could you take a look at GN/GYP changes? The change is similar to
https://codereview.chromium.org/481453003/. (core -> modules)

Kent-san, could you take a look at web/ ? Changes are renaming only.

https://codereview.chromium.org/547383004/diff/80001/Source/web/WebMIDIPermis...
File Source/web/WebMIDIPermissionRequest.cpp (right):

https://codereview.chromium.org/547383004/diff/80001/Source/web/WebMIDIPermis...
Source/web/WebMIDIPermissionRequest.cpp:55: void
WebMIDIPermissionRequest::setIsAllowed(bool allowed)
On 2014/09/10 08:30:27, Takashi Toyoshima (chromium) wrote:
> Thank you for fixing function names.
> Yeah, maybe Blink web API here is a good boundary :P

Yes, I thought a 3-way patch is required, but isn't:)

[tkent, 2014-09-10 23:44:06]

> MIDIOptions. Renamed m_sysEnabled and setSysexEnabled()

m_sysEnabled -> m_sysexEnabled.

https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:20:
MIDIAccessInitializer::MIDIAccessInitializer(ScriptState* scriptState, const
MIDIOptions* options)
It seems we still assume |options| is non-null.  Can we avoid to change |const
MIDIOptions&| to |const MIDIOptions*|?

[bashi, 2014-09-12 00:19:13]

Thank you for review!

> m_sysEnabled -> m_sysexEnabled.

Done.

https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:20:
MIDIAccessInitializer::MIDIAccessInitializer(ScriptState* scriptState, const
MIDIOptions* options)
On 2014/09/10 23:44:05, tkent (overloaded) wrote:
> It seems we still assume |options| is non-null.  Can we avoid to change |const
> MIDIOptions&| to |const MIDIOptions*|?

IDL dictionaries are garbage collected. We need to change IDL compiler if we
want to change |const MIDIOptions*| to |const MIDIOptions&|, which also changes
all signatures of other constructors and method which use IDL dictionaries.
Other customers may want to hold an IDL dictionary instance in its class. Is it
safe to hold a reference of a GarbageCollected instance?

I'm willing to change the argument type from pointers to references if it's
fine, but I'd like to do it in a follow-up CL because it will make this CL
contain two logically independent changes.

[tkent, 2014-09-12 00:20:54]

https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):

https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIAccessInitializer.cpp:20:
MIDIAccessInitializer::MIDIAccessInitializer(ScriptState* scriptState, const
MIDIOptions* options)
On 2014/09/12 00:19:13, bashi1 wrote:
> On 2014/09/10 23:44:05, tkent (overloaded) wrote:
> > It seems we still assume |options| is non-null.  Can we avoid to change
|const
> > MIDIOptions&| to |const MIDIOptions*|?
> 
> IDL dictionaries are garbage collected. We need to change IDL compiler if we
> want to change |const MIDIOptions*| to |const MIDIOptions&|, which also
changes
> all signatures of other constructors and method which use IDL dictionaries.
> Other customers may want to hold an IDL dictionary instance in its class. Is
it
> safe to hold a reference of a GarbageCollected instance?

It should be safe.

> I'm willing to change the argument type from pointers to references if it's
> fine, but I'd like to do it in a follow-up CL because it will make this CL
> contain two logically independent changes.

It's reasonable.  lgtm.

[bashi, 2014-09-12 00:24:45]

On 2014/09/12 00:20:54, tkent (overloaded) wrote:
>
https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
> File Source/modules/webmidi/MIDIAccessInitializer.cpp (right):
> 
>
https://codereview.chromium.org/547383004/diff/80001/Source/modules/webmidi/M...
> Source/modules/webmidi/MIDIAccessInitializer.cpp:20:
> MIDIAccessInitializer::MIDIAccessInitializer(ScriptState* scriptState, const
> MIDIOptions* options)
> On 2014/09/12 00:19:13, bashi1 wrote:
> > On 2014/09/10 23:44:05, tkent (overloaded) wrote:
> > > It seems we still assume |options| is non-null.  Can we avoid to change
> |const
> > > MIDIOptions&| to |const MIDIOptions*|?
> > 
> > IDL dictionaries are garbage collected. We need to change IDL compiler if we
> > want to change |const MIDIOptions*| to |const MIDIOptions&|, which also
> changes
> > all signatures of other constructors and method which use IDL dictionaries.
> > Other customers may want to hold an IDL dictionary instance in its class. Is
> it
> > safe to hold a reference of a GarbageCollected instance?
> 
> It should be safe.
Thanks. Will create a CL to change pointers to reference when this change is
landed.
> 
> > I'm willing to change the argument type from pointers to references if it's
> > fine, but I'd like to do it in a follow-up CL because it will make this CL
> > contain two logically independent changes.
> 
> It's reasonable.  lgtm.

[bashi, 2014-09-12 00:24:53]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-09-12 00:25:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/547383004/100001

[commit-bot: I haz the power, 2014-09-12 01:17:27]

Committed patchset #6 (id:100001) as 181866