[WebMIDI] [Android] Set appropriate port properties.

[WebMIDI] [Android] Set appropriate port properties.

A WebMIDI port has properties such as .manufacturer, .name and .version
that remain empty in the current WebMIDI implementation on Android.
This CL fixes that by setting them to values got from USB descriptors.

BUG=473639
Committed: https://crrev.com/19dea16b4492c29f586bc4bff89854d3c2829d7c
Cr-Commit-Position: refs/heads/master@{#326747}

[yhirano, 2015-04-22 02:06:30]

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

[Takashi Toyoshima, 2015-04-22 05:25:13]

lg with minor comments.

Also, my CL under review will replace the Java code directory. Let's be careful
for merge errors.

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:150: std::string version =
Can you add a comment as the endian is converted in
UsbMidiDescriptorParser::ParseDevice()? Or it may be enough that
GetDeviceVersion() returns a string so that this class does not need to care for
what's BCD.

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:157: // sufficiently unique although there is no
user-friendly meaning.
Can we use SHA256 from crypto/sha2.h to hash this "port-%d-%ld" string?
ALSA port already uses it and it's nice to provide consistent format ID over the
platforms. I'd gradually change other platforms to be consistent.

https://codereview.chromium.org/1098913003/diff/140001/media/midi/usb_midi_de...
File media/midi/usb_midi_descriptor_parser.cc (right):

https://codereview.chromium.org/1098913003/diff/140001/media/midi/usb_midi_de...
media/midi/usb_midi_descriptor_parser.cc:101: *info = DeviceInfo();
Shall we initialize *info always at the beginning of this function instead of
here and at line 105 just in case?

For now, behaviors are not consistent, e.g., code paths at line 90, and 95 do
not initialize *info at all for error cases, but line 105 does.

Or, we can just remove line 101 and 105 since caller must create it via
constructor with right initialization.

[yhirano, 2015-04-22 06:11:09]

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:150: std::string version =
On 2015/04/22 05:25:13, Takashi Toyoshima (chromium) wrote:
> Can you add a comment as the endian is converted in
> UsbMidiDescriptorParser::ParseDevice()? Or it may be enough that
> GetDeviceVersion() returns a string so that this class does not need to care
for
> what's BCD.

Done.

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:157: // sufficiently unique although there is no
user-friendly meaning.
On 2015/04/22 05:25:13, Takashi Toyoshima (chromium) wrote:
> Can we use SHA256 from crypto/sha2.h to hash this "port-%d-%ld" string?
> ALSA port already uses it and it's nice to provide consistent format ID over
the
> platforms. I'd gradually change other platforms to be consistent.

Should I care about hash collision?

https://codereview.chromium.org/1098913003/diff/140001/media/midi/usb_midi_de...
File media/midi/usb_midi_descriptor_parser.cc (right):

https://codereview.chromium.org/1098913003/diff/140001/media/midi/usb_midi_de...
media/midi/usb_midi_descriptor_parser.cc:101: *info = DeviceInfo();
On 2015/04/22 05:25:13, Takashi Toyoshima (chromium) wrote:
> Shall we initialize *info always at the beginning of this function instead of
> here and at line 105 just in case?
> 
> For now, behaviors are not consistent, e.g., code paths at line 90, and 95 do
> not initialize *info at all for error cases, but line 105 does.
> 
> Or, we can just remove line 101 and 105 since caller must create it via
> constructor with right initialization.

You're right, thanks. Done.

[Takashi Toyoshima, 2015-04-22 06:52:27]

lgtm on midi/ if you split the hash change to another CL.

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:157: // sufficiently unique although there is no
user-friendly meaning.
I think we can safely ignore it.
Also, it would be better to concat other factors, e.g. manufacturer,
product_name, and so on.

Also, it's fine to split the hash change to another CL.

[yhirano, 2015-04-22 08:07:42]

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
File media/midi/midi_manager_usb.cc (right):

https://codereview.chromium.org/1098913003/diff/140001/media/midi/midi_manage...
media/midi/midi_manager_usb.cc:157: // sufficiently unique although there is no
user-friendly meaning.
On 2015/04/22 06:52:27, Takashi Toyoshima (chromium) wrote:
> I think we can safely ignore it.
> Also, it would be better to concat other factors, e.g. manufacturer,
> product_name, and so on.
> 
> Also, it's fine to split the hash change to another CL.

Added TODO.

[yhirano, 2015-04-22 08:08:18]

yhirano@chromium.org changed reviewers:
+ qinmin@chromium.org

[yhirano, 2015-04-22 08:08:19]

+qinmin for UsbMidiDeviceAndroid.java.

[qinmin, 2015-04-22 17:36:12]

https://codereview.chromium.org/1098913003/diff/240001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/UsbMidiDeviceAndroid.java
(right):

https://codereview.chromium.org/1098913003/diff/240001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/UsbMidiDeviceAndroid.java:86: static
final int STRING_DESCRIPTOR_TYPE = 0x03;
Since we always do a <<8 on this, why not make this 0x0300 to get rid of the <<
operator?

https://codereview.chromium.org/1098913003/diff/240001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/UsbMidiDeviceAndroid.java:273: *
@param index
be more specific. 
"@param index Index of the descriptor"

[yhirano, 2015-04-23 02:36:12]

https://codereview.chromium.org/1098913003/diff/240001/media/midi/java/src/or...
File media/midi/java/src/org/chromium/media/midi/UsbMidiDeviceAndroid.java
(right):

https://codereview.chromium.org/1098913003/diff/240001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/UsbMidiDeviceAndroid.java:86: static
final int STRING_DESCRIPTOR_TYPE = 0x03;
On 2015/04/22 17:36:12, qinmin wrote:
> Since we always do a <<8 on this, why not make this 0x0300 to get rid of the
<<
> operator?

The value at L284 is composed as ((type << 8) | index). type itself has its own
meaning (the type of string descriptor), but (type << 8) does not. So I would
like to define the type.

https://codereview.chromium.org/1098913003/diff/240001/media/midi/java/src/or...
media/midi/java/src/org/chromium/media/midi/UsbMidiDeviceAndroid.java:273: *
@param index
On 2015/04/22 17:36:12, qinmin wrote:
> be more specific. 
> "@param index Index of the descriptor"

Done.

[qinmin, 2015-04-23 16:28:41]

lgtm

[Takashi Toyoshima, 2015-04-24 04:16:08]

still lgtm.
Also, my CL to replace the directory was reverted. So, please submit this
firstly with another rebase to follow the revert.

[yhirano, 2015-04-24 06:02:42]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-04-24 06:02:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1098913003/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-24 06:02:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1098913003/280001

[commit-bot: I haz the power, 2015-04-24 06:06:31]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-24 06:07:26]

Patchset 15 (id:??) landed as
https://crrev.com/19dea16b4492c29f586bc4bff89854d3c2829d7c
Cr-Commit-Position: refs/heads/master@{#326747}