Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2506553002:
Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (Closed)
|
Created:
4 years, 1 month ago by nhiroki Modified:
4 years, 1 month ago CC:
chromium-reviews, tyoshino+watch_chromium.org, nessy, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, Srirama, Nate Chapin, scheduler-bugs_chromium.org, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask
BUG=665285
Committed: https://crrev.com/536a75d6f8cdbb64f957cf6abb13ff1e2d581743
Cr-Commit-Position: refs/heads/master@{#432451}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add comments #
Total comments: 6
Messages
Total messages: 31 (14 generated)
The CQ bit was checked by nhiroki@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...
Patchset #1 (id:1) has been deleted
nhiroki@chromium.org changed reviewers: + tzik@chromium.org, yhirano@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2506553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:277: WTF::bind(&ResourceCallback::runTask, WTF::unretained(this))); Please write a comment explaining why this unretained(this) is safe.
Thank you. Updated.
https://codereview.chromium.org/2506553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:277: WTF::bind(&ResourceCallback::runTask, WTF::unretained(this))); On 2016/11/15 05:40:07, yhirano wrote: > Please write a comment explaining why this unretained(this) is safe. Done.
The CQ bit was checked by nhiroki@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.
core/fetch LGTM
lgtm
Description was changed from ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 ==========
nhiroki@chromium.org changed reviewers: + haraken@chromium.org
+haraken@, can you review core/html and platform/scheduler? Thanks.
https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:273: Platform::current() Can we use TaskRunnerHelper or ParentFrameTaskRunner? Our plan is to also deprecate Platform::current()->currentThread()->...->postTask() with TaskRunnerHelper/ParentFrameTaskRunner.
Thank you for your comment. https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:273: Platform::current() On 2016/11/15 16:15:28, haraken wrote: > > Can we use TaskRunnerHelper or ParentFrameTaskRunner? Our plan is to also > deprecate Platform::current()->currentThread()->...->postTask() with > TaskRunnerHelper/ParentFrameTaskRunner. I'd prefer to work on it in a separate CL because seemingly it needs more extensive work to get Document/ExecutionContext/etc here for accessing a task runner.
LGTM https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:271: // |m_taskHandle| is destroyed on the dtor of this ResourceCallback. tzik@: We prefer using wrapWeakPersistent(), right?
https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:271: // |m_taskHandle| is destroyed on the dtor of this ResourceCallback. On 2016/11/16 04:22:55, haraken wrote: > > tzik@: We prefer using wrapWeakPersistent(), right? Since ResourceCallback is not GCed object, we need unretained here rather than wrapWeakPersistent.
https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:271: // |m_taskHandle| is destroyed on the dtor of this ResourceCallback. On 2016/11/16 04:45:33, tzik wrote: > On 2016/11/16 04:22:55, haraken wrote: > > > > tzik@: We prefer using wrapWeakPersistent(), right? > > Since ResourceCallback is not GCed object, we need unretained here rather than > wrapWeakPersistent. Makes sense. BTW, what happens if we post a cancellable task with a wrapPersistent and keep holding a task handle on |this| object? Will it cause a leak?
https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/Resource.cpp:271: // |m_taskHandle| is destroyed on the dtor of this ResourceCallback. On 2016/11/16 05:02:55, haraken wrote: > On 2016/11/16 04:45:33, tzik wrote: > > On 2016/11/16 04:22:55, haraken wrote: > > > > > > tzik@: We prefer using wrapWeakPersistent(), right? > > > > Since ResourceCallback is not GCed object, we need unretained here rather than > > wrapWeakPersistent. > > Makes sense. > > BTW, what happens if we post a cancellable task with a wrapPersistent and keep > holding a task handle on |this| object? Will it cause a leak? I guess the task just runs and then releases the reference to |this| on its dtor.
On 2016/11/16 05:54:01, nhiroki wrote: > https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/fetch/Resource.cpp (right): > > https://codereview.chromium.org/2506553002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/fetch/Resource.cpp:271: // |m_taskHandle| is > destroyed on the dtor of this ResourceCallback. > On 2016/11/16 05:02:55, haraken wrote: > > On 2016/11/16 04:45:33, tzik wrote: > > > On 2016/11/16 04:22:55, haraken wrote: > > > > > > > > tzik@: We prefer using wrapWeakPersistent(), right? > > > > > > Since ResourceCallback is not GCed object, we need unretained here rather > than > > > wrapWeakPersistent. > > > > Makes sense. > > > > BTW, what happens if we post a cancellable task with a wrapPersistent and keep > > holding a task handle on |this| object? Will it cause a leak? > > I guess the task just runs and then releases the reference to |this| on its > dtor. ah, you're right :)
The CQ bit was checked by nhiroki@chromium.org
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 ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 Committed: https://crrev.com/536a75d6f8cdbb64f957cf6abb13ff1e2d581743 Cr-Commit-Position: refs/heads/master@{#432451} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/536a75d6f8cdbb64f957cf6abb13ff1e2d581743 Cr-Commit-Position: refs/heads/master@{#432451}
Message was sent while issue was closed.
Description was changed from ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask BUG=665285 Committed: https://crrev.com/536a75d6f8cdbb64f957cf6abb13ff1e2d581743 Cr-Commit-Position: refs/heads/master@{#432451} ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask BUG=665285 Committed: https://crrev.com/536a75d6f8cdbb64f957cf6abb13ff1e2d581743 Cr-Commit-Position: refs/heads/master@{#432451} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
