Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2741713002:
Web MIDI: implement TaskService (Closed)
|
Created:
3 years, 9 months ago by Takashi Toyoshima Modified:
3 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, toyoshim+midi_chromium.org, pmalani_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb MIDI: implement TaskService
Current Web MIDI code base has platform specific implementation
that provides graceful asynchronous task handling on Linux and
Windows. It allows MidiManager to post member method to another
threads safely even if the MidiManager instance can be destructed
dynamically.
To expand this functionalities to all platform, I will introduce
a common infrastructure, TaskService, to be used on all platforms.
BUG=672793
Review-Url: https://codereview.chromium.org/2741713002
Cr-Commit-Position: refs/heads/master@{#478982}
Committed: https://chromium.googlesource.com/chromium/src/+/0492fc281b7eb603be6e7f3f00afb7d9802ef3d5
Patch Set 1 #Patch Set 2 : task_service #Patch Set 3 : unit tests #Patch Set 4 : more tests #
Total comments: 9
Patch Set 5 : fix #
Total comments: 10
Patch Set 6 : review #21 #
Total comments: 1
Patch Set 7 : offline review: merge reply task to bound task #
Total comments: 2
Patch Set 8 : review #33 #
Total comments: 2
Patch Set 9 : review #35 #
Dependent Patchsets: Messages
Total messages: 42 (28 generated)
Patchset #2 (id:20001) has been deleted
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
Description was changed from ========== wip; Web MIDI: implement TaskService BUG= ========== to ========== Web MIDI: implement TaskService Current Web MIDI code base has platform specific implementation that provides graceful asynchronous task handling on Linux and Windows. It allows MidiManager to post member method to another threads safely even if the MidiManager instance can be destructed dynamically. To expand this functionalities to all platform, I will introduce a common infrastructure, TaskService, to be used on all platforms. BUG=672793 ==========
toyoshim@chromium.org changed reviewers: + yhirano@chromium.org
cc: pmalani (fyi, this will be applied to alsa port) Yutaka, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't know how the interface is used: If you have subsequent CLs which actually use the interface, that would be helpful. https://codereview.chromium.org/2741713002/diff/80001/media/midi/midi_service.h File media/midi/midi_service.h (right): https://codereview.chromium.org/2741713002/diff/80001/media/midi/midi_service... media/midi/midi_service.h:63: TaskService* task() { return task_service_.get(); } Maybe task_service is better? https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service.h File media/midi/task_service.h (right): https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.h:37: void PostStaticTask(RunnerId runner, const base::Closure& task); I think OnceClosure and BindOnce are generally preferred. Ditto below.
On 2017/06/07 09:21:03, yhirano wrote: > I don't know how the interface is used: If you have subsequent CLs which > actually use the interface, that would be helpful. > Nevermind. https://codereview.chromium.org/2923163003/ is the CL.
https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service.cc File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.cc:128: if (instance_id != bound_instance_id_ && instance_id != kInvalidInstanceId) Is it possible that instance_id == kInvalidInstanceId? https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.cc:130: } Is there any chance that UnbindInterface is executed between L130 and L131?
https://codereview.chromium.org/2741713002/diff/80001/media/midi/midi_service.h File media/midi/midi_service.h (right): https://codereview.chromium.org/2741713002/diff/80001/media/midi/midi_service... media/midi/midi_service.h:63: TaskService* task() { return task_service_.get(); } On 2017/06/07 09:21:03, yhirano wrote: > Maybe task_service is better? Done. https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service.cc File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.cc:96: kInvalidInstanceId, kReplyRunnerId, task)); This kInvalidInstanceId should be bound_instance_id_. I forgot to fix this when I change PostReply to be PostBoundReply. https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.cc:128: if (instance_id != bound_instance_id_ && instance_id != kInvalidInstanceId) Good catch. There was a case before, but it isn't in review-ready patch sets. https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.cc:130: } Right. Let me remove these two lines. https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service.h File media/midi/task_service.h (right): https://codereview.chromium.org/2741713002/diff/80001/media/midi/task_service... media/midi/task_service.h:37: void PostStaticTask(RunnerId runner, const base::Closure& task); On 2017/06/07 09:21:03, yhirano wrote: > I think OnceClosure and BindOnce are generally preferred. Ditto below. Done.
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/2741713002/diff/100001/media/midi/task_servic... File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.cc:50: // Now RunTask never run any posted task. But invoked tasks might be still [optional] "From now on RunTask will never run any task bound to the instance id..." https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_service.h File media/midi/task_service.h (right): https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:33: bool BindInstance(); In https://codereview.chromium.org/2923163003/ the return value is not used. Is it needed? If it's needed, please add a comment for it. https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:39: // Post a task to run on a specificed TaskRunner, and ensures that the bound Post"s" https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:41: void PostBoundTask(RunnerId runner, base::OnceClosure task); You should add a comment (and DCHECK) that |runner| must not be negative. If it's possible to say that |runner| must be positive, we can remove reply_task_lock_ by setting kReplyRunnerId to 0. https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:47: // that the bound instance should not quit UnbindInstance() while thee task is s/thee/the/
yhirano@chromium.org changed reviewers: + tzik@chromium.org
https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.cc:50: // Now RunTask never run any posted task. But invoked tasks might be still On 2017/06/08 09:03:38, yhirano wrote: > [optional] "From now on RunTask will never run any task bound to the instance > id..." Done. https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_service.h File media/midi/task_service.h (right): https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:33: bool BindInstance(); Yeah, I should fix caller side to check this returned value as current platform dependent codes does. https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:39: // Post a task to run on a specificed TaskRunner, and ensures that the bound On 2017/06/08 09:03:38, yhirano wrote: > Post"s" Done. https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:41: void PostBoundTask(RunnerId runner, base::OnceClosure task); That would make code a little confusing due to "index != id", but let me try. https://codereview.chromium.org/2741713002/diff/100001/media/midi/task_servic... media/midi/task_service.h:47: // that the bound instance should not quit UnbindInstance() while thee task is On 2017/06/08 09:03:38, yhirano wrote: > s/thee/the/ Done. https://codereview.chromium.org/2741713002/diff/120001/media/midi/task_servic... File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/120001/media/midi/task_servic... media/midi/task_service.cc:66: runner->PostTask(FROM_HERE, std::move(task)); changed to call PostTask without keeping on taking |lock_| as tzik@ suggested.
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.
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...
ptal. ps7 reflect yhirano's offline comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2741713002/diff/140001/media/midi/task_servic... File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/140001/media/midi/task_servic... media/midi/task_service.cc:111: DCHECK_EQ(threads_.size() + 1u, thread_task_locks_.size()); You don't need this "+1". Just leaving threads_[0] as nullptr is enough.
https://codereview.chromium.org/2741713002/diff/140001/media/midi/task_servic... File media/midi/task_service.cc (right): https://codereview.chromium.org/2741713002/diff/140001/media/midi/task_servic... media/midi/task_service.cc:111: DCHECK_EQ(threads_.size() + 1u, thread_task_locks_.size()); sounds a nice idea to keep the code simple. Done.
lgtm https://codereview.chromium.org/2741713002/diff/160001/media/midi/task_service.h File media/midi/task_service.h (right): https://codereview.chromium.org/2741713002/diff/160001/media/midi/task_servic... media/midi/task_service.h:67: scoped_refptr<base::SingleThreadTaskRunner> reply_task_runner_; [optional] PostReplyTask is gone, so it may be good to update the comment. Renaming reply_task_runner_ to something else (e.g., default_task_runner_) may be good, too.
https://codereview.chromium.org/2741713002/diff/160001/media/midi/task_service.h File media/midi/task_service.h (right): https://codereview.chromium.org/2741713002/diff/160001/media/midi/task_servic... media/midi/task_service.h:67: scoped_refptr<base::SingleThreadTaskRunner> reply_task_runner_; On 2017/06/13 09:30:47, yhirano wrote: > [optional] PostReplyTask is gone, so it may be good to update the comment. > Renaming reply_task_runner_ to something else (e.g., default_task_runner_) may > be good, too. Done.
The CQ bit was checked by toyoshim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2741713002/#ps180001 (title: "review #35")
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": 180001, "attempt_start_ts": 1497348441088930,
"parent_rev": "20c4506a8eeac8b679a8cd61cfb21e38c6a8348c", "commit_rev":
"0492fc281b7eb603be6e7f3f00afb7d9802ef3d5"}
Message was sent while issue was closed.
Description was changed from ========== Web MIDI: implement TaskService Current Web MIDI code base has platform specific implementation that provides graceful asynchronous task handling on Linux and Windows. It allows MidiManager to post member method to another threads safely even if the MidiManager instance can be destructed dynamically. To expand this functionalities to all platform, I will introduce a common infrastructure, TaskService, to be used on all platforms. BUG=672793 ========== to ========== Web MIDI: implement TaskService Current Web MIDI code base has platform specific implementation that provides graceful asynchronous task handling on Linux and Windows. It allows MidiManager to post member method to another threads safely even if the MidiManager instance can be destructed dynamically. To expand this functionalities to all platform, I will introduce a common infrastructure, TaskService, to be used on all platforms. BUG=672793 Review-Url: https://codereview.chromium.org/2741713002 Cr-Commit-Position: refs/heads/master@{#478982} Committed: https://chromium.googlesource.com/chromium/src/+/0492fc281b7eb603be6e7f3f00af... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/0492fc281b7eb603be6e7f3f00af... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
