Rename CardInfo to MidiDevice and factor out some device probing logic

Rename CardInfo to MidiDevice and factor out some device probing logic

BUG=431489
Committed: https://crrev.com/cc936a29da86d2fe721f3d3211d8a4a3ec7f9d22
Cr-Commit-Position: refs/heads/master@{#320846}

[Adam Goode, 2015-03-15 02:33:08]

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

[Takashi Toyoshima, 2015-03-16 07:22:08]

lgtm with some trivial comments.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:163: void
MidiManagerAlsa::DispatchSendMidiData(MidiManagerClient* client,
Just FYI, I'll factor out this timestamp management logic to MidiManager class
near the future to share it from all platforms. Since as you know, we have the
same code for Windows, and others need this. And to realize MIDIPort.clear(), we
need to modify this a little. See crbug.com/467442.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:226: MidiManagerAlsa::MidiDevice::~MidiDevice()
{
See my comment on the header.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:362: }
Is it better to move (client_id >= 16) check below to here?
I feel doing all client_id related checks into one place makes it easy to
understand.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:379: (current_device < devices.size()) &&
(client_id >= 16)) {
It's better to use a const int for 16 as kMinimumValidClientId for instance.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:394: // TODO(agoode): hash this value.
Can you use crbug.com/467448 for this?

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:505: VLOG(1) << "snd_seq_event_input fails: " <<
snd_strerror(err);
Should we record this unexpected case?
RecordAction() will be able to report how often this happens in the real world.
Also, we may want to set an error flag to return MIDI_INITIALIZATION_ERROR
against incoming new sessions.
TODO is ok in this CL.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:559: VLOG(1) << "snd_midi_event_decoder fails "
<< snd_strerror(count);
Shall we record this too?

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.h (right):

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.h:47: ~MidiDevice();
Can you just remove the destructor?
Since it does not have any implementation.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.h:70: std::string udev_id_id_;
It seems we forgot to add DISALLOW_COPY_AND_ASSIGN for this inner class in the
previous changes.

[Adam Goode, 2015-03-16 20:30:54]

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.cc (right):

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:163: void
MidiManagerAlsa::DispatchSendMidiData(MidiManagerClient* client,
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> Just FYI, I'll factor out this timestamp management logic to MidiManager class
near the future to share it from all platforms. Since as you know, we have the
same code for Windows, and others need this. And to realize MIDIPort.clear(), we
need to modify this a little. See crbug.com/467442.

Thanks.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:362: }
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> Is it better to move (client_id >= 16) check below to here?
> I feel doing all client_id related checks into one place makes it easy to
understand.

No, because we do want clients under 16. We just won't have a card device for
them. Specifically, we get the Midi Through device at client 14 (it's hard coded
to this).

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:379: (current_device < devices.size()) &&
(client_id >= 16)) {
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> It's better to use a const int for 16 as kMinimumValidClientId for instance.

done

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:394: // TODO(agoode): hash this value.
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> Can you use crbug.com/467448 for this?

I've removed the TODO and will track at the bug.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:505: VLOG(1) << "snd_seq_event_input fails: " <<
snd_strerror(err);
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> Should we record this unexpected case?
> RecordAction() will be able to report how often this happens in the real
world.
> Also, we may want to set an error flag to return MIDI_INITIALIZATION_ERROR
against incoming new sessions.
> TODO is ok in this CL.

Great idea. I never thought to use RecordAction.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.cc:559: VLOG(1) << "snd_midi_event_decoder fails "
<< snd_strerror(count);
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> Shall we record this too?

Added TODO. Thanks!

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
File media/midi/midi_manager_alsa.h (right):

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.h:47: ~MidiDevice();
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> Can you just remove the destructor?
> Since it does not have any implementation.

When I do this I get:

In file included from ../../media/midi/midi_manager_alsa.cc:5:
../../media/midi/midi_manager_alsa.h:40:3: error: [chromium-style] Complex
class/struct needs an explicit out-of-line destructor.
  class MidiDevice {
  ^
1 error generated.

https://codereview.chromium.org/1007613006/diff/120001/media/midi/midi_manage...
media/midi/midi_manager_alsa.h:70: std::string udev_id_id_;
On 2015/03/16 at 07:22:08, Takashi Toyoshima (chromium) wrote:
> It seems we forgot to add DISALLOW_COPY_AND_ASSIGN for this inner class in the
previous changes.

done

[Adam Goode, 2015-03-17 01:38:35]

The CQ bit was checked by agoode@chromium.org

[Adam Goode, 2015-03-17 01:38:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
toyoshim@chromium.org
Link to the patchset: https://codereview.chromium.org/1007613006/#ps140001
(title: "Final fixes")

[commit-bot: I haz the power, 2015-03-17 01:38:58]

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

[commit-bot: I haz the power, 2015-03-17 02:17:33]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-17 02:18:29]

Patchset 8 (id:??) landed as
https://crrev.com/cc936a29da86d2fe721f3d3211d8a4a3ec7f9d22
Cr-Commit-Position: refs/heads/master@{#320846}