Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2353913005:
Add WebTaskRunner::postCancellableTask (Closed)
DescriptionAdd WebTaskRunner::postCancellableTask
This CL introduces WebTaskRunner::postCancellableTask, that returns
TaskHandle. The caller can use TaskHandle to check if the task is already
run, and to cancel the invocation.
This should replace CancellableTaskFactory, makeCancellable, and zero-delay
Timer.
BUG=
Committed: https://crrev.com/3ee9fc49731407879d63cf5c2304934d737e9e77
Cr-Commit-Position: refs/heads/master@{#428982}
Patch Set 1 #Patch Set 2 : simplify #Patch Set 3 : +EXPORT #Patch Set 4 : fix #Patch Set 5 : move to a separate file #Patch Set 6 : win fix #Patch Set 7 : back to PS4. rebase. #Patch Set 8 : fix #Patch Set 9 : +comment #Patch Set 10 : +test #
Total comments: 27
Patch Set 11 : . #
Total comments: 21
Patch Set 12 : fix #Patch Set 13 : . #
Total comments: 13
Patch Set 14 : +DISALLOW_COPY_AND_ASSIGN. +WARN_UNUSED_RETURN. #Patch Set 15 : rebase #Messages
Total messages: 96 (58 generated)
The CQ bit was checked by tzik@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 checked by tzik@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 ========== WebTaskRunner::postCancellableTask BUG= ========== to ========== WebTaskRunner::postCancellableTask BUG= ==========
tzik@chromium.org changed reviewers: + alexclarke@chromium.org, dcheng@chromium.org, haraken@chromium.org
Description was changed from ========== WebTaskRunner::postCancellableTask BUG= ========== to ========== WebTaskRunner::postCancellableTask TODO: description BUG= ==========
Description was changed from ========== WebTaskRunner::postCancellableTask TODO: description BUG= ========== to ========== Add WebTaskRunner::postCancellableTask TODO: description BUG= ==========
PTAL. As an alternative of WTF::makeCancellable, can we integrate it to WebTaskRunner::postTask()? This is similar to an Alex's proposal as the API, but less intrusive to the impl of TaskRunner. WDYT?
The CQ bit was checked by tzik@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...
On 2016/09/21 06:09:10, tzik wrote: > PTAL. > > As an alternative of WTF::makeCancellable, can we integrate it to > WebTaskRunner::postTask()? > This is similar to an Alex's proposal as the API, but less intrusive to the impl > of TaskRunner. > > WDYT? Sorry I forgot to send my replies on the other review. I'm a bit iffy about this. This isn't an abstraction that the rest of Chromium has needed, and adding this makes it harder to do any task runner convergence in the future. The extra requirements from Oilpan are an interesting argument, but I'm not sure TaskHandle would avoid those same issues. Basically, I'd like to see a compelling use case that would require a new Blink abstraction.
The CQ bit was checked by tzik@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/09/21 06:17:14, dcheng wrote: > On 2016/09/21 06:09:10, tzik wrote: > > PTAL. > > > > As an alternative of WTF::makeCancellable, can we integrate it to > > WebTaskRunner::postTask()? > > This is similar to an Alex's proposal as the API, but less intrusive to the > impl > > of TaskRunner. > > > > WDYT? > > Sorry I forgot to send my replies on the other review. > I'm a bit iffy about this. This isn't an abstraction that the rest of Chromium > has needed, and adding this makes it harder to do any task runner convergence in > the future. The extra requirements from Oilpan are an interesting argument, but > I'm not sure TaskHandle would avoid those same issues. Basically, I'd like to > see a compelling use case that would require a new Blink abstraction. FWIW the reason I proposed adding WebTaskRunner::postCancellableTask was due to the original way we intended to implement this at the scheduler level rather than aesthetics. I don't mid this API, but for symmetry it makes sense for chromium's task runners to have it too. Another thing, it's not clear to me what the status of TimerBase or CancellableTaskFactory would be if this was implemented, would those go away?
On 2016/09/21 10:33:42, alex clarke wrote: > On 2016/09/21 06:17:14, dcheng wrote: > > On 2016/09/21 06:09:10, tzik wrote: > > > PTAL. > > > > > > As an alternative of WTF::makeCancellable, can we integrate it to > > > WebTaskRunner::postTask()? > > > This is similar to an Alex's proposal as the API, but less intrusive to the > > impl > > > of TaskRunner. > > > > > > WDYT? > > > > Sorry I forgot to send my replies on the other review. > > I'm a bit iffy about this. This isn't an abstraction that the rest of Chromium > > has needed, and adding this makes it harder to do any task runner convergence > in > > the future. The extra requirements from Oilpan are an interesting argument, > but > > I'm not sure TaskHandle would avoid those same issues. Basically, I'd like to > > see a compelling use case that would require a new Blink abstraction. > > FWIW the reason I proposed adding WebTaskRunner::postCancellableTask was due to > the original way we intended to implement this at the scheduler level rather > than > aesthetics. I don't mid this API, but for symmetry it makes sense for > chromium's > task runners to have it too. Another thing, it's not clear to me what the > status > of TimerBase or CancellableTaskFactory would be if this was implemented, would > those > go away? Right, tzik@'s proposal is to replace CancellableTaskFactory and makeCancellable (and probably TimerBase) with WebTaskRunner::postCancellableTask. However, we can be convinced the other way if you have a better proposal :D
On 2016/09/21 12:38:46, haraken wrote: > On 2016/09/21 10:33:42, alex clarke wrote: > > On 2016/09/21 06:17:14, dcheng wrote: > > > On 2016/09/21 06:09:10, tzik wrote: > > > > PTAL. > > > > > > > > As an alternative of WTF::makeCancellable, can we integrate it to > > > > WebTaskRunner::postTask()? > > > > This is similar to an Alex's proposal as the API, but less intrusive to > the > > > impl > > > > of TaskRunner. > > > > > > > > WDYT? > > > > > > Sorry I forgot to send my replies on the other review. > > > I'm a bit iffy about this. This isn't an abstraction that the rest of > Chromium > > > has needed, and adding this makes it harder to do any task runner > convergence > > in > > > the future. The extra requirements from Oilpan are an interesting argument, > > but > > > I'm not sure TaskHandle would avoid those same issues. Basically, I'd like > to > > > see a compelling use case that would require a new Blink abstraction. > > > > FWIW the reason I proposed adding WebTaskRunner::postCancellableTask was due > to > > the original way we intended to implement this at the scheduler level rather > > than > > aesthetics. I don't mid this API, but for symmetry it makes sense for > > chromium's > > task runners to have it too. Another thing, it's not clear to me what the > > status > > of TimerBase or CancellableTaskFactory would be if this was implemented, would > > those > > go away? > > Right, tzik@'s proposal is to replace CancellableTaskFactory and makeCancellable > (and probably TimerBase) with WebTaskRunner::postCancellableTask. > > However, we can be convinced the other way if you have a better proposal :D Any further comments? We're not intending to stick to WebTaskRunner::postCancellableTask but do want to make progress.
On 2016/09/23 04:52:58, haraken wrote: > On 2016/09/21 12:38:46, haraken wrote: > > On 2016/09/21 10:33:42, alex clarke wrote: > > > On 2016/09/21 06:17:14, dcheng wrote: > > > > On 2016/09/21 06:09:10, tzik wrote: > > > > > PTAL. > > > > > > > > > > As an alternative of WTF::makeCancellable, can we integrate it to > > > > > WebTaskRunner::postTask()? > > > > > This is similar to an Alex's proposal as the API, but less intrusive to > > the > > > > impl > > > > > of TaskRunner. > > > > > > > > > > WDYT? > > > > > > > > Sorry I forgot to send my replies on the other review. > > > > I'm a bit iffy about this. This isn't an abstraction that the rest of > > Chromium > > > > has needed, and adding this makes it harder to do any task runner > > convergence > > > in > > > > the future. The extra requirements from Oilpan are an interesting > argument, > > > but > > > > I'm not sure TaskHandle would avoid those same issues. Basically, I'd like > > to > > > > see a compelling use case that would require a new Blink abstraction. > > > > > > FWIW the reason I proposed adding WebTaskRunner::postCancellableTask was due > > to > > > the original way we intended to implement this at the scheduler level rather > > > than > > > aesthetics. I don't mid this API, but for symmetry it makes sense for > > > chromium's > > > task runners to have it too. Another thing, it's not clear to me what the > > > status > > > of TimerBase or CancellableTaskFactory would be if this was implemented, > would > > > those > > > go away? > > > > Right, tzik@'s proposal is to replace CancellableTaskFactory and > makeCancellable > > (and probably TimerBase) with WebTaskRunner::postCancellableTask. > > > > However, we can be convinced the other way if you have a better proposal :D > > Any further comments? > > We're not intending to stick to WebTaskRunner::postCancellableTask but do want > to make progress. Sorry, it looks like the conversation is split into two threads. I posted some thoughts in https://codereview.chromium.org/2322253002/#msg19 - I'm curious if any of those ideas are feasible.
The CQ bit was checked by tzik@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 tzik@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.
tzik@chromium.org changed reviewers: + skyostil@chromium.org
So after chatting with haraken@ and tzik@ (and alexclarke@ earlier) I think we're now all in agreement that postCancellableTask() is the way to go. With that, we can remove CancellableTaskFactory, MakeCancellable, CancelableClosureHolder, TimerBase (?) and any other cancellation helpers we've dreamed up in recent history. When it comes to this patch, I think my preference would be to put postCancellableTask right in WebTaskRunner so that folks can find it easily (i.e., patch set #4). To answer Alex's question, my feeling is that we shouldn't also add it to base::TaskRunner because it's really only necessary in the Oilpan world (would be good to mention this in a comment). Sounds good?
On 2016/10/13 04:22:44, Sami (slow -- traveling) wrote: > So after chatting with haraken@ and tzik@ (and alexclarke@ earlier) I think > we're now all in agreement that postCancellableTask() is the way to go. With > that, we can remove CancellableTaskFactory, MakeCancellable, > CancelableClosureHolder, TimerBase (?) and any other cancellation helpers we've > dreamed up in recent history. > > When it comes to this patch, I think my preference would be to put > postCancellableTask right in WebTaskRunner so that folks can find it easily > (i.e., patch set #4). To answer Alex's question, my feeling is that we shouldn't > also add it to base::TaskRunner because it's really only necessary in the Oilpan > world (would be good to mention this in a comment). Sounds good.
On 2016/10/13 04:24:27, haraken wrote: > On 2016/10/13 04:22:44, Sami (slow -- traveling) wrote: > > So after chatting with haraken@ and tzik@ (and alexclarke@ earlier) I think > > we're now all in agreement that postCancellableTask() is the way to go. With > > that, we can remove CancellableTaskFactory, MakeCancellable, > > CancelableClosureHolder, TimerBase (?) and any other cancellation helpers > we've > > dreamed up in recent history. > > > > When it comes to this patch, I think my preference would be to put > > postCancellableTask right in WebTaskRunner so that folks can find it easily > > (i.e., patch set #4). To answer Alex's question, my feeling is that we > shouldn't > > also add it to base::TaskRunner because it's really only necessary in the > Oilpan > > world (would be good to mention this in a comment). > > Sounds good. Sorry, I was hoping we'd be able to chat about this sooner rather than later (as this would also help me better document what abstractions should be used when). Looking at the API now, it's not very different from a one-shot Timer. If we do not save the return value, we cannot cancel. And if we do, then we need to save it in a member, just like Timer. I know that there were some opinions that Timer did not feel like the right abstraction for one-shot tasks, but what is the reason?
On 2016/10/13 04:34:12, dcheng wrote: > Sorry, I was hoping we'd be able to chat about this sooner rather than later (as > this would also help me better document what abstractions should be used when). Today is probably too late for you but I'd be happy to chat tomorrow morning (TOK time). It might be impossible to get alexclarke@ into the same meeting though :P > Looking at the API now, it's not very different from a one-shot Timer. If we do > not save the return value, we cannot cancel. And if we do, then we need to save > it in a member, just like Timer. I know that there were some opinions that Timer > did not feel like the right abstraction for one-shot tasks, but what is the > reason? My totally non-scientific reasoning for this preference is that a timer implies a time dependency which almost never actually exists -- the vast majority of call sites are really after a one-shot task they can post and cancel on a whim. My feeling is that a timer is a clunky way to accomplish this -- but this is just a preference. I think we need to decide whether it makes sense to offer this more targeted interface or just stick with timers for everything. (My other, mostly unrelated beef with timers is that when folks do use delays, they are fairly arbitrary and not tied into lifecycle updates, BeginFrames or anything like that.)
On 2016/10/13 04:44:10, Sami (slow -- traveling) wrote: > On 2016/10/13 04:34:12, dcheng wrote: > > Sorry, I was hoping we'd be able to chat about this sooner rather than later > (as > > this would also help me better document what abstractions should be used > when). > > Today is probably too late for you but I'd be happy to chat tomorrow morning > (TOK time). It might be impossible to get alexclarke@ into the same meeting > though :P I can chat in the mornings or night PST. However, it is fairly short notice, and I think it's not necessary after all. > > > Looking at the API now, it's not very different from a one-shot Timer. If we > do > > not save the return value, we cannot cancel. And if we do, then we need to > save > > it in a member, just like Timer. I know that there were some opinions that > Timer > > did not feel like the right abstraction for one-shot tasks, but what is the > > reason? > > My totally non-scientific reasoning for this preference is that a timer implies > a time dependency which almost never actually exists -- the vast majority of > call sites are really after a one-shot task they can post and cancel on a whim. I see, this makes sense to me. > My feeling is that a timer is a clunky way to accomplish this -- but this is > just a preference. I think we need to decide whether it makes sense to offer > this more targeted interface or just stick with timers for everything. > > (My other, mostly unrelated beef with timers is that when folks do use delays, > they are fairly arbitrary and not tied into lifecycle updates, BeginFrames or > anything like that.) Anything with a delay is generally largely arbitrary, whether it's Timer or postDelayedTask(), right? I don't wish to bikeshed, so I am OK with proceeding. The rest is just me rambling and may be something we can look at more in the future (especially since usage is relatively limited): One thing I've often wished is that blink::Timer looked a bit more like base::Timer: blink::Timer is bound to a class/method at creation time, which is usually in the initializer list. Then, usually, somewhere far away, we call startOneShot(0, BLINK_FROM_HERE). Maybe if starting the timer looked more like posting a task: m_timer.queueTask(BLINK_FROM_HERE, &Class::method, this); then we could have Timer (for things that actually need a delay) and CancellableTaskPoster (for things that don't need a delay, and yes, I know this name is bad) Just random thoughts!
On 2016/10/13 05:02:54, dcheng wrote: > Anything with a delay is generally largely arbitrary, whether it's Timer or > postDelayedTask(), right? That's right. Because of this I was wondering whether we even need the delayed variant here but maybe it's better for consistency. > I don't wish to bikeshed, so I am OK with proceeding. But that's the fun part! :) > The rest is just me rambling and may be something we can look at more in the > future (especially since usage is relatively limited): > > One thing I've often wished is that blink::Timer looked a bit more like > base::Timer: blink::Timer is bound to a class/method at creation time, which is > usually in the initializer list. Then, usually, somewhere far away, we call > startOneShot(0, BLINK_FROM_HERE). > > Maybe if starting the timer looked more like posting a task: > m_timer.queueTask(BLINK_FROM_HERE, &Class::method, this); > > then we could have Timer (for things that actually need a delay) and > CancellableTaskPoster (for things that don't need a delay, and yes, I know this > name is bad) > > Just random thoughts! If I squint enough that kind of Timer starts looking a lot like a WebTaskRunner :) But yeah, I agree that it is visually confusing to have the called method be in the constructor separate from where that method is scheduled.
On 2016/10/13 05:16:33, Sami (slow -- traveling) wrote: > On 2016/10/13 05:02:54, dcheng wrote: > > Anything with a delay is generally largely arbitrary, whether it's Timer or > > postDelayedTask(), right? > > That's right. Because of this I was wondering whether we even need the delayed > variant here but maybe it's better for consistency. > > > I don't wish to bikeshed, so I am OK with proceeding. > > But that's the fun part! :) > > > The rest is just me rambling and may be something we can look at more in the > > future (especially since usage is relatively limited): > > > > One thing I've often wished is that blink::Timer looked a bit more like > > base::Timer: blink::Timer is bound to a class/method at creation time, which > is > > usually in the initializer list. Then, usually, somewhere far away, we call > > startOneShot(0, BLINK_FROM_HERE). > > > > Maybe if starting the timer looked more like posting a task: > > m_timer.queueTask(BLINK_FROM_HERE, &Class::method, this); > > > > then we could have Timer (for things that actually need a delay) and > > CancellableTaskPoster (for things that don't need a delay, and yes, I know > this > > name is bad) > > > > Just random thoughts! > > If I squint enough that kind of Timer starts looking a lot like a WebTaskRunner > :) But yeah, I agree that it is visually confusing to have the called method be > in the constructor separate from where that method is scheduled. Let's chat more in the meeting scheduled at tomorrow's morning.
The CQ bit was checked by tzik@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.
Notes from our meeting: - Let's go with postCancellableTask \o/ - Let's add a nice comment block explaining why postCancellableTask exists and when folks should use it. - Users of startOneShot() will be migrated to postCancellableTask.
tzik@: Is PS8 ready for final review?
The CQ bit was checked by tzik@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...
On 2016/10/17 09:19:08, haraken wrote: > tzik@: Is PS8 ready for final review? Added a comment. Yes, it's now ready for review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/18 04:44:07, tzik wrote: > On 2016/10/17 09:19:08, haraken wrote: > > tzik@: Is PS8 ready for final review? > > Added a comment. Yes, it's now ready for review. Thanks! Can we add a test?
The CQ bit was checked by tzik@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...
On 2016/10/18 19:14:03, haraken wrote: > On 2016/10/18 04:44:07, tzik wrote: > > On 2016/10/17 09:19:08, haraken wrote: > > > tzik@: Is PS8 ready for final review? > > > > Added a comment. Yes, it's now ready for review. > > Thanks! Can we add a test? Done!
https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:9: class TaskHandle::ClearOnScopeOut { Sorry, would you help me understand why we need ClearOnScopeOut? https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:21: ~ClearOnScopeOut() { cancel(); } For example, why we need to call cancel() when ClearOnScopeOut is destructed? https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:45: scoper.cancel(); Can we simply call cancel() instead of scoper.cancel()? https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp:1: Add a copyright. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc:109: std::deque<base::Closure> task_queue = std::move(data_->task_queue_); Why do we need std::move? https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:34: // This function is not thread safe. Call this on the thread that the task the thread that has posted the task https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:40: // This function is not thread safe. Call this on the thread that the task Ditto. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:44: ~TaskHandle(); Add a comment and mention that the task is not cancelled on the TaskHandle's destruction. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:136: // Example: For |task_runner| and |foo| below. Instead of adding this comment, shall we add a deprecation comment on CancellableTaskFactory?
https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:32: std::unique_ptr<WTF::Closure> task = std::move(m_task); Is there any reason to prefer this over: m_task = WTF::Closure() https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:45: scoper.cancel(); On 2016/10/19 15:20:39, haraken wrote: > > Can we simply call cancel() instead of scoper.cancel()? +1 Haraken is essentially suggesting what TimerBase::runInternal does today. In so far as cancellation goes. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:44: ~TaskHandle(); On 2016/10/19 15:20:40, haraken wrote: > > Add a comment and mention that the task is not cancelled on the TaskHandle's > destruction. Are you sure? :) Looks to me like the destructor of |m_weakPtrFactory| cancels the tasks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:9: class TaskHandle::ClearOnScopeOut { On 2016/10/19 15:20:39, haraken wrote: > > Sorry, would you help me understand why we need ClearOnScopeOut? I'll add a comment for this. This is to avoid a circular reference among related stuff: If we don't clear it on the destruction of the wrapped callback, below will make a reference cycle for foo -> m_handle -> m_task -> Bind()'s internal storage -> foo. struct Foo : GarbageCollected<Foo> { RefPtr<TaskHandle> m_handle; void bar(); }; Foo* foo = new Foo; foo->m_handle = taskRunner->postCancellableTask(WTF::bind(&Foo::bar, wrapPersistent(foo))); https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:32: std::unique_ptr<WTF::Closure> task = std::move(m_task); On 2016/10/19 15:36:46, alex clarke wrote: > Is there any reason to prefer this over: m_task = WTF::Closure() Just in case. This is to avoid potential reentrancy. If the destructor of m_task touches TaskHandle, m_task can be an undefined state. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:44: ~TaskHandle(); On 2016/10/19 15:36:46, alex clarke wrote: > On 2016/10/19 15:20:40, haraken wrote: > > > > Add a comment and mention that the task is not cancelled on the TaskHandle's > > destruction. > > Are you sure? :) Looks to me like the destructor of |m_weakPtrFactory| cancels > the tasks. Since the task has a ref to TaskHandle, TaskHandle outlives the task.
https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:32: std::unique_ptr<WTF::Closure> task = std::move(m_task); On 2016/10/19 20:46:47, tzik wrote: > On 2016/10/19 15:36:46, alex clarke wrote: > > Is there any reason to prefer this over: m_task = WTF::Closure() > > Just in case. This is to avoid potential reentrancy. If the destructor of m_task > touches TaskHandle, m_task can be an undefined state. FWIW, probably worth adding a comment for this. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:81: PassRefPtr<TaskHandle> WebTaskRunner::postCancellableTask( My understanding is that PassRefPtr is deprecated, and we should just use RefPtr directly. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc:109: std::deque<base::Closure> task_queue = std::move(data_->task_queue_); On 2016/10/19 15:20:39, haraken wrote: > > Why do we need std::move? I believe this is because it's possible that executing a task in the task queue will mutate the fake web task runner, so we pull everything out first. (Probably worth commenting on this)
https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:9: class TaskHandle::ClearOnScopeOut { On 2016/10/19 20:46:47, tzik wrote: > On 2016/10/19 15:20:39, haraken wrote: > > > > Sorry, would you help me understand why we need ClearOnScopeOut? > > I'll add a comment for this. This is to avoid a circular reference among related > stuff: > If we don't clear it on the destruction of the wrapped callback, below will make > a reference cycle for foo -> m_handle -> m_task -> Bind()'s internal storage -> > foo. > > struct Foo : GarbageCollected<Foo> { > RefPtr<TaskHandle> m_handle; > void bar(); > }; > > Foo* foo = new Foo; > foo->m_handle = taskRunner->postCancellableTask(WTF::bind(&Foo::bar, > wrapPersistent(foo))); That makes sense. However, then why is it not enough to call TaskHandle::cancel() after running the task? I don't fully understand why we need the comlexity of ClearOnScopeOut.
The CQ bit was checked by tzik@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/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:9: class TaskHandle::ClearOnScopeOut { On 2016/10/20 09:42:46, haraken wrote: > On 2016/10/19 20:46:47, tzik wrote: > > On 2016/10/19 15:20:39, haraken wrote: > > > > > > Sorry, would you help me understand why we need ClearOnScopeOut? > > > > I'll add a comment for this. This is to avoid a circular reference among > related > > stuff: > > If we don't clear it on the destruction of the wrapped callback, below will > make > > a reference cycle for foo -> m_handle -> m_task -> Bind()'s internal storage > -> > > foo. > > > > struct Foo : GarbageCollected<Foo> { > > RefPtr<TaskHandle> m_handle; > > void bar(); > > }; > > > > Foo* foo = new Foo; > > foo->m_handle = taskRunner->postCancellableTask(WTF::bind(&Foo::bar, > > wrapPersistent(foo))); > > That makes sense. > > However, then why is it not enough to call TaskHandle::cancel() after running > the task? I don't fully understand why we need the comlexity of ClearOnScopeOut. That still leaves a circular reference if a task is deleted before running. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:21: ~ClearOnScopeOut() { cancel(); } On 2016/10/19 15:20:39, haraken wrote: > > For example, why we need to call cancel() when ClearOnScopeOut is destructed? This is to clear the TaskHandle::m_task to cut the circular reference. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:45: scoper.cancel(); On 2016/10/19 15:36:46, alex clarke wrote: > On 2016/10/19 15:20:39, haraken wrote: > > > > Can we simply call cancel() instead of scoper.cancel()? > > +1 Haraken is essentially suggesting what TimerBase::runInternal does today. In > so far as cancellation goes. Done. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:81: PassRefPtr<TaskHandle> WebTaskRunner::postCancellableTask( On 2016/10/19 22:21:47, dcheng wrote: > My understanding is that PassRefPtr is deprecated, and we should just use RefPtr > directly. Done. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp:1: On 2016/10/19 15:20:39, haraken wrote: > > Add a copyright. Done. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/test/fake_web_task_runner.cc:109: std::deque<base::Closure> task_queue = std::move(data_->task_queue_); On 2016/10/19 22:21:47, dcheng wrote: > On 2016/10/19 15:20:39, haraken wrote: > > > > Why do we need std::move? > > I believe this is because it's possible that executing a task in the task queue > will mutate the fake web task runner, so we pull everything out first. > > (Probably worth commenting on this) Added a comment, and fixed a behavior as well. The previous implementation didn't run tasks that posted in runUntilIdle. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:34: // This function is not thread safe. Call this on the thread that the task On 2016/10/19 15:20:39, haraken wrote: > > the thread that has posted the task Done. https://codereview.chromium.org/2353913005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:40: // This function is not thread safe. Call this on the thread that the task On 2016/10/19 15:20:40, haraken wrote: > > Ditto. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
LGTM https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:11: // possible circular references in a case below: avoid a circular reference like below https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:24: class TaskHandle::CancelOnScopeOut { I'd rename this to CancelOnTaskDestruction https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:33: // cancelled or the task is ran already. run (or completed) https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:38: // Cancels the task invocation. Do nothing if the task is cancelled or ran Ditto.
https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:9: // This class holds a reference to a TaskHandle to make sure it keeps alive Nit: to make sure it keeps alive => to keep it alive https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:10: // while a task is pending in a task queue, and clears its reference to avoid a Nit: until we read line 22-23, it's not clear when this reference is cleared. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:22: // CancelOnScopeOut is needed to cut the circle by clearing |m_task| when the Nit: cut the circle => break the cycle https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:26: explicit CancelOnScopeOut(WTF::PassRefPtr<TaskHandle> handle) Nit: RefPtr here (also no WTF:: prefix is needed) https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:32: if (m_handle) Is there a time when this will be null? https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:19: // WebTaskRunner::postCancellableTask will take over CancellableTaskFactory. Nit: take over => replace https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:37: // Note that the task is not automatically cancelled on TaskHandle scope out. Hmm, that's unfortunate. That means that something using postCancellableTask that binds a WeakPersistent won't be able to take advantage of the scheduler fast-path for cancelled callbacks.
The CQ bit was checked by tzik@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 checked by tzik@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/2353913005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:9: // This class holds a reference to a TaskHandle to make sure it keeps alive On 2016/10/21 06:14:46, dcheng wrote: > Nit: to make sure it keeps alive => to keep it alive Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:11: // possible circular references in a case below: On 2016/10/20 17:07:37, haraken wrote: > > avoid a circular reference like below Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:22: // CancelOnScopeOut is needed to cut the circle by clearing |m_task| when the On 2016/10/21 06:14:46, dcheng wrote: > Nit: cut the circle => break the cycle Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:24: class TaskHandle::CancelOnScopeOut { On 2016/10/20 17:07:37, haraken wrote: > > I'd rename this to CancelOnTaskDestruction Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:26: explicit CancelOnScopeOut(WTF::PassRefPtr<TaskHandle> handle) On 2016/10/21 06:14:46, dcheng wrote: > Nit: RefPtr here (also no WTF:: prefix is needed) Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:32: if (m_handle) On 2016/10/21 06:14:46, dcheng wrote: > Is there a time when this will be null? Yes. We make a temporary object on WTF::bind call and move it into the internal storage, then the temporary object is nulled and destructed. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:19: // WebTaskRunner::postCancellableTask will take over CancellableTaskFactory. On 2016/10/21 06:14:46, dcheng wrote: > Nit: take over => replace Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:37: // Note that the task is not automatically cancelled on TaskHandle scope out. On 2016/10/21 06:14:46, dcheng wrote: > Hmm, that's unfortunate. That means that something using postCancellableTask > that binds a WeakPersistent won't be able to take advantage of the scheduler > fast-path for cancelled callbacks. It hopefully doesn't make big difference. I expect the users of the postCancellableTask manually clears the TaskHandle on their destructor, and the destructor call will be not so late after WeakPersistent invalidation. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:33: // cancelled or the task is ran already. On 2016/10/20 17:07:37, haraken wrote: > > run (or completed) Done. https://codereview.chromium.org/2353913005/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:38: // Cancels the task invocation. Do nothing if the task is cancelled or ran On 2016/10/20 17:07:37, haraken wrote: > > Ditto. Done.
Description was changed from ========== Add WebTaskRunner::postCancellableTask TODO: description BUG= ========== to ========== Add WebTaskRunner::postCancellableTask This CL introduces WebTaskRunner::postCancellableTask, that returns TaskHandle. The caller can use TaskHandle to check if the task is already run, and to cancel the invocation. This should replace CancellableTaskFactory, makeCancellable, and zero-delay Timer. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:60: cancel(); This call looks a bit interesting, because cancel() also moves m_task into a local temporary. (I wonder if cancel() can just reset() m_task after invalidating the weak pointers?) https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp:65: // handle->isActive() should switch to false bofer the task start running. Nit: bofer the task start running => before the task starts running https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:137: RefPtr<TaskHandle> postDelayedCancellableTask(const WebTraceLocation&, Does it make sense to mark these as WARN_UNUSED_RESULT? (If the TaskHandle isn't saved, it doesn't really make sense to use these, right?)
I think the API looks great now. https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:39: RefPtr<TaskHandle> m_handle; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:37: // Note that the task is not automatically cancelled on TaskHandle scope out. Is the last sentence still accurate?
https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:22: // CancelOnTaskDestruction is needed to break the circle by clearing |m_task| I wonder if we could do something in a follow on patch to detect if this is necessary for a given WTF::Closure and use a "fast path" when it isn't? Maybe that is a micro optimization too far?
The CQ bit was checked by tzik@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/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:22: // CancelOnTaskDestruction is needed to break the circle by clearing |m_task| On 2016/10/26 12:40:42, alex clarke wrote: > I wonder if we could do something in a follow on patch to detect if this is > necessary for a given WTF::Closure and use a "fast path" when it isn't? > > Maybe that is a micro optimization too far? Do you mean a "fast path" for closures that don't have any strong reference (or maybe just detect trivially destructible objects)? https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:39: RefPtr<TaskHandle> m_handle; On 2016/10/26 12:32:51, Sami wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:60: cancel(); On 2016/10/26 01:58:54, dcheng wrote: > This call looks a bit interesting, because cancel() also moves m_task into a > local temporary. > > (I wonder if cancel() can just reset() m_task after invalidating the weak > pointers?) I think cancel() can be potentially reenterred through a destructor of bound objects, and causes a double free if we call reset() directly there. # libstdc++ implementation seems to safe for this, but libc++ implementation doesn't. https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunnerTest.cpp:65: // handle->isActive() should switch to false bofer the task start running. On 2016/10/26 01:58:54, dcheng wrote: > Nit: bofer the task start running => before the task starts running Done. https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/CancellableTaskFactory.h:37: // Note that the task is not automatically cancelled on TaskHandle scope out. On 2016/10/26 12:32:51, Sami wrote: > Is the last sentence still accurate? Yes, it's mostly still valid. Added a comment to mention that. Since the wrapped function has a strong reference to the TaskHandle, the scope out of the resulting RefPtr<TaskHandle> doesn't imply the cancellation. https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebTaskRunner.h (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebTaskRunner.h:137: RefPtr<TaskHandle> postDelayedCancellableTask(const WebTraceLocation&, On 2016/10/26 01:58:54, dcheng wrote: > Does it make sense to mark these as WARN_UNUSED_RESULT? > > (If the TaskHandle isn't saved, it doesn't really make sense to use these, > right?) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
lgtm https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/WebTaskRunner.cpp (right): https://codereview.chromium.org/2353913005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/WebTaskRunner.cpp:22: // CancelOnTaskDestruction is needed to break the circle by clearing |m_task| On 2016/10/27 06:38:53, tzik wrote: > On 2016/10/26 12:40:42, alex clarke wrote: > > I wonder if we could do something in a follow on patch to detect if this is > > necessary for a given WTF::Closure and use a "fast path" when it isn't? > > > > Maybe that is a micro optimization too far? > > Do you mean a "fast path" for closures that don't have any strong reference (or > maybe just detect trivially destructible objects)? I'm wondering about things like this: foo->m_handle = taskRunner->postCancellableTask( BLINK_FROM_HERE, WTF::bind(&Foo::bar, foo->GetWeakPtr())); It might require some plumbing (not in this patch!) but it should be possible to detect the weak pointer.
The CQ bit was checked by tzik@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.
Sami: ping. Any other concern on this?
My apologies, I thought I already gave my lgtm. Thanks for pushing this through :)
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dcheng@chromium.org, alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2353913005/#ps280001 (title: "rebase")
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.
Description was changed from ========== Add WebTaskRunner::postCancellableTask This CL introduces WebTaskRunner::postCancellableTask, that returns TaskHandle. The caller can use TaskHandle to check if the task is already run, and to cancel the invocation. This should replace CancellableTaskFactory, makeCancellable, and zero-delay Timer. BUG= ========== to ========== Add WebTaskRunner::postCancellableTask This CL introduces WebTaskRunner::postCancellableTask, that returns TaskHandle. The caller can use TaskHandle to check if the task is already run, and to cancel the invocation. This should replace CancellableTaskFactory, makeCancellable, and zero-delay Timer. BUG= ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add WebTaskRunner::postCancellableTask This CL introduces WebTaskRunner::postCancellableTask, that returns TaskHandle. The caller can use TaskHandle to check if the task is already run, and to cancel the invocation. This should replace CancellableTaskFactory, makeCancellable, and zero-delay Timer. BUG= ========== to ========== Add WebTaskRunner::postCancellableTask This CL introduces WebTaskRunner::postCancellableTask, that returns TaskHandle. The caller can use TaskHandle to check if the task is already run, and to cancel the invocation. This should replace CancellableTaskFactory, makeCancellable, and zero-delay Timer. BUG= Committed: https://crrev.com/3ee9fc49731407879d63cf5c2304934d737e9e77 Cr-Commit-Position: refs/heads/master@{#428982} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/3ee9fc49731407879d63cf5c2304934d737e9e77 Cr-Commit-Position: refs/heads/master@{#428982} |
