Introduce MIDIInputMap and MIDIOutputMap.

Introduce MIDIInputMap and MIDIOutputMap.

Currently MIDIAccess.inputs is a function that returns a sequence. With this CL
it will be an attribute of type MIDIInputMap, as specified in the spec.
MIDIAccess.outputs will be an attribute of type MIDIOutputMap in the same
manner. |forEach| is not yet supported.
The existing MIDIAccess.inputs and MIDIAccess.outputs functions cannot
be used with this CL.

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

[yhirano, 2014-08-29 04:35:03]

yhirano@chromium.org changed reviewers:
+ haraken@chromium.org, toyoshim@chromium.org

[yhirano, 2014-08-29 04:37:36]

+toyoshim for modules/webmidi
+haraken for core and bindings

This CL depends on https://codereview.chromium.org/483163003/ .
As in the description, this CL breaks the existing API. I have a CL[1] that
recovers it. toyoshim@, what do you think of it?

1: https://codereview.chromium.org/518633003/

[yhirano, 2014-08-29 05:43:06]

I noticed I could remove custom bindings and I did so (PS5).
Sorry for changing the CL after asking for review.

[haraken, 2014-08-29 06:19:42]

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));

Why can't we just call toV8() instead of V8ValueTraits::toV8Value()?

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.idl (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.idl:17: [ImplementedAs=getPublic,
CallWith=ScriptState] any get(DOMString id);

getPublic => getForBinding

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPortMap.h:21: class MIDIPortMap : public
GarbageCollected<MIDIPortMap<T> > {

Is this map implementation specific to MIDIPort? Or are you planning to use it
in other maps in the future? If so, we want to generalize the implementation and
add this to bindings/.b

[yhirano, 2014-08-29 07:29:13]

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/08/29 06:19:42, haraken wrote:
> 
> Why can't we just call toV8() instead of V8ValueTraits::toV8Value()?

That's because we don't include V8MIDIOutput.h.
I don't quite understand the policy here. Which is more preferable?
 - Include V8MIDIOutput.h and use toV8
 - Do not include V8MIDIOutput.h and use toV8NoInline or
V8ValueTraits::toV8Value

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.idl (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.idl:17: [ImplementedAs=getPublic,
CallWith=ScriptState] any get(DOMString id);
On 2014/08/29 06:19:42, haraken wrote:
> 
> getPublic => getForBinding

Done.

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIPortMap.h:21: class MIDIPortMap : public
GarbageCollected<MIDIPortMap<T> > {
On 2014/08/29 06:19:42, haraken wrote:
> 
> Is this map implementation specific to MIDIPort? Or are you planning to use it
> in other maps in the future? If so, we want to generalize the implementation
and
> add this to bindings/.b

This implementation relies on the entries immutability. Otherwise, we should
take care of the case that container is modified after some iterators are
issued. The current WebMIDI meets the condition, but I don't know it is general
or not.
I would like the class be in modules/webmidi for now. If another module uses a
similar class, definitely it should be in bindings, but I don't know the other
modules' situation.
The WebIDL draft has MapClass attribute, but the editor wants to delete it (see
[1]). Once a proper abstraction is introduced in the WebIDL spec, we can define
it in bindings/ and use it from core/ and modules/.

1: https://github.com/WebAudio/web-midi-api/issues/91

[yhirano, 2014-08-29 07:31:50]

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/08/29 07:29:13, yhirano wrote:
> On 2014/08/29 06:19:42, haraken wrote:
> > 
> > Why can't we just call toV8() instead of V8ValueTraits::toV8Value()?
> 
> That's because we don't include V8MIDIOutput.h.
> I don't quite understand the policy here. Which is more preferable?
>  - Include V8MIDIOutput.h and use toV8
>  - Do not include V8MIDIOutput.h and use toV8NoInline or
> V8ValueTraits::toV8Value

Sorry for the poor English. I meant "Because we don't include...".

[haraken, 2014-08-31 07:18:57]

toyoshima-san: Are you fine with this change overall?

I'm happy to review the detailed implementation, but I'd like to first confirm
that this is an intended change from the perspective of WebMIDI.

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/08/29 07:29:13, yhirano wrote:
> On 2014/08/29 06:19:42, haraken wrote:
> > 
> > Why can't we just call toV8() instead of V8ValueTraits::toV8Value()?
> 
> That's because we don't include V8MIDIOutput.h.
> I don't quite understand the policy here. Which is more preferable?
>  - Include V8MIDIOutput.h and use toV8
>  - Do not include V8MIDIOutput.h and use toV8NoInline or
> V8ValueTraits::toV8Value

It's confusing to have multiple ways to convert a native value to a V8 value.
Probably can we replace toV8NoInline with V8ValueTraits::toV8Value? I mean, is
it possible to deprecate toV8NoInline by adopting the following rules?

- Use toV8 if you cannot include V8X.h.
- Use V8ValueTraits::toV8Value if you cannot include V8X.h (this should be rare
cases)

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:84: // We currently assume no ports will
be added once an MIDIAccess object is

will be added once => are added after

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:93: // created.

will be added once => are added after

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIInput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));

You can add MIDIInputMap.cpp and use toV8.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.idl (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.idl:5: // callback ForEachCallback =
void(DOMString id, MIDIInput port);

Add a FIXME about this.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));

You can add MIDIOutputMap.cpp and use toV8.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutputMap.idl (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutputMap.idl:5: // callback ForEachCallback =
void(DOMString id, MIDIOutput port);

Add a FIXME about this.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:79:
exceptionState.throwDOMException(InvalidModificationError, "Not implemented");

Just help me understand: Is there any way for user's script to reach this code?

If yes, I think it's problematic. If no, can we change this to
ASSERT_NOT_REACHED()?

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:91: const Member<const MIDIPortMap<T> >
m_map;

Why do we need this? trace(m_entries) in MIDIPortMap is not enough to keep the
map alive?

[yhirano, 2014-09-01 02:22:11]

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/08/31 07:18:57, haraken wrote:
> On 2014/08/29 07:29:13, yhirano wrote:
> > On 2014/08/29 06:19:42, haraken wrote:
> > > 
> > > Why can't we just call toV8() instead of V8ValueTraits::toV8Value()?
> > 
> > That's because we don't include V8MIDIOutput.h.
> > I don't quite understand the policy here. Which is more preferable?
> >  - Include V8MIDIOutput.h and use toV8
> >  - Do not include V8MIDIOutput.h and use toV8NoInline or
> > V8ValueTraits::toV8Value
> 
> It's confusing to have multiple ways to convert a native value to a V8 value.
> Probably can we replace toV8NoInline with V8ValueTraits::toV8Value? I mean, is
> it possible to deprecate toV8NoInline by adopting the following rules?
> 
> - Use toV8 if you cannot include V8X.h.
> - Use V8ValueTraits::toV8Value if you cannot include V8X.h (this should be
rare
> cases)

OK, I understand including bindings/{core, modules}/v8/V8*.h is fine. I have a
question (note related to this CL): Can we includes bindings/{core,
modules}/v8/custom/V8*Custom.h as well?

> Is it possible to deprecate toV8NoInline by adopting the following rules?
V8ValueTraits::toV8Value uses toV8NoInline, but we could add a "not to use
toV8NoInline" comment.
I added a comment at V8Binding.h. What do you think about it?

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:84: // We currently assume no ports will
be added once an MIDIAccess object is
On 2014/08/31 07:18:57, haraken wrote:
> 
> will be added once => are added after

Done.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:93: // created.
On 2014/08/31 07:18:57, haraken wrote:
> 
> will be added once => are added after

Done.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIInput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/08/31 07:18:57, haraken wrote:
> 
> You can add MIDIInputMap.cpp and use toV8.

Done.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.idl (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.idl:5: // callback ForEachCallback =
void(DOMString id, MIDIInput port);
On 2014/08/31 07:18:57, haraken wrote:
> 
> Add a FIXME about this.

Done.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/08/31 07:18:57, haraken wrote:
> 
> You can add MIDIOutputMap.cpp and use toV8.

Done.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutputMap.idl (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutputMap.idl:5: // callback ForEachCallback =
void(DOMString id, MIDIOutput port);
On 2014/08/31 07:18:57, haraken wrote:
> 
> Add a FIXME about this.

Done.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:79:
exceptionState.throwDOMException(InvalidModificationError, "Not implemented");
On 2014/08/31 07:18:57, haraken wrote:
> 
> Just help me understand: Is there any way for user's script to reach this
code?
> 
> If yes, I think it's problematic. If no, can we change this to
> ASSERT_NOT_REACHED()?

Yes, it can be called from a script. See [1]. It is intended to used with
yield[2], but we can call it manually.

1: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-iteratornext
2:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-generator-function-...

I don't understand why it is problematic: can you explain?

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:91: const Member<const MIDIPortMap<T> >
m_map;
On 2014/08/31 07:18:57, haraken wrote:
> 
> Why do we need this? trace(m_entries) in MIDIPortMap is not enough to keep the
> map alive?

Scripts can hold iterators independently. i.e. you can write a script like this:

 // |map| is backed by MIDIPortMap.
 var i = map[Symbol.iterator]();
 map = undefined;

Here we must not release the MIDIPortMap::m_entries because the iterator uses it
while it is not reachable via |map|.

[haraken, 2014-09-01 07:17:48]

Mostly looks good. I'll take another close look at the implementation after
waiting for the approval of toyoshima-san.

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/09/01 02:22:10, yhirano wrote:
> On 2014/08/31 07:18:57, haraken wrote:
> > On 2014/08/29 07:29:13, yhirano wrote:
> > > On 2014/08/29 06:19:42, haraken wrote:
> > > > 
> > > > Why can't we just call toV8() instead of V8ValueTraits::toV8Value()?
> > > 
> > > That's because we don't include V8MIDIOutput.h.
> > > I don't quite understand the policy here. Which is more preferable?
> > >  - Include V8MIDIOutput.h and use toV8
> > >  - Do not include V8MIDIOutput.h and use toV8NoInline or
> > > V8ValueTraits::toV8Value
> > 
> > It's confusing to have multiple ways to convert a native value to a V8
value.
> > Probably can we replace toV8NoInline with V8ValueTraits::toV8Value? I mean,
is
> > it possible to deprecate toV8NoInline by adopting the following rules?
> > 
> > - Use toV8 if you cannot include V8X.h.
> > - Use V8ValueTraits::toV8Value if you cannot include V8X.h (this should be
> rare
> > cases)
> 
> OK, I understand including bindings/{core, modules}/v8/V8*.h is fine. I have a
> question (note related to this CL): Can we includes bindings/{core,
> modules}/v8/custom/V8*Custom.h as well?

That's technically possible, but in what cases do you want to do that?

> > Is it possible to deprecate toV8NoInline by adopting the following rules?
> V8ValueTraits::toV8Value uses toV8NoInline, but we could add a "not to use
> toV8NoInline" comment.
> I added a comment at V8Binding.h. What do you think about it?

Looks good.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:79:
exceptionState.throwDOMException(InvalidModificationError, "Not implemented");
On 2014/09/01 02:22:11, yhirano wrote:
> On 2014/08/31 07:18:57, haraken wrote:
> > 
> > Just help me understand: Is there any way for user's script to reach this
> code?
> > 
> > If yes, I think it's problematic. If no, can we change this to
> > ASSERT_NOT_REACHED()?
> 
> Yes, it can be called from a script. See [1]. It is intended to used with
> yield[2], but we can call it manually.
> 
> 1: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-iteratornext
> 2:
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-generator-function-...
> 
> I don't understand why it is problematic: can you explain?

I just thought it would be problematic to expose "Not implemented" features to
outside. Wouldn't it be a concern for WebMIDI?

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:91: const Member<const MIDIPortMap<T> >
m_map;
On 2014/09/01 02:22:11, yhirano wrote:
> On 2014/08/31 07:18:57, haraken wrote:
> > 
> > Why do we need this? trace(m_entries) in MIDIPortMap is not enough to keep
the
> > map alive?
> 
> Scripts can hold iterators independently. i.e. you can write a script like
this:
> 
>  // |map| is backed by MIDIPortMap.
>  var i = map[Symbol.iterator]();
>  map = undefined;
> 
> Here we must not release the MIDIPortMap::m_entries because the iterator uses
it
> while it is not reachable via |map|.

Makes sense. A comment about it will be helpful.

// The map needs to be kept alive while JavaScript holds the iterator to the
map.

[yhirano, 2014-09-01 07:31:35]

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
V8ValueTraits<MIDIOutput*>::toV8Value(result, scriptState->context()->Global(),
scriptState->isolate()));
On 2014/09/01 07:17:47, haraken wrote:
> > OK, I understand including bindings/{core, modules}/v8/V8*.h is fine. I have
a
> > question (note related to this CL): Can we includes bindings/{core,
> > modules}/v8/custom/V8*Custom.h as well?
> 
> That's technically possible, but in what cases do you want to do that?

I am reviewing a CL doing it and I want to understand the policy clearly:
https://codereview.chromium.org/516603004/diff/60001/Source/modules/servicewo...

[haraken, 2014-09-01 07:35:38]

On 2014/09/01 07:31:35, yhirano wrote:
>
https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
> File Source/modules/webmidi/MIDIOutputMap.h (right):
> 
>
https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
> Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
> V8ValueTraits<MIDIOutput*>::toV8Value(result,
scriptState->context()->Global(),
> scriptState->isolate()));
> On 2014/09/01 07:17:47, haraken wrote:
> > > OK, I understand including bindings/{core, modules}/v8/V8*.h is fine. I
have
> a
> > > question (note related to this CL): Can we includes bindings/{core,
> > > modules}/v8/custom/V8*Custom.h as well?
> > 
> > That's technically possible, but in what cases do you want to do that?
> 
> I am reviewing a CL doing it and I want to understand the policy clearly:
>
https://codereview.chromium.org/516603004/diff/60001/Source/modules/servicewo...

Ah, got it. ArrayBuffer is special and it's ok to include it at the moment. In
fact, shiino-san is planning to remove the custom binding for ArrayBuffer in the
near future.

The policy is:

- There is no problem in inculding bindings/{core,
modules}/v8/custom/V8*Custom.h.
- However, you don't need to do that in most cases except for a couple of cases
like ArrayBuffer.

[yhirano, 2014-09-01 08:15:15]

On 2014/09/01 07:35:38, haraken wrote:
> On 2014/09/01 07:31:35, yhirano wrote:
> >
>
https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
> > File Source/modules/webmidi/MIDIOutputMap.h (right):
> > 
> >
>
https://codereview.chromium.org/513203002/diff/80001/Source/modules/webmidi/M...
> > Source/modules/webmidi/MIDIOutputMap.h:24: return ScriptValue(scriptState,
> > V8ValueTraits<MIDIOutput*>::toV8Value(result,
> scriptState->context()->Global(),
> > scriptState->isolate()));
> > On 2014/09/01 07:17:47, haraken wrote:
> > > > OK, I understand including bindings/{core, modules}/v8/V8*.h is fine. I
> have
> > a
> > > > question (note related to this CL): Can we includes bindings/{core,
> > > > modules}/v8/custom/V8*Custom.h as well?
> > > 
> > > That's technically possible, but in what cases do you want to do that?
> > 
> > I am reviewing a CL doing it and I want to understand the policy clearly:
> >
>
https://codereview.chromium.org/516603004/diff/60001/Source/modules/servicewo...
> 
> Ah, got it. ArrayBuffer is special and it's ok to include it at the moment. In
> fact, shiino-san is planning to remove the custom binding for ArrayBuffer in
the
> near future.
> 
> The policy is:
> 
> - There is no problem in inculding bindings/{core,
> modules}/v8/custom/V8*Custom.h.
> - However, you don't need to do that in most cases except for a couple of
cases
> like ArrayBuffer.

Thank you!

[yhirano, 2014-09-01 11:32:52]

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:79:
exceptionState.throwDOMException(InvalidModificationError, "Not implemented");
On 2014/09/01 07:17:48, haraken wrote:
> On 2014/09/01 02:22:11, yhirano wrote:
> > On 2014/08/31 07:18:57, haraken wrote:
> > > 
> > > Just help me understand: Is there any way for user's script to reach this
> > code?
> > > 
> > > If yes, I think it's problematic. If no, can we change this to
> > > ASSERT_NOT_REACHED()?
> > 
> > Yes, it can be called from a script. See [1]. It is intended to used with
> > yield[2], but we can call it manually.
> > 
> > 1: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-iteratornext
> > 2:
> >
>
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-generator-function-...
> > 
> > I don't understand why it is problematic: can you explain?
> 
> I just thought it would be problematic to expose "Not implemented" features to
> outside. Wouldn't it be a concern for WebMIDI?

I reread the spec and found ignoring the parameter was the desired behavior
here. Fixed.

https://codereview.chromium.org/513203002/diff/100001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:91: const Member<const MIDIPortMap<T> >
m_map;
On 2014/09/01 07:17:48, haraken wrote:
> On 2014/09/01 02:22:11, yhirano wrote:
> > On 2014/08/31 07:18:57, haraken wrote:
> > > 
> > > Why do we need this? trace(m_entries) in MIDIPortMap is not enough to keep
> the
> > > map alive?
> > 
> > Scripts can hold iterators independently. i.e. you can write a script like
> this:
> > 
> >  // |map| is backed by MIDIPortMap.
> >  var i = map[Symbol.iterator]();
> >  map = undefined;
> > 
> > Here we must not release the MIDIPortMap::m_entries because the iterator
uses
> it
> > while it is not reachable via |map|.
> 
> Makes sense. A comment about it will be helpful.
> 
> // The map needs to be kept alive while JavaScript holds the iterator to the
> map.

Done.

[Takashi Toyoshima, 2014-09-01 13:48:39]

Thank you.

Haraken: Yes, this is an API change that we can not avoid. But this should be
the last compatibility breakage. Yutaka prepared a CL to support legacy
interface together as https://codereview.chromium.org/518633003/. If we can have
it while 1 or 2 milestones, it will be nice because we have enough time to
advertise on this API update. But this is a feature behind a flag. Take it easy
if it is bothering.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:84: // We currently assume no ports are
added after an MIDIAccess object is
Is there any real problem on adding a new port?
Let's chat on this offline to update device list dynamically.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.idl (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.idl:5: // FIXME: Implement forEach.
Agreed to postpone forEach() this time. Since the IDL will be changed to have
'this' as a second argument later.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.idl:14: Iterator keys();
Aha. Iterator can bring an argument of any type.
So, is it better to fix Web MIDI spec's IDL to return Iterator?
Now the spec uses MIDIKeyIterator and so on.
http://webaudio.github.io/web-midi-api/#dfn-midiinputmap

[Takashi Toyoshima, 2014-09-01 13:49:56]

LG in terms of Web MIDI. Defer to binding owners.

[haraken, 2014-09-02 01:20:40]

LGTM

https://codereview.chromium.org/513203002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:213: // internally. If you want to convert a
C++ value to a JS value without

JS value => V8 value

https://codereview.chromium.org/513203002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:218: // more: You can handle value
conversion generally with it.

Slightly more concise:

// toV8NoInline is intended to be used internally. If you want to convert a DOM
object to a V8 value:
//
// - Use toV8() if you can include V8X.h.
// - Use V8ValueTraits<T>::toV8Value if you cannot include V8X.h.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.h:18: MIDIInputMap(const HeapHashMap<String,
Member<MIDIInput> >map)

Add explicit.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.h:19: : MIDIPortMap<MIDIInput>(map) {
ScriptWrappable::init(this); }

Nit: Blink normally writes '{ ... }' in separate lines if you cannot write all
of the method definitions in one line.

  void foo() { return m_foo; }

or

  void foo()
  {
    return m_foo;
  }

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutputMap.h:18: MIDIOutputMap(HeapHashMap<String,
Member<MIDIOutput> > map)

Add explicit.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:66: , m_end(end) { }

Nit: Move { and } to the next lines.

[yhirano, 2014-09-02 04:43:06]

https://codereview.chromium.org/513203002/diff/180001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:213: // internally. If you want to convert a
C++ value to a JS value without
On 2014/09/02 01:20:40, haraken wrote:
> 
> JS value => V8 value

Done.

https://codereview.chromium.org/513203002/diff/180001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:218: // more: You can handle value
conversion generally with it.
On 2014/09/02 01:20:40, haraken wrote:
> 
> Slightly more concise:
> 
> // toV8NoInline is intended to be used internally. If you want to convert a
DOM
> object to a V8 value:
> //
> // - Use toV8() if you can include V8X.h.
> // - Use V8ValueTraits<T>::toV8Value if you cannot include V8X.h.

Done.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.h:18: MIDIInputMap(const HeapHashMap<String,
Member<MIDIInput> >map)
On 2014/09/02 01:20:40, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.h:19: : MIDIPortMap<MIDIInput>(map) {
ScriptWrappable::init(this); }
On 2014/09/02 01:20:40, haraken wrote:
> 
> Nit: Blink normally writes '{ ... }' in separate lines if you cannot write all
> of the method definitions in one line.
> 
>   void foo() { return m_foo; }
> 
> or
> 
>   void foo()
>   {
>     return m_foo;
>   }
Done (move the implementation to the cpp file).

[yhirano, 2014-09-02 05:17:13]

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIAccess.cpp:84: // We currently assume no ports are
added after an MIDIAccess object is
On 2014/09/01 13:48:39, Takashi Toyoshima (chromium) wrote:
> Is there any real problem on adding a new port?
> Let's chat on this offline to update device list dynamically.

Discussed offline: We decided to delete this assertion and make MIDIAccess
return a new MIDI{Input, Output}Map for each call. Done.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIInputMap.idl (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIInputMap.idl:14: Iterator keys();
On 2014/09/01 13:48:39, Takashi Toyoshima (chromium) wrote:
> Aha. Iterator can bring an argument of any type.
> So, is it better to fix Web MIDI spec's IDL to return Iterator?
> Now the spec uses MIDIKeyIterator and so on.
> http://webaudio.github.io/web-midi-api/#dfn-midiinputmap

"Iterator" is not a standard name (it is not exposed to Window). I hope the
WebIDL spec will invent proper names for iterator / itertable interfaces. Can we
leave them be and wait?

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIOutputMap.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIOutputMap.h:18: MIDIOutputMap(HeapHashMap<String,
Member<MIDIOutput> > map)
On 2014/09/02 01:20:40, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
File Source/modules/webmidi/MIDIPortMap.h (right):

https://codereview.chromium.org/513203002/diff/180001/Source/modules/webmidi/...
Source/modules/webmidi/MIDIPortMap.h:66: , m_end(end) { }
On 2014/09/02 01:20:40, haraken wrote:
> 
> Nit: Move { and } to the next lines.

Done.

[haraken, 2014-09-02 08:17:08]

Also this change will need an intent-to-ship and API review.

[Takashi Toyoshima, 2014-09-02 08:22:59]

Haraken: Is it true even for features behind a flag?

[haraken, 2014-09-02 08:29:22]

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

[haraken, 2014-09-02 08:29:23]

On 2014/09/02 08:22:59, Takashi Toyoshima (chromium) wrote:
> Haraken: Is it true even for features behind a flag?

In my understanding, intent-to-implement would be needed for non-trivial
features behind a flag (I'm not sure if this feature is trivial or not, but I
think you're taking a somewhat strange API deprecation step). At least, this
needs reviews from API owners.

+tkent-san

[tkent, 2014-09-02 08:43:39]

Web MIDI is not shipped yet.  You don't need to involve API owners.

[yhirano, 2014-09-05 03:52:22]

toyoshim@: Do you have other comments?

[Takashi Toyoshima, 2014-09-05 05:39:40]

That's fine enough.
LGTM.

[yhirano, 2014-09-05 05:52:30]

Thanks!

[yhirano, 2014-09-05 05:52:39]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2014-09-05 05:53:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yhirano@chromium.org/513203002/240001

[commit-bot: I haz the power, 2014-09-05 06:44:06]

Committed patchset #14 (id:240001) as 181447

[sof, 2014-09-07 20:02:28]

Looks like this added an Oilpan crash (webmidi/send_messages.html),


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...

https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_oilpan...

[yhirano, 2014-09-08 02:20:50]

On 2014/09/07 20:02:28, sof wrote:
> Looks like this added an Oilpan crash (webmidi/send_messages.html),
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Oil...
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_oilpan...

Thanks, filed a bug: https://code.google.com/p/chromium/issues/detail?id=411742