Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2507673002:
Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2) (Closed)
|
Created:
4 years, 1 month ago by nhiroki Modified:
4 years, 1 month ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, rwlbuis, sof, nessy, Srirama, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2)
BUG=665285
Committed: https://crrev.com/350ebf173edb4994dd561ee16c2f7cf5afdaa7be
Cr-Commit-Position: refs/heads/master@{#433486}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 19
Patch Set 3 : address review comments #Dependent Patchsets: Messages
Total messages: 38 (20 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
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...
nhiroki@chromium.org changed reviewers: + haraken@chromium.org, tzik@chromium.org
PTAL, thanks! (Probably the next patch will completely remove CancellableTaskFactory)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by 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...
https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3268: // task handle is destroyed on the dtor of this Document. tzik@: Should we use wrapWeakPersistent? Or wrapPersistent? Regarding overhead, wrapPersistent is smaller than wrapWeakPersistent since it doesn't require weak processing. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:535: scheduler->loadingTaskRunner()->postDelayedCancellableTask( Nit: We should move this to TaskRunnerHelper::xxx in a follow up.
Thank you for your comments. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:530: scheduler->addPendingNavigation(m_frameType); Question: addPendingNavigation and removePendingNavigation do nothing in the current implementation. Are we able to remove them? It seems that these were added for prioritizing navigation tasks[1]. I'm not sure they're still useful after per-frame task scheduler and throttling mechanism are enabled. [1] https://codereview.chromium.org/1368643002 https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:535: scheduler->loadingTaskRunner()->postDelayedCancellableTask( On 2016/11/16 11:37:20, haraken wrote: > > Nit: We should move this to TaskRunnerHelper::xxx in a follow up. Acknowledged. I have a question related to this cleanup. Can you see my other comment?
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_...)
haraken@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:530: scheduler->addPendingNavigation(m_frameType); On 2016/11/16 13:49:32, nhiroki wrote: > Question: addPendingNavigation and removePendingNavigation do nothing in the > current implementation. Are we able to remove them? > > It seems that these were added for prioritizing navigation tasks[1]. I'm not > sure they're still useful after per-frame task scheduler and throttling > mechanism are enabled. > > [1] https://codereview.chromium.org/1368643002 It is calling UpdatePolicy(), right? However, WebThread::scheduler() should be replaced with the per-frame scheduler. So if we need the logic, we need to move the logic to WebFrameScheduler. +alex +sami
https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:530: scheduler->addPendingNavigation(m_frameType); On 2016/11/17 05:59:09, haraken wrote: > On 2016/11/16 13:49:32, nhiroki wrote: > > Question: addPendingNavigation and removePendingNavigation do nothing in the > > current implementation. Are we able to remove them? > > > > It seems that these were added for prioritizing navigation tasks[1]. I'm not > > sure they're still useful after per-frame task scheduler and throttling > > mechanism are enabled. > > > > [1] https://codereview.chromium.org/1368643002 > > It is calling UpdatePolicy(), right? Are you seeing RendererScheduler::AddPendingNavigation? I meant WebScheduler::addPendingNavigation that has an empty body. According to codesearch, RendererScheduler(Impl)::AddPendingNavigation is called only from unittests (RendererSchedulerImplTest), so we could remove them, too? > However, WebThread::scheduler() should be replaced with the per-frame scheduler. > So if we need the logic, we need to move the logic to WebFrameScheduler. > > +alex +sami
https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3268: // task handle is destroyed on the dtor of this Document. On 2016/11/16 11:37:20, haraken wrote: > > tzik@: Should we use wrapWeakPersistent? Or wrapPersistent? > > Regarding overhead, wrapPersistent is smaller than wrapWeakPersistent since it > doesn't require weak processing. That depends on the context: We should use wrapPersistent if we want to keep |this| alive and run the task even after all other references to |this| have gone. Otherwise, wrapWeakPersistent. In this case, IIUC, we should not keep Document alive just for the script execution. So wrapWeakPersistent should fit here. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4004: // error code needs to be saved. Can we put m_playPromiseErrorCode into the closure? # That's not need to do in this CL, but could you leave a TODO comment here? https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4008: // task handle is destroyed on the dtor of this HTMLMediaElement. "wrapWeakPersistent(this) is safe because..." sounds weird to me, since WeakPersistent also cancels the task when |this| has gone. So, unlike unretained, this is safe even without the TaskHandle destruction. Could you update the comment or just remove it? https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.cpp:79: // task handle is destroyed on the dtor of this HTMLParserScheduler. ditto
LGTM with comments. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3268: // task handle is destroyed on the dtor of this Document. On 2016/11/17 06:20:14, tzik wrote: > On 2016/11/16 11:37:20, haraken wrote: > > > > tzik@: Should we use wrapWeakPersistent? Or wrapPersistent? > > > > Regarding overhead, wrapPersistent is smaller than wrapWeakPersistent since it > > doesn't require weak processing. > > That depends on the context: We should use wrapPersistent if we want to keep > |this| alive and run the task even after all other references to |this| have > gone. Otherwise, wrapWeakPersistent. > In this case, IIUC, we should not keep Document alive just for the script > execution. So wrapWeakPersistent should fit here. Makes sense. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3982: wrapWeakPersistent(this))); I'm not 100% sure, but this should probably be wrapPersistent? The spec requires to fire the event, which means that the event should keep alive the media element. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4014: wrapWeakPersistent(this))); Ditto. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:530: scheduler->addPendingNavigation(m_frameType); On 2016/11/17 06:15:59, nhiroki wrote: > On 2016/11/17 05:59:09, haraken wrote: > > On 2016/11/16 13:49:32, nhiroki wrote: > > > Question: addPendingNavigation and removePendingNavigation do nothing in the > > > current implementation. Are we able to remove them? > > > > > > It seems that these were added for prioritizing navigation tasks[1]. I'm not > > > sure they're still useful after per-frame task scheduler and throttling > > > mechanism are enabled. > > > > > > [1] https://codereview.chromium.org/1368643002 > > > > It is calling UpdatePolicy(), right? > > Are you seeing RendererScheduler::AddPendingNavigation? I meant > WebScheduler::addPendingNavigation that has an empty body. According to > codesearch, RendererScheduler(Impl)::AddPendingNavigation is called only from > unittests (RendererSchedulerImplTest), so we could remove them, too? > > > However, WebThread::scheduler() should be replaced with the per-frame > scheduler. > > So if we need the logic, we need to move the logic to WebFrameScheduler. > > > > +alex +sami > ah, got it.
nhiroki@chromium.org changed reviewers: + mlamouri@chromium.org
+mlamouri@, could you take a look at my comment in HTMLMediaElement.cpp? Thanks. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:3268: // task handle is destroyed on the dtor of this Document. On 2016/11/17 06:35:19, haraken wrote: > On 2016/11/17 06:20:14, tzik wrote: > > On 2016/11/16 11:37:20, haraken wrote: > > > > > > tzik@: Should we use wrapWeakPersistent? Or wrapPersistent? > > > > > > Regarding overhead, wrapPersistent is smaller than wrapWeakPersistent since > it > > > doesn't require weak processing. > > > > That depends on the context: We should use wrapPersistent if we want to keep > > |this| alive and run the task even after all other references to |this| have > > gone. Otherwise, wrapWeakPersistent. > > In this case, IIUC, we should not keep Document alive just for the script > > execution. So wrapWeakPersistent should fit here. > > Makes sense. Thank you for the clarification. Added the comment. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3982: wrapWeakPersistent(this))); On 2016/11/17 06:35:19, haraken wrote: > > I'm not 100% sure, but this should probably be wrapPersistent? > > The spec requires to fire the event, which means that the event should keep > alive the media element. Before this CL, CancellableTaskFactory posts a task in the same manner with this postCancellableTask+wrapWeakPersistent, so replacing this with wrapPersistent may change behavior. We might want to have inputs from experts. +mlamouri@, what do you think about this? I chose you because you recently changed this class. Please forward if you know more appropriate reviewer. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4004: // error code needs to be saved. On 2016/11/17 06:20:15, tzik wrote: > Can we put m_playPromiseErrorCode into the closure? > # That's not need to do in this CL, but could you leave a TODO comment here? Done. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4008: // task handle is destroyed on the dtor of this HTMLMediaElement. On 2016/11/17 06:20:15, tzik wrote: > "wrapWeakPersistent(this) is safe because..." sounds weird to me, since > WeakPersistent also cancels the task when |this| has gone. So, unlike > unretained, this is safe even without the TaskHandle destruction. > Could you update the comment or just remove it? Good point. Removed the comment entirely. https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.cpp (right): https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLParserScheduler.cpp:79: // task handle is destroyed on the dtor of this HTMLParserScheduler. On 2016/11/17 06:20:15, tzik wrote: > ditto Done.
lgtm
On 2016/11/17 05:59:09, haraken wrote: > https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/NavigationScheduler.cpp (right): > > https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/NavigationScheduler.cpp:530: > scheduler->addPendingNavigation(m_frameType); > On 2016/11/16 13:49:32, nhiroki wrote: > > Question: addPendingNavigation and removePendingNavigation do nothing in the > > current implementation. Are we able to remove them? That's odd, I'll need to look into why that happened. > > > > It seems that these were added for prioritizing navigation tasks[1]. I'm not > > sure they're still useful after per-frame task scheduler and throttling > > mechanism are enabled. > > > > [1] https://codereview.chromium.org/1368643002 > > It is calling UpdatePolicy(), right? > > However, WebThread::scheduler() should be replaced with the per-frame scheduler. > So if we need the logic, we need to move the logic to WebFrameScheduler. > > +alex +sami Right, lets look at that in another patch :)
lgtm
Description was changed from ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskScheduller::postCancellableTask (2) BUG=665285 ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2) BUG=665285 ==========
HTMLMediaElement.* lgtm
On 2016/11/18 14:13:26, mlamouri (OOO until 29-11) wrote: > HTMLMediaElement.* lgtm
On 2016/11/18 14:13:26, mlamouri (OOO until 29-11) wrote:
> HTMLMediaElement.* lgtm
Just in case you missed my comment in HTMLMediaElement.* because my review
request message was super unclear ("could you take a look at my comment in
HTMLMediaElement.cpp?"). I meant to ask you to review not TODO comment change
but a following review comment in HTMLMediaElement.cpp:
>
https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3982:
> wrapWeakPersistent(this)));
> On 2016/11/17 06:35:19, haraken wrote:
> >
> > I'm not 100% sure, but this should probably be wrapPersistent?
> >
> > The spec requires to fire the event, which means that the event should keep
> > alive the media element.
>
> Before this CL, CancellableTaskFactory posts a task in the same manner with
this
> postCancellableTask+wrapWeakPersistent, so replacing this with wrapPersistent
> may change behavior. We might want to have inputs from experts.
>
> +mlamouri@, what do you think about this? I chose you because you recently
> changed this class. Please forward if you know more appropriate reviewer.
If you already noticed this and then gave me L-G-T-M, please ignore this email.
Sorry for my unclear message.
On 2016/11/21 03:42:52, nhiroki wrote:
> On 2016/11/18 14:13:26, mlamouri (OOO until 29-11) wrote:
> > HTMLMediaElement.* lgtm
>
> Just in case you missed my comment in HTMLMediaElement.* because my review
> request message was super unclear ("could you take a look at my comment in
> HTMLMediaElement.cpp?"). I meant to ask you to review not TODO comment change
> but a following review comment in HTMLMediaElement.cpp:
>
> >
>
https://codereview.chromium.org/2507673002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3982:
> > wrapWeakPersistent(this)));
> > On 2016/11/17 06:35:19, haraken wrote:
> > >
> > > I'm not 100% sure, but this should probably be wrapPersistent?
> > >
> > > The spec requires to fire the event, which means that the event should
keep
> > > alive the media element.
> >
> > Before this CL, CancellableTaskFactory posts a task in the same manner with
> this
> > postCancellableTask+wrapWeakPersistent, so replacing this with
wrapPersistent
> > may change behavior. We might want to have inputs from experts.
> >
> > +mlamouri@, what do you think about this? I chose you because you recently
> > changed this class. Please forward if you know more appropriate reviewer.
>
> If you already noticed this and then gave me L-G-T-M, please ignore this
email.
> Sorry for my unclear message.
As my previous reply, using wrapWeakPersistent() would be consistent with the
original code, so let me commit this as is. If you think wrapPersistent() is
more preferable, I'll work on it in a separate CL.
The CQ bit was checked by nhiroki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2507673002/#ps60001 (title: "address review comments")
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": 60001, "attempt_start_ts": 1479700000201500,
"parent_rev": ["f56f003cf1b4bfcc81a3fbbd24a9abda1b5af7f8", null], "commit_rev":
["4df042a9364b36c16e40b7f9584284fe7c02671c", null]}
Message was sent while issue was closed.
Description was changed from ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2) BUG=665285 ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2) BUG=665285 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2) BUG=665285 ========== to ========== Scheduler: Deprecate CancellableTaskFactory in favor of WebTaskRunner::postCancellableTask (2) BUG=665285 Committed: https://crrev.com/350ebf173edb4994dd561ee16c2f7cf5afdaa7be Cr-Commit-Position: refs/heads/master@{#433486} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/350ebf173edb4994dd561ee16c2f7cf5afdaa7be Cr-Commit-Position: refs/heads/master@{#433486} |
