Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2180953002:
Support PostTaskAndReply from a sequenced task. (Closed)
DescriptionSupport PostTaskAndReply from a sequenced task.
Currently, TaskRunner::PostTaskAndReply and
WorkerPool::PostTaskAndReply can only be called when
ThreadTaskRunnerHandle::IsSet(). The reply is posted back to the
thread on which PostTaskAndReply was invoked.
With this CL, these methods can also be called when
SequencedTaskRunnerHandle::IsSet(). The reply is posted back to the
sequence on which PostTaskReply was invoked.
This change is a prerequisite to implement base::PostTaskAndReply
which must work from a sequenced task
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa5k/edit#heading=h.gf6e3bwrxqqs
BUG=553459
Committed: https://crrev.com/2b295de8104e97a4fd186e03a465ffa46747cf4a
Cr-Commit-Position: refs/heads/master@{#408794}
Patch Set 1 #
Total comments: 23
Patch Set 2 : CR gab #3 #
Total comments: 4
Patch Set 3 : rebase #Patch Set 4 : CR gab #5 #Patch Set 5 : self-review #Patch Set 6 : self-review #
Total comments: 13
Patch Set 7 : CR robliao #8 #Patch Set 8 : ASSERT -> EXPECT in constructor #Patch Set 9 : CR danakj #11 (no callback) #Patch Set 10 : rebase + self-review #Patch Set 11 : rebase #
Total comments: 4
Patch Set 12 : CR dankaj #13 #Patch Set 13 : fix build error #
Total comments: 3
Messages
Total messages: 28 (8 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
Woot, this was next on my TODO list from https://groups.google.com/a/chromium.org/d/topic/chromium-dev/cd9hldEhIEc/dis... :-) Can you link the associated bug (618043) to this CL as well. Looks very good, love the approach, some comments below. Thanks! https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc File base/task_runner.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 base/task_runner.cc:8: #include "base/bind_helpers.h" I think bind_helpers.h is an implicit include from bind.h (people rarely include the latter) https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl.cc (left): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.cc:60: // Cue mission impossible theme. Aaahhh... this has been my favorite comment in Chromium for a while ;-). I guess |delete this| is not that exceptional anymore :-) https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.cc:8: #include "base/bind_helpers.h" Ditto https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.cc:65: SequenceChecker sequence_checker_; const (never detached) https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl.h (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.h:20: // invoked. Can only be called when SequencedTaskRunnerHandle::IsSet(). Indeed, this was also the conclusion from https://groups.google.com/a/chromium.org/d/topic/chromium-dev/cd9hldEhIEc/dis... on which we decided that PostTaskAndReply should move from TaskRunner to SequencedTaskRunner, is this something you'd want to take on? (this has to be true already today, in fact today people must only be calling it from SingleThreadTaskRunner or ThreadTaskRunnerHandle::Get() would blow up -- the API change might trip a few callers using overly generic TaskRunner handles for their SingleThreadTaskRunners though and making this call through them... not sure..!) Add a TODO here in my name (or yours if you want to do this -- but I'd prefer to prioritize MessageLoop deprecation): // TODO(gab): Move TaskRunner::PostTaskAndReply() to SequencedTaskRunner. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:27: ~ObjectToDelete() { *delete_flag_ = true; } Destructor private for RefCounted objects https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:43: // Expect the task's argument to be deleted before the reply runs. s/task's argument to be deleted/task's deletion flag to be set/ https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:57: scoped_refptr<TestSimpleTaskRunner> task_runner(new TestSimpleTaskRunner); Should TestSimpleTaskRunner grab SequenceTokens once they exist? Otherwise this pattern of two TaskRunners which run on same underlying thread in unitests doesn't exercise the Sequence/ThreadCheckers (they never have but now they could :-)) https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:58: scoped_refptr<TestSimpleTaskRunner> reply_runner(new TestSimpleTaskRunner); s/reply_runner/current_runner/ (i.e. it's not a new target, rather the reply binds to "current") https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:76: EXPECT_CALL(mock_object, Task(::testing::_)); using testing::_; after includes (same for other testing:: stuff, this is typically what is done to avoid namespace cruft in gmock tests) https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:83: // Expect the reply to be posted to |reply_runner|. EXPECT_FALSE(task_runner->HasPendingTask()); EXPECT_TRUE(current_runner->HasPendingTask()); https://codereview.chromium.org/2180953002/diff/1/base/threading/worker_pool.cc File base/threading/worker_pool.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/worker_pool.... base/threading/worker_pool.cc:103: [](bool task_is_slow, const tracked_objects::Location& from_here, I think you can bind |task_is_slow| by value in the [] here (ref. lambda capture) and forgo the piping trick below. I guess whether that's allowed depends on one's interpretation of the "Lambda Expressions" section @ https://chromium-cpp.appspot.com/ Turns out Dana is a great reference in this domain :-)
gab@: PTAnL https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc File base/task_runner.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/task_runner.cc#newcode8 base/task_runner.cc:8: #include "base/bind_helpers.h" On 2016/07/25 17:57:44, gab wrote: > I think bind_helpers.h is an implicit include from bind.h (people rarely include > the latter) Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.cc:8: #include "base/bind_helpers.h" On 2016/07/25 17:57:44, gab wrote: > Ditto Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.cc:65: SequenceChecker sequence_checker_; On 2016/07/25 17:57:44, gab wrote: > const (never detached) Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl.h (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl.h:20: // invoked. Can only be called when SequencedTaskRunnerHandle::IsSet(). On 2016/07/25 17:57:44, gab wrote: > Indeed, this was also the conclusion from > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/cd9hldEhIEc/dis... > on which we decided that PostTaskAndReply should move from TaskRunner to > SequencedTaskRunner, is this something you'd want to take on? > > (this has to be true already today, in fact today people must only be calling it > from SingleThreadTaskRunner or ThreadTaskRunnerHandle::Get() would blow up -- > the API change might trip a few callers using overly generic TaskRunner handles > for their SingleThreadTaskRunners though and making this call through them... > not sure..!) > > Add a TODO here in my name (or yours if you want to do this -- but I'd prefer to > prioritize MessageLoop deprecation): > > // TODO(gab): Move TaskRunner::PostTaskAndReply() to SequencedTaskRunner. PostTaskAndReply() doesn't have to be called on a SequencedTaskRunner. The only requirement is that there is a registered ThreadTaskRunnerHandle (or a SequencedTaskRunnerHandle with this CL). This works fine: scoped_refptr<SequencedTaskRunner> reply_runner = ...; SequencedTaskRunnerHandle sequenced_task_runner_handle(reply_task_runner); scoped_refptr<TaskRunner> parallel_task_runner = ...; parallel_task_runner->PostTaskAndReply(...); The task will run on |parallel_task_runner| and the reply on |reply_runner|. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:27: ~ObjectToDelete() { *delete_flag_ = true; } On 2016/07/25 17:57:44, gab wrote: > Destructor private for RefCounted objects Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:43: // Expect the task's argument to be deleted before the reply runs. On 2016/07/25 17:57:44, gab wrote: > s/task's argument to be deleted/task's deletion flag to be set/ Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:57: scoped_refptr<TestSimpleTaskRunner> task_runner(new TestSimpleTaskRunner); On 2016/07/25 17:57:44, gab wrote: > Should TestSimpleTaskRunner grab SequenceTokens once they exist? Otherwise this > pattern of two TaskRunners which run on same underlying thread in unitests > doesn't exercise the Sequence/ThreadCheckers (they never have but now they could > :-)) Created bug to remember to do this crbug.com/631186 https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:58: scoped_refptr<TestSimpleTaskRunner> reply_runner(new TestSimpleTaskRunner); On 2016/07/25 17:57:44, gab wrote: > s/reply_runner/current_runner/ (i.e. it's not a new target, rather the reply > binds to "current") Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:76: EXPECT_CALL(mock_object, Task(::testing::_)); On 2016/07/25 17:57:44, gab wrote: > using testing::_; > > after includes (same for other testing:: stuff, this is typically what is done > to avoid namespace cruft in gmock tests) Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/post_task_an... base/threading/post_task_and_reply_impl_unittest.cc:83: // Expect the reply to be posted to |reply_runner|. On 2016/07/25 17:57:44, gab wrote: > EXPECT_FALSE(task_runner->HasPendingTask()); > EXPECT_TRUE(current_runner->HasPendingTask()); Done. https://codereview.chromium.org/2180953002/diff/1/base/threading/worker_pool.cc File base/threading/worker_pool.cc (right): https://codereview.chromium.org/2180953002/diff/1/base/threading/worker_pool.... base/threading/worker_pool.cc:103: [](bool task_is_slow, const tracked_objects::Location& from_here, On 2016/07/25 17:57:44, gab wrote: > I think you can bind |task_is_slow| by value in the [] here (ref. lambda > capture) and forgo the piping trick below. > > I guess whether that's allowed depends on one's interpretation of the "Lambda > Expressions" section @ https://chromium-cpp.appspot.com/ > > Turns out Dana is a great reference in this domain :-) I think this code https://cs.chromium.org/chromium/src/base/bind_internal.h?q=bind_internal.h&s... prevents me to use base::Bind with a lambda that captures variables. And I don't know how to convert a lambda to a base::Callback without base::Bind.
lgtm w/ comments, thanks! https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:78: const PostTaskCallback& post_task_callback) { To enforce that this is intended by this API, I'd like to see a DCHECK(SequencedTaskRunnerHandle::IsSet()); here. And a comment on TaskRunner::PostTaskAndReply() clearly stating this requirement (I know I said I would do that, but it makes much sense to do it in this CL IMO). https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... base/threading/post_task_and_reply_impl_unittest.cc:16: using ::testing::_; rm leading ::
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: PTAL https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... base/threading/post_task_and_reply_impl.cc:78: const PostTaskCallback& post_task_callback) { On 2016/07/26 20:55:47, gab wrote: > To enforce that this is intended by this API, I'd like to see a > > DCHECK(SequencedTaskRunnerHandle::IsSet()); > > here. And a comment on TaskRunner::PostTaskAndReply() clearly stating this > requirement (I know I said I would do that, but it makes much sense to do it in > this CL IMO). Done. https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/20001/base/threading/post_tas... base/threading/post_task_and_reply_impl_unittest.cc:16: using ::testing::_; On 2016/07/26 20:55:47, gab wrote: > rm leading :: Done.
https://codereview.chromium.org/2180953002/diff/100001/base/task_runner.h File base/task_runner.h (right): https://codereview.chromium.org/2180953002/diff/100001/base/task_runner.h#new... base/task_runner.h:88: // PostTaskAndReply() is invoked. This allows objects that must be deleted on Nit, remove double space. https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl.cc:23: // If |reply| doesn't run because the originating execution context is no longer Might be clearer to say If RunReplyAndSelfDestruct() doesn't run. https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:26: ObjectToDelete(bool* delete_flag) : delete_flag_(delete_flag) { Explicit https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:27: EXPECT_FALSE(*delete_flag_); Should the test be allowed to proceed if this is fails? https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:28: } Add an ASSERT_TRUE(delete_flag) https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:84: EXPECT_FALSE(delete_flag); There should also be an EXPECT_TRUE for this after everything runs, right?
robliao@: PTAnL danakj@: PTAL https://codereview.chromium.org/2180953002/diff/100001/base/task_runner.h File base/task_runner.h (right): https://codereview.chromium.org/2180953002/diff/100001/base/task_runner.h#new... base/task_runner.h:88: // PostTaskAndReply() is invoked. This allows objects that must be deleted on On 2016/07/27 17:50:07, robliao wrote: > Nit, remove double space. Done. https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl.cc:23: // If |reply| doesn't run because the originating execution context is no longer On 2016/07/27 17:50:07, robliao wrote: > Might be clearer to say If RunReplyAndSelfDestruct() doesn't run. Done. https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:26: ObjectToDelete(bool* delete_flag) : delete_flag_(delete_flag) { On 2016/07/27 17:50:07, robliao wrote: > Explicit Done. https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:27: EXPECT_FALSE(*delete_flag_); On 2016/07/27 17:50:07, robliao wrote: > Should the test be allowed to proceed if this is fails? Probably not. But ASSERTs are not supported in a constructor: ../../base/threading/post_task_and_reply_impl_unittest.cc:27:5: error: constructor 'ObjectToDelete' must not return void expression ASSERT_TRUE(delete_flag); ^~~~~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro 'ASSERT_TRUE' GTEST_FATAL_FAILURE_) ^~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: expanded from macro 'GTEST_TEST_BOOLEAN_' fail(::testing::internal::GetBoolAssertionFailureMessage(\ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: expanded from macro 'GTEST_FATAL_FAILURE_' return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:28: } On 2016/07/27 17:50:07, robliao wrote: > Add an ASSERT_TRUE(delete_flag) Added EXPECT_TRUE. ASSERT_TRUE isn't supported in a constructor. EXPECT_TRUE is a better solution than DCHECK because it produces a nice error message (before crashing) even when !DCHECK_IS_ON(). https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:84: EXPECT_FALSE(delete_flag); On 2016/07/27 17:50:07, robliao wrote: > There should also be an EXPECT_TRUE for this after everything runs, right? Done.
lgtm https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/100001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:27: EXPECT_FALSE(*delete_flag_); On 2016/07/28 13:40:48, fdoray wrote: > On 2016/07/27 17:50:07, robliao wrote: > > Should the test be allowed to proceed if this is fails? > > Probably not. But ASSERTs are not supported in a constructor: > > ../../base/threading/post_task_and_reply_impl_unittest.cc:27:5: error: > constructor 'ObjectToDelete' must not return void expression > ASSERT_TRUE(delete_flag); > ^~~~~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/gtest.h:1871:23: note: expanded from macro > 'ASSERT_TRUE' > GTEST_FATAL_FAILURE_) > ^~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1192:5: note: > expanded from macro 'GTEST_TEST_BOOLEAN_' > fail(::testing::internal::GetBoolAssertionFailureMessage(\ > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../testing/gtest/include/gtest/internal/gtest-internal.h:1110:3: note: > expanded from macro 'GTEST_FATAL_FAILURE_' > return GTEST_MESSAGE_(message, ::testing::TestPartResult::kFatalFailure) Ah, that's indeed because of the early return. Probably not worth it to do the whole factory conversion just for this.
I think I'd prefer if you just did the sequence task runner bits in this CL and not change it from a class to a Callback. I'm not super sold on the callback stuff the way it was done, it reads very confusingly at the callsite, where you're passing a task in, getting a task back, and then posting that. But this is all orthogonal to the sequence stuff, so can we just do that in this CL?
danakj@: PTAnL
LGTM https://codereview.chromium.org/2180953002/diff/200001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/200001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:75: scoped_refptr<TestSimpleTaskRunner> task_runner(new TestSimpleTaskRunner); nit: post_runner and reply_runner as names maybe? https://codereview.chromium.org/2180953002/diff/200001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:101: EXPECT_FALSE(delete_flag); can you EXPECT_TRUE the delete_flag at some point?
https://codereview.chromium.org/2180953002/diff/200001/base/threading/post_ta... File base/threading/post_task_and_reply_impl_unittest.cc (right): https://codereview.chromium.org/2180953002/diff/200001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:75: scoped_refptr<TestSimpleTaskRunner> task_runner(new TestSimpleTaskRunner); On 2016/07/29 19:52:06, danakj wrote: > nit: post_runner and reply_runner as names maybe? Done. https://codereview.chromium.org/2180953002/diff/200001/base/threading/post_ta... base/threading/post_task_and_reply_impl_unittest.cc:101: EXPECT_FALSE(delete_flag); On 2016/07/29 19:52:06, danakj wrote: > can you EXPECT_TRUE the delete_flag at some point? Done.
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, robliao@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2180953002/#ps210001 (title: "CR dankaj #13")
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
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2180953002/#ps230001 (title: "fix build error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Support PostTaskAndReply from a sequenced task. Currently, TaskRunner::PostTaskAndReply and WorkerPool::PostTaskAndReply can only be called when ThreadTaskRunnerHandle::IsSet(). The reply is posted back to the thread on which PostTaskAndReply was invoked. With this CL, these methods can also be called when SequencedTaskRunnerHandle::IsSet(). The reply is posted back to the sequence on which PostTaskReply was invoked. This change is a prerequisite to implement base::PostTaskAndReply which must work from a sequenced task https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... BUG=553459 ========== to ========== Support PostTaskAndReply from a sequenced task. Currently, TaskRunner::PostTaskAndReply and WorkerPool::PostTaskAndReply can only be called when ThreadTaskRunnerHandle::IsSet(). The reply is posted back to the thread on which PostTaskAndReply was invoked. With this CL, these methods can also be called when SequencedTaskRunnerHandle::IsSet(). The reply is posted back to the sequence on which PostTaskReply was invoked. This change is a prerequisite to implement base::PostTaskAndReply which must work from a sequenced task https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... BUG=553459 Committed: https://crrev.com/2b295de8104e97a4fd186e03a465ffa46747cf4a Cr-Commit-Position: refs/heads/master@{#408794} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/2b295de8104e97a4fd186e03a465ffa46747cf4a Cr-Commit-Position: refs/heads/master@{#408794}
Message was sent while issue was closed.
Took a post-commit peak at last modifications : one nit below. https://codereview.chromium.org/2180953002/diff/230001/base/threading/post_ta... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/230001/base/threading/post_ta... base/threading/post_task_and_reply_impl.cc:32: : sequence_checker_(), Not necessary, member should be automatically initialized with its default constructor.
Message was sent while issue was closed.
https://codereview.chromium.org/2180953002/diff/230001/base/threading/post_ta... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/230001/base/threading/post_ta... base/threading/post_task_and_reply_impl.cc:32: : sequence_checker_(), On 2016/07/30 16:57:54, gab wrote: > Not necessary, member should be automatically initialized with its default > constructor. Build error with clang when const member is not explicitly initialized. See difference between patch set 12 and 13.
Message was sent while issue was closed.
https://codereview.chromium.org/2180953002/diff/230001/base/threading/post_ta... File base/threading/post_task_and_reply_impl.cc (right): https://codereview.chromium.org/2180953002/diff/230001/base/threading/post_ta... base/threading/post_task_and_reply_impl.cc:32: : sequence_checker_(), On 2016/08/01 13:22:22, fdoray wrote: > On 2016/07/30 16:57:54, gab wrote: > > Not necessary, member should be automatically initialized with its default > > constructor. > > Build error with clang when const member is not explicitly initialized. See > difference between patch set 12 and 13. Uh, that surprises me, I don't see why that's necessary, but okay. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
