Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765703002:
Add the COM STA Task Runner (Closed)
DescriptionAdd the COM STA Task Runner
The COM STA Task Runner executes tasks in a COM STA. Task runners in
the future may share STAs, so callers should not assume STAs are shared
or are dedicated.
Callers requiring tasks to run in the same STA should post to the same
task runner.
BUG=662122
Review-Url: https://codereview.chromium.org/2765703002
Cr-Commit-Position: refs/heads/master@{#458900}
Committed: https://chromium.googlesource.com/chromium/src/+/80891393a521ee82efb1d9c513e540baa270dd02
Patch Set 1 #
Total comments: 38
Patch Set 2 : CR Feedback #Patch Set 3 : CR Feedback #
Total comments: 10
Patch Set 4 : Templatized Delegate Creation #Patch Set 5 : CR Feedback #
Total comments: 4
Patch Set 6 : Nits #
Total comments: 2
Depends on Patchset: Messages
Total messages: 30 (14 generated)
The CQ bit was checked by robliao@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.
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:25: #include "build/build_config.h" Not needed if you include it in the .h. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:145: : SchedulerWorkerDelegate(thread_name), task_tracker_(task_tracker) {} ~SchedulerWorkerCOMDelegate() { DCHECK(!scoped_comp_initializer_); } https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:202: } void ReEnqueueSequence(scoped_refptr<Sequence> sequence) override { DCHECK_NE(sequence, message_pump_sequence_); SchedulerWorkerDelegate::ReEnqueueSequence(std::move(sequence)); } https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:219: message_pump_sequence_->PushTask(std::move(pump_message_task)); bool was_empty = message_pump_sequence_->PushTask(...); DCHECK(was_empty); https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:227: scoped_refptr<Sequence> message_pump_sequence_ = new Sequence; >>const<< scoped_refptr<Sequence> message_pump_sequence_ = new Sequence; https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: std::unique_ptr<SchedulerWorkerDelegate> delegate; Optional: - Instantiate the right type of delegate from CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits and pass it to CreateAndRegisterSchedulerWorker in lieu of the the enum. - Use an AtomicSequenceNumber for the id (to avoid having to lock when instantiating the delegate from CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits). - Get rid of the enum. I like to avoid enums when possible. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:17: #include "base/task_scheduler/task_scheduler.h" #include "build/build_config.h" https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:53: enum DelegateType { enum class
Patchset #2 (id:20001) has been deleted
Thanks for the comments! https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:25: #include "build/build_config.h" On 2017/03/21 15:23:12, fdoray wrote: > Not needed if you include it in the .h. Done. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:145: : SchedulerWorkerDelegate(thread_name), task_tracker_(task_tracker) {} On 2017/03/21 15:23:12, fdoray wrote: > ~SchedulerWorkerCOMDelegate() { > DCHECK(!scoped_comp_initializer_); > } Done. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:202: } On 2017/03/21 15:23:12, fdoray wrote: > void ReEnqueueSequence(scoped_refptr<Sequence> sequence) override { > DCHECK_NE(sequence, message_pump_sequence_); > SchedulerWorkerDelegate::ReEnqueueSequence(std::move(sequence)); > } This check is already covered by SchedulerWorkerDelegate::ReEnqueueSequence sequence check. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:219: message_pump_sequence_->PushTask(std::move(pump_message_task)); On 2017/03/21 15:23:12, fdoray wrote: > bool was_empty = message_pump_sequence_->PushTask(...); > DCHECK(was_empty); Done. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:227: scoped_refptr<Sequence> message_pump_sequence_ = new Sequence; On 2017/03/21 15:23:12, fdoray wrote: > >>const<< scoped_refptr<Sequence> message_pump_sequence_ = new Sequence; Done. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: std::unique_ptr<SchedulerWorkerDelegate> delegate; On 2017/03/21 15:23:12, fdoray wrote: > Optional: > - Instantiate the right type of delegate from > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits and pass > it to CreateAndRegisterSchedulerWorker in lieu of the the enum. > - Use an AtomicSequenceNumber for the id (to avoid having to lock when > instantiating the delegate from > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits). > - Get rid of the enum. > > I like to avoid enums when possible. I guess this comes down to how much duplication we want. We either duplicate the increment and emplace_back calls or we use the enum to drive what we want to do. This, in the future will likely need COM_MTA and/or WINRT. The only thing that differs here is the delegate, so keeping the current structure seems to make more sense. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:17: #include "base/task_scheduler/task_scheduler.h" On 2017/03/21 15:23:12, fdoray wrote: > #include "build/build_config.h" Done. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:53: enum DelegateType { On 2017/03/21 15:23:12, fdoray wrote: > enum class Done.
Cool :), simple after all, thought you wanted to re-use MessagePumpWin code? Guess that wasn't worth it? https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:31: #endif #endif // defined(OS_WIN) https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:156: // * Tasks only come from SchedulerWorkerDelegate::GetWork(): s/only come/are only coming/ here and below? Feels this tense works better for me? https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:180: // SchedulerWorkerDelegate::GetWork(). As the same time, we don't want to s/As/At/ (but overall I don't think this last sentence is even required, it's pretty obvious, just like we don't comment when we set |get_work_first_| above) https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:212: MSG msg = msg_in; Why do you need this extra variable? https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:216: msg), std::move(msg) ? Not sure if MSG is moveable but if it is (or becomes so later) that's a plus and it doesn't hurt otherwise? Looks like it's a struct (from MSDN) so maybe not but I'll let you decide. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:218: if (task_tracker_->WillPostTask(pump_message_task.get())) { Otherwise do we have to tell Windows we're dropping this MSG? https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: std::unique_ptr<SchedulerWorkerDelegate> delegate; On 2017/03/21 20:00:19, robliao wrote: > On 2017/03/21 15:23:12, fdoray wrote: > > Optional: > > - Instantiate the right type of delegate from > > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits and > pass > > it to CreateAndRegisterSchedulerWorker in lieu of the the enum. > > - Use an AtomicSequenceNumber for the id (to avoid having to lock when > > instantiating the delegate from > > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits). > > - Get rid of the enum. > > > > I like to avoid enums when possible. > > I guess this comes down to how much duplication we want. > > We either duplicate the increment and emplace_back calls or we use the enum to > drive what we want to do. > > This, in the future will likely need COM_MTA and/or WINRT. The only thing that > differs here is the delegate, so keeping the current structure seems to make > more sense. Or we make CreateAndRegisterSchedulerWorker<T> a templated method? https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:54: REGULAR, How about "DEFAULT"? Sounds more natural to me than "REGULAR" https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:390: const wchar_t* const kTestWindowClassName = constexpr wchar_t[]
> Cool :), simple after all, thought you wanted to re-use MessagePumpWin code? Guess that wasn't worth it? Yeah. Simplifying MessagePumpWin gets us much closer to what's here. Then pumping messages as tasks allows SchedulerWorker to be agnostic to any sort of message pump, so that made for a cleaner approach. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:31: #endif On 2017/03/21 21:09:34, gab wrote: > #endif // defined(OS_WIN) Ah, I thought we didn't want these for certain short scoped things. Added it back in. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:156: // * Tasks only come from SchedulerWorkerDelegate::GetWork(): On 2017/03/21 21:09:34, gab wrote: > s/only come/are only coming/ > > here and below? Feels this tense works better for me? Rephrased // This scheme below allows us to cover the following scenarios: // * Only SchedulerWorkerDelegate::GetWork() has work: // Always return the sequence from GetWork(). // * Only the Windows Message Queue has work: // Always return the sequence from GetWorkFromWindowsMessageQueue(); // * Both SchedulerWorkerDelegate::GetWork() and the Windows Message Queue // have work: // Process sequences from each source round-robin style. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:180: // SchedulerWorkerDelegate::GetWork(). As the same time, we don't want to On 2017/03/21 21:09:33, gab wrote: > s/As/At/ > > (but overall I don't think this last sentence is even required, it's pretty > obvious, just like we don't comment when we set |get_work_first_| above) Removed. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:212: MSG msg = msg_in; On 2017/03/21 21:09:34, gab wrote: > Why do you need this extra variable? Because in the shuffle I lost a "const MSG& msg_in" :-). In the past, I received feedback we didn't like non "const MSG&" even though we were going mutate them immediately (although I can't seem to find the CR from years ago). I'd actually prefer to do just MSG msg, and if you're fine with that, we'll go with that. The C++ style guide doesn't have a preference either way other than consistency. https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:216: msg), On 2017/03/21 21:09:34, gab wrote: > std::move(msg) ? Not sure if MSG is moveable but if it is (or becomes so later) > that's a plus and it doesn't hurt otherwise? > > Looks like it's a struct (from MSDN) so maybe not but I'll let you decide. sgtm. No qualms with that. It is just a struct underneath the covers. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:218: if (task_tracker_->WillPostTask(pump_message_task.get())) { On 2017/03/21 21:09:33, gab wrote: > Otherwise do we have to tell Windows we're dropping this MSG? Generally, the only thing you can do with a message is either process it or drop it. What you do with it is up to the app. We actually do drop messages in Chrome here: https://cs.chromium.org/chromium/src/ui/base/ime/input_method_win.cc?rcl=e739...) This is basically the same as shutting down with messages remaining in the queue. Those are effectively dropped. The impact is that callers who used SendMessage (like our unit test) will hang forever, but they should have been using SendMessageTimeout anyways. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: std::unique_ptr<SchedulerWorkerDelegate> delegate; On 2017/03/21 21:09:34, gab wrote: > On 2017/03/21 20:00:19, robliao wrote: > > On 2017/03/21 15:23:12, fdoray wrote: > > > Optional: > > > - Instantiate the right type of delegate from > > > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits and > > pass > > > it to CreateAndRegisterSchedulerWorker in lieu of the the enum. > > > - Use an AtomicSequenceNumber for the id (to avoid having to lock when > > > instantiating the delegate from > > > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits). > > > - Get rid of the enum. > > > > > > I like to avoid enums when possible. > > > > I guess this comes down to how much duplication we want. > > > > We either duplicate the increment and emplace_back calls or we use the enum to > > drive what we want to do. > > > > This, in the future will likely need COM_MTA and/or WINRT. The only thing that > > differs here is the delegate, so keeping the current structure seems to make > > more sense. > > Or we make CreateAndRegisterSchedulerWorker<T> a templated method? That is an interesting idea, but SchedulerWorkerCOMDelegate takes a task_tracker_ and SchedulerWorkerDelegate doesn't. As a result, we would need to specialize (separate functions), create a factory scheme to package the task_tracker_, or base::Bind a function that creates it. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:54: REGULAR, On 2017/03/21 21:09:34, gab wrote: > How about "DEFAULT"? Sounds more natural to me than "REGULAR" sgtm. Done.
https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:390: std::unique_ptr<SchedulerWorkerDelegate> delegate; On 2017/03/21 22:25:32, robliao wrote: > On 2017/03/21 21:09:34, gab wrote: > > On 2017/03/21 20:00:19, robliao wrote: > > > On 2017/03/21 15:23:12, fdoray wrote: > > > > Optional: > > > > - Instantiate the right type of delegate from > > > > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits > and > > > pass > > > > it to CreateAndRegisterSchedulerWorker in lieu of the the enum. > > > > - Use an AtomicSequenceNumber for the id (to avoid having to lock when > > > > instantiating the delegate from > > > > CreateSingleThreadTaskRunnerWithTraits/CreateCOMSTATaskRunnerWithTraits). > > > > - Get rid of the enum. > > > > > > > > I like to avoid enums when possible. > > > > > > I guess this comes down to how much duplication we want. > > > > > > We either duplicate the increment and emplace_back calls or we use the enum > to > > > drive what we want to do. > > > > > > This, in the future will likely need COM_MTA and/or WINRT. The only thing > that > > > differs here is the delegate, so keeping the current structure seems to make > > > more sense. > > > > Or we make CreateAndRegisterSchedulerWorker<T> a templated method? > That is an interesting idea, but SchedulerWorkerCOMDelegate takes a > task_tracker_ and SchedulerWorkerDelegate doesn't. > As a result, we would need to specialize (separate functions), create a factory > scheme to package the task_tracker_, or base::Bind a function that creates it. I think I just came up with a scheme to make this work. I'll add it in the morning.
lgtm https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:209: base::Bind( no base:: https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:394: delegate = MakeUnique<SchedulerWorkerDelegate>(base::StringPrintf( no base:: https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:400: base::StringPrintf("TaskSchedulerSingleThreadWorker%d%sCOMSTA", id, no base::
https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:448: WaitableEvent wait_for_create_window( This block could be replaced with task_tracker_.Flush();
> I think I just came up with a scheme to make this work. I'll add it in the morning. Ok will wait on that but let's not go overboard (hadn't realized a simple template wouldn't work), it won't change much in the end. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:31: #endif On 2017/03/21 22:25:32, robliao wrote: > On 2017/03/21 21:09:34, gab wrote: > > #endif // defined(OS_WIN) > > Ah, I thought we didn't want these for certain short scoped things. > Added it back in. Yeah, it's kind of an hand-wavy rule, for tight scopes yes, though I'm stricter on #endif comments than closing namespace comments because of lack of indentation making it quickly hard to read with preprocessor macros. Here the empty line made me lean towards preferring the closing comment (I never oppose to closing comment for preprocessor macros FWIW because I find them so hard to read in general). https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:212: MSG msg = msg_in; On 2017/03/21 22:25:32, robliao wrote: > On 2017/03/21 21:09:34, gab wrote: > > Why do you need this extra variable? > > Because in the shuffle I lost a "const MSG& msg_in" :-). > > In the past, I received feedback we didn't like non "const MSG&" even though we > were going mutate them immediately (although I can't seem to find the CR from > years ago). > > I'd actually prefer to do just MSG msg, and if you're fine with that, we'll go > with that. > The C++ style guide doesn't have a preference either way other than consistency. > https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering sgtm https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:218: if (task_tracker_->WillPostTask(pump_message_task.get())) { On 2017/03/21 22:25:32, robliao wrote: > On 2017/03/21 21:09:33, gab wrote: > > Otherwise do we have to tell Windows we're dropping this MSG? > Generally, the only thing you can do with a message is either process it or drop > it. What you do with it is up to the app. > > We actually do drop messages in Chrome here: > https://cs.chromium.org/chromium/src/ui/base/ime/input_method_win.cc?rcl=e739...) > > This is basically the same as shutting down with messages remaining in the > queue. Those are effectively dropped. The impact is that callers who used > SendMessage (like our unit test) will hang forever, but they should have been > using SendMessageTimeout anyways. Got it, and I guess SendMessage would return with an error or something if the receiving process/thread dies (which is what would happen soon after TaskTracker shuts down)? https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:214: std::move(msg)), #include <utility>
Added a templatized creation path. https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:218: if (task_tracker_->WillPostTask(pump_message_task.get())) { On 2017/03/22 16:16:04, gab wrote: > On 2017/03/21 22:25:32, robliao wrote: > > On 2017/03/21 21:09:33, gab wrote: > > > Otherwise do we have to tell Windows we're dropping this MSG? > > Generally, the only thing you can do with a message is either process it or > drop > > it. What you do with it is up to the app. > > > > We actually do drop messages in Chrome here: > > > https://cs.chromium.org/chromium/src/ui/base/ime/input_method_win.cc?rcl=e739...) > > > > This is basically the same as shutting down with messages remaining in the > > queue. Those are effectively dropped. The impact is that callers who used > > SendMessage (like our unit test) will hang forever, but they should have been > > using SendMessageTimeout anyways. > > Got it, and I guess SendMessage would return with an error or something if the > receiving process/thread dies (which is what would happen soon after TaskTracker > shuts down)? Hrm... that's a good question. One of the things about SendMessage is you have to make sure the target hwnd's thread is alive and pumping messages. I haven't encountered a case where the thread dies during the call. I might have to try this out sometime. In either case, the caller needs to be using SendMessageTimeout. https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:209: base::Bind( On 2017/03/22 12:20:19, fdoray wrote: > no base:: Done. https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:214: std::move(msg)), On 2017/03/22 16:16:04, gab wrote: > #include <utility> Done. https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:394: delegate = MakeUnique<SchedulerWorkerDelegate>(base::StringPrintf( On 2017/03/22 12:20:19, fdoray wrote: > no base:: Done. https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:400: base::StringPrintf("TaskSchedulerSingleThreadWorker%d%sCOMSTA", id, On 2017/03/22 12:20:19, fdoray wrote: > no base:: Done. https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2765703002/diff/60001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:448: WaitableEvent wait_for_create_window( On 2017/03/22 16:07:19, fdoray wrote: > This block could be replaced with task_tracker_.Flush(); Handy! Done.
lgtm w/ nits https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:421: params.priority_hint(), std::move(delegate), task_tracker_, inline |delegate|? https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:62: scoped_refptr<SingleThreadTaskRunner> CreateSingleTheadTaskRunnerWithDelegate( s/Thead/Thread/ :)
The CQ bit was checked by robliao@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...
https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.cc (right): https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:421: params.priority_hint(), std::move(delegate), task_tracker_, On 2017/03/22 18:52:34, gab wrote: > inline |delegate|? Done. https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:62: scoped_refptr<SingleThreadTaskRunner> CreateSingleTheadTaskRunnerWithDelegate( On 2017/03/22 18:52:35, gab wrote: > s/Thead/Thread/ :) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2765703002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:34: class SchedulerWorkerDelegate; Instead of exposing a class from the anonymous namespace, use SchedulerWorker::Delegate for the return value of CreateSchedulerWorkerDelegate?
https://codereview.chromium.org/2765703002/diff/120001/base/task_scheduler/sc... File base/task_scheduler/scheduler_single_thread_task_runner_manager.h (right): https://codereview.chromium.org/2765703002/diff/120001/base/task_scheduler/sc... base/task_scheduler/scheduler_single_thread_task_runner_manager.h:34: class SchedulerWorkerDelegate; On 2017/03/22 20:41:53, fdoray wrote: > Instead of exposing a class from the anonymous namespace, use > SchedulerWorker::Delegate for the return value of CreateSchedulerWorkerDelegate? I was thinking about that, but then that meant including scheduler_worker.h here to do that. Didn't seem worth it just for that but if there is a strong preference, we can do that.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2765703002/#ps120001 (title: "Nits")
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": 120001, "attempt_start_ts": 1490221457432240,
"parent_rev": "4ecbd1cc3eec2593a082c53367415f6d1b0a6523", "commit_rev":
"80891393a521ee82efb1d9c513e540baa270dd02"}
Message was sent while issue was closed.
Description was changed from ========== Add the COM STA Task Runner The COM STA Task Runner executes tasks in a COM STA. Task runners in the future may share STAs, so callers should not assume STAs are shared or are dedicated. Callers requiring tasks to run in the same STA should post to the same task runner. BUG=662122 ========== to ========== Add the COM STA Task Runner The COM STA Task Runner executes tasks in a COM STA. Task runners in the future may share STAs, so callers should not assume STAs are shared or are dedicated. Callers requiring tasks to run in the same STA should post to the same task runner. BUG=662122 Review-Url: https://codereview.chromium.org/2765703002 Cr-Commit-Position: refs/heads/master@{#458900} Committed: https://chromium.googlesource.com/chromium/src/+/80891393a521ee82efb1d9c513e5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/80891393a521ee82efb1d9c513e5... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
