Web MIDI: tentative sysex check for receiving messages

Web MIDI: tentative sysex check for receiving messages

Strict MIDI sequence checker should be introduced and shared between
sender side and receiver side. But, tentatively enable it with minimum
change in receiver side to confirm a browser side change works.

BUG=163795
TEST=LayoutTests/webmidi

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

[Takashi Toyoshima, 2013-09-06 12:50:47]

Hi, can you take a look?

[kouhei (in TOK), 2013-09-09 00:46:51]

Sorry, I would like to avoid this change if possible. Maintaining an reference
to parent is painful, and has to be done very carefully.

The primary problem with this patch is that it creates a reference cycle.
MIDIAccess owns a RefPtr to MIDIInput, and this CL introduces MIDIInput a RefPtr
to MIDIAccess. This means that both objects will never be collected.

My understanding is that the only filtering info that needs to be shared between
MIDIAccess/MIDIInput is sysexenabled flag, so I would recommend just to pass
that onto MIDIInput c-tor instead of carrying the RefPtr.

https://codereview.chromium.org/23648006/diff/1/Source/modules/webmidi/MIDIAc...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/23648006/diff/1/Source/modules/webmidi/MIDIAc...
Source/modules/webmidi/MIDIAccess.cpp:85: // FIXME: Pass in |this| to create()
method so we can filter system exclusive messages correctly.
Should this comment be deleted?

[kouhei (in TOK), 2013-09-09 00:58:37]

It seems that MIDIAccess needs to be alive while MIDIInput is alive, and it was
to be ensured with this patch.

I would suggest doing that by adding a CustomToV8 to set a hidden reference to
MIDIAccess from MIDIInputs. See V8NodeIteratorCustom.cpp for its example.

Also, I don't see a reason for having MIDIPort as an ActiveDOMObject. I think it
can be a simple ScriptWrappable, as it is not directly touched from objects
outside DOM. If you introduce the hidden reference as I explained above,
MIDIPort will be kept alive while MIDIAccess is alive anyway.

[Takashi Toyoshima, 2013-09-12 05:02:44]

Thank you for helping me to handle cyclic dependency issue.
Now the problem was fixed by another CL, and this becomes very simple one.

Also, I'd make another CL to make MIDIPort not inherit ActiveDOMObject.

https://codereview.chromium.org/23648006/diff/1/Source/modules/webmidi/MIDIAc...
File Source/modules/webmidi/MIDIAccess.cpp (right):

https://codereview.chromium.org/23648006/diff/1/Source/modules/webmidi/MIDIAc...
Source/modules/webmidi/MIDIAccess.cpp:85: // FIXME: Pass in |this| to create()
method so we can filter system exclusive messages correctly.
Done in another CL.

[kouhei (in TOK), 2013-09-12 07:02:46]

On 2013/09/12 05:02:44, Takashi Toyoshima (chromium) wrote:
> Thank you for helping me to handle cyclic dependency issue.
> Now the problem was fixed by another CL, and this becomes very simple one.
> 
> Also, I'd make another CL to make MIDIPort not inherit ActiveDOMObject.
> 
>
https://codereview.chromium.org/23648006/diff/1/Source/modules/webmidi/MIDIAc...
> File Source/modules/webmidi/MIDIAccess.cpp (right):
> 
>
https://codereview.chromium.org/23648006/diff/1/Source/modules/webmidi/MIDIAc...
> Source/modules/webmidi/MIDIAccess.cpp:85: // FIXME: Pass in |this| to create()
> method so we can filter system exclusive messages correctly.
> Done in another CL.

lgtm

[commit-bot: I haz the power, 2013-09-12 07:09:36]

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

[commit-bot: I haz the power, 2013-09-12 08:11:04]

Change committed as 157664