Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701503005:
Web MIDI: device open/close for dynamic manager instantiation on Windows (Closed)
|
Created:
3 years, 10 months ago by Takashi Toyoshima Modified:
3 years, 10 months ago Reviewers:
yhirano CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: device open/close for dynamic manager instantiation on Windows
This patch implements functionalities to open and close devices.
Closing operations will be performed outside the manager instance
so that the manager does not block I/O thread. But it would block
next instance's initialization correctly.
BUG=497573, 617086, 672793
Review-Url: https://codereview.chromium.org/2701503005
Cr-Commit-Position: refs/heads/master@{#452786}
Committed: https://chromium.googlesource.com/chromium/src/+/e262421e78d33170893fd1985df28658ba45a811
Patch Set 1 : . #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : cleanup #Patch Set 4 : incl disconnect #Patch Set 5 : build fix #
Total comments: 3
Patch Set 6 : add clear comments on threading #
Total comments: 7
Patch Set 7 : explicit handle reset #Messages
Total messages: 33 (22 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Web MIDI: device open/close for dynamic manager instantiation on Windows This patch implements functionalities to open and close devices. Closing operations will be performed outside the manager instance so that the manager does not block I/O thread. But it would block next instance's initialization correctly. BUG=497573, 617086, 672793 ========== to ========== Web MIDI: device open/close for dynamic manager instantiation on Windows This patch implements functionalities to open and close devices. Closing operations will be performed outside the manager instance so that the manager does not block I/O thread. But it would block next instance's initialization correctly. BUG=497573, 617086, 672793 ==========
toyoshim@chromium.org changed reviewers: + yhirano@chromium.org
ptal https://codereview.chromium.org/2701503005/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (left): https://codereview.chromium.org/2701503005/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:237: // Port Overrides: I forgot to remove this stale line. https://codereview.chromium.org/2701503005/diff/40001/media/midi/dynamically_... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701503005/diff/40001/media/midi/dynamically_... media/midi/dynamically_initialized_midi_manager_win.cc:302: bool Connect() override { Connect() method is just replaced here so that we can place all override methods after other methods.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
oops, it seems I ran ninja without saving patch set 4 changes. ptal pastch set 5 that fixes build errors.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2701503005/diff/100001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701503005/diff/100001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:215: void Finalize(scoped_refptr<base::SingleThreadTaskRunner> runner) { Finalize() runs on the I/O thread, and others run on TaskRunner. in|out_handle_ access needs a lock. Will fix in the next patch set.
https://codereview.chromium.org/2701503005/diff/100001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701503005/diff/100001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:215: void Finalize(scoped_refptr<base::SingleThreadTaskRunner> runner) { Ah, no. Now I remember that this was intentional. I expect the "task lock" protect all members. See line 400. TaskRunner always runs inside the "task lock". So we can access everything behind the "task lock". I thought I wrote comments about this somewhere, but it isn't here by this CL.
PTAL Patch Set 5 that has clear comments how we manage thread safety. https://codereview.chromium.org/2701503005/diff/100001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701503005/diff/100001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:215: void Finalize(scoped_refptr<base::SingleThreadTaskRunner> runner) { https://codereview.chromium.org/2686043003/diff/140001/media/midi/dynamically... line 150-151 I will add similar comments here, and around the task lock declaration.
oops, not PS5 but PS6
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:223: runner->PostTask(FROM_HERE, base::Bind(&FinalizeInPort, in_handle_)); in_handle_ = kInvalidInHandle; https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:258: else Why isn't this symmetric to OutPort::Open? https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:298: runner->PostTask(FROM_HERE, base::Bind(&FinalizeOutPort, out_handle_)); out_handle_ = kInvalidOutHandle;
https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... File media/midi/dynamically_initialized_midi_manager_win.cc (right): https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:223: runner->PostTask(FROM_HERE, base::Bind(&FinalizeInPort, in_handle_)); this is not mandatory, but ok just in case. https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:251: // TODO(toyoshim): Pass instance_id to implement HandleMidiInCallback. Just in case, |instance_id| is needed only by callback for inputs. https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:258: else Ah, I set kInvalidOutHandle for OutPort::Open just in case. Actually, passed handles should not be touched when it fails, and they are not mandatory, but maybe I prefer resetting this explicitly here too. https://codereview.chromium.org/2701503005/diff/120001/media/midi/dynamically... media/midi/dynamically_initialized_midi_manager_win.cc:298: runner->PostTask(FROM_HERE, base::Bind(&FinalizeOutPort, out_handle_)); On 2017/02/24 04:18:05, yhirano wrote: > out_handle_ = kInvalidOutHandle; Acknowledged.
The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm
The CQ bit was checked by toyoshim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487926450008460,
"parent_rev": "99f41af9c02fabf7c960c6ebb770ec0a66d1ec2d", "commit_rev":
"e262421e78d33170893fd1985df28658ba45a811"}
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: device open/close for dynamic manager instantiation on Windows This patch implements functionalities to open and close devices. Closing operations will be performed outside the manager instance so that the manager does not block I/O thread. But it would block next instance's initialization correctly. BUG=497573, 617086, 672793 ========== to ========== Web MIDI: device open/close for dynamic manager instantiation on Windows This patch implements functionalities to open and close devices. Closing operations will be performed outside the manager instance so that the manager does not block I/O thread. But it would block next instance's initialization correctly. BUG=497573, 617086, 672793 Review-Url: https://codereview.chromium.org/2701503005 Cr-Commit-Position: refs/heads/master@{#452786} Committed: https://chromium.googlesource.com/chromium/src/+/e262421e78d33170893fd1985df2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as https://chromium.googlesource.com/chromium/src/+/e262421e78d33170893fd1985df2... |
