Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(839)

Issue 261263002: Web MIDI: add an unit test to check MidiManager instantiation (Closed)

Created:
6 years, 7 months ago by Takashi Toyoshima
Modified:
6 years, 7 months ago
Reviewers:
yukawa, yhirano
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Web MIDI: add an unit test to check MidiManager instantiation Add an unit test to check if MidiManager can be created and returns MIDI_OK at starting a session on each platform. To make the design and test simple, now MidiManager is changed to invoke CompleteStartSession() always on the thread that calls StartSession(). BUG=344759 TEST=media_unittests --gtest_filter='*Midi*' Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268533

Patch Set 1 #

Patch Set 2 : comment #

Patch Set 3 : midi_manager_usb_unittest #

Patch Set 4 : clean up for review #

Total comments: 11

Patch Set 5 : review #2 #

Patch Set 6 : fix test errors #

Total comments: 2

Patch Set 7 : comment update #

Total comments: 7

Patch Set 8 : review #5 #

Total comments: 8

Patch Set 9 : const nits #

Patch Set 10 : win build fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+151 lines, -99 lines) Patch
M media/midi/midi_manager.h View 1 2 3 4 5 6 7 8 9 6 chunks +44 lines, -13 lines 0 comments Download
M media/midi/midi_manager.cc View 1 2 3 4 7 chunks +37 lines, -21 lines 0 comments Download
M media/midi/midi_manager_unittest.cc View 1 2 3 4 5 6 7 8 8 chunks +50 lines, -54 lines 0 comments Download
M media/midi/midi_manager_usb_unittest.cc View 1 2 3 4 5 10 chunks +20 lines, -11 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
Takashi Toyoshima
Hi OWNERS, can you take a look?
6 years, 7 months ago (2014-05-04 22:59:50 UTC) #1
yukawa
https://codereview.chromium.org/261263002/diff/50001/media/midi/midi_manager.cc File media/midi/midi_manager.cc (right): https://codereview.chromium.org/261263002/diff/50001/media/midi/midi_manager.cc#newcode96 media/midi/midi_manager.cc:96: CompleteInitializationInternal(result); Do we intentionally call CompleteInitializationInternal again here? Sorry ...
6 years, 7 months ago (2014-05-04 23:16:01 UTC) #2
Takashi Toyoshima
Done. PTAL. https://codereview.chromium.org/261263002/diff/50001/media/midi/midi_manager.cc File media/midi/midi_manager.cc (right): https://codereview.chromium.org/261263002/diff/50001/media/midi/midi_manager.cc#newcode96 media/midi/midi_manager.cc:96: CompleteInitializationInternal(result); Oops, I removed it once, but ...
6 years, 7 months ago (2014-05-05 00:45:08 UTC) #3
Takashi Toyoshima
One nit is fixed at Patch Set 7. https://codereview.chromium.org/261263002/diff/110001/media/midi/midi_manager.h File media/midi/midi_manager.h (right): https://codereview.chromium.org/261263002/diff/110001/media/midi/midi_manager.h#newcode162 media/midi/midi_manager.h:162: // ...
6 years, 7 months ago (2014-05-05 00:49:02 UTC) #4
yukawa
https://codereview.chromium.org/261263002/diff/110001/media/midi/midi_manager.h File media/midi/midi_manager.h (right): https://codereview.chromium.org/261263002/diff/110001/media/midi/midi_manager.h#newcode110 media/midi/midi_manager.h:110: virtual void StartInitialization(); It would be nice if the ...
6 years, 7 months ago (2014-05-05 04:07:36 UTC) #5
Takashi Toyoshima
Addressed. PTAL again. https://codereview.chromium.org/261263002/diff/110001/media/midi/midi_manager.h File media/midi/midi_manager.h (right): https://codereview.chromium.org/261263002/diff/110001/media/midi/midi_manager.h#newcode110 media/midi/midi_manager.h:110: virtual void StartInitialization(); On 2014/05/05 04:07:36, ...
6 years, 7 months ago (2014-05-06 01:58:17 UTC) #6
yukawa
LGTM with minor nits. https://codereview.chromium.org/261263002/diff/120001/media/midi/midi_manager.h File media/midi/midi_manager.h (right): https://codereview.chromium.org/261263002/diff/120001/media/midi/midi_manager.h#newcode140 media/midi/midi_manager.h:140: size_t get_clients_size_for_testing() { return clients_.size(); ...
6 years, 7 months ago (2014-05-06 02:46:02 UTC) #7
Takashi Toyoshima
https://codereview.chromium.org/261263002/diff/120001/media/midi/midi_manager.h File media/midi/midi_manager.h (right): https://codereview.chromium.org/261263002/diff/120001/media/midi/midi_manager.h#newcode140 media/midi/midi_manager.h:140: size_t get_clients_size_for_testing() { return clients_.size(); } On 2014/05/06 02:46:02, ...
6 years, 7 months ago (2014-05-06 11:11:52 UTC) #8
Takashi Toyoshima
The CQ bit was checked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-06 11:12:18 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/261263002/140001
6 years, 7 months ago (2014-05-06 11:12:54 UTC) #10
Takashi Toyoshima
Oops... _win.cc accesses to input_ports_ and output_ports_.
6 years, 7 months ago (2014-05-06 12:56:48 UTC) #11
Takashi Toyoshima
Yukawa: I moved them from private to protected tentatively, and added TODO for them. I'll ...
6 years, 7 months ago (2014-05-06 13:06:14 UTC) #12
Takashi Toyoshima
The CQ bit was checked by toyoshim@chromium.org
6 years, 7 months ago (2014-05-06 13:06:19 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/261263002/160001
6 years, 7 months ago (2014-05-06 13:06:32 UTC) #14
yukawa
On 2014/05/06 13:06:14, Takashi Toyoshima (chromium) wrote: > Yukawa: I moved them from private to ...
6 years, 7 months ago (2014-05-06 14:12:27 UTC) #15
commit-bot: I haz the power
6 years, 7 months ago (2014-05-06 16:39:23 UTC) #16
Message was sent while issue was closed.
Change committed as 268533

Powered by Google App Engine
This is Rietveld 408576698