Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2430513003:
Move some Document timers to frame-specific task runners. (Closed)
DescriptionMove some Document timers to frame-specific task runners.
BUG=624694
R=haraken@chromium.org,skyostil@chromium.org
Committed: https://crrev.com/578956728fd1653f71722be150e2a158a19fa04c
Cr-Commit-Position: refs/heads/master@{#427527}
Patch Set 1 #
Total comments: 10
Patch Set 2 : . #
Total comments: 1
Patch Set 3 : Remove FrameTimer #
Messages
Total messages: 28 (15 generated)
Description was changed from ========== Move some Document timers to frame-specific task runners. BUG= R=haraken@chromium.org ========== to ========== Move some Document timers to frame-specific task runners. BUG= R=haraken@chromium.org ==========
dcheng@chromium.org changed reviewers: - haraken@chromium.org
The CQ bit was checked by dcheng@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.
Description was changed from ========== Move some Document timers to frame-specific task runners. BUG= R=haraken@chromium.org ========== to ========== Move some Document timers to frame-specific task runners. BUG=624694 R=haraken@chromium.org,skyostil@chromium.org ==========
dcheng@chromium.org changed reviewers: + haraken@chromium.org, skyostil@chromium.org
https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:480: this, This look a bit weird... but I don't have better ideas atm. https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:483: m_pluginLoadingTimer(TaskType::Embed, I'm not really sure if this is the right task queue, any thoughts? (The spec doesn't talk about this task source a lot, except to mention that this task source is used to run the embed setup steps.... which is sort of plugin loading? Though in this case, it's just used to force a layout update...) https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameTimer.h (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameTimer.h:20: Document* document, I'll add additional overloads as needed?
Note the alternative is to just have these pass an explicit WebTaskRunner using TaskRunnerHelper... maybe that's what we should just do instead of introducing FrameTimer?
https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:483: m_pluginLoadingTimer(TaskType::Embed, On 2016/10/19 00:08:38, dcheng wrote: > I'm not really sure if this is the right task queue, any thoughts? > > (The spec doesn't talk about this task source a lot, except to mention that this > task source is used to run the embed setup steps.... which is sort of plugin > loading? Though in this case, it's just used to force a layout update...) Maybe this is not a speced task? If so we could use Task::Internal, because it's just internal implementation details to force a layout update. https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameTimer.h (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameTimer.h:14: class FrameTimer : public TaskRunnerTimer<TimerFiredClass> { What's your plan in the end? Are you planning to mix FrameTimers and TaskRunnerTimers in the code base? Or are you planning to minimize # of TaskRunnerTimers? https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameTimer.h:20: Document* document, On 2016/10/19 00:08:39, dcheng wrote: > I'll add additional overloads as needed? Sure. ExecutionContext* might be better since workers will also use FrameTimers.
https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Document.cpp:483: m_pluginLoadingTimer(TaskType::Embed, On 2016/10/19 03:07:47, haraken wrote: > On 2016/10/19 00:08:38, dcheng wrote: > > I'm not really sure if this is the right task queue, any thoughts? > > > > (The spec doesn't talk about this task source a lot, except to mention that > this > > task source is used to run the embed setup steps.... which is sort of plugin > > loading? Though in this case, it's just used to force a layout update...) > > Maybe this is not a speced task? If so we could use Task::Internal, because it's > just internal implementation details to force a layout update. Done. https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameTimer.h (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameTimer.h:14: class FrameTimer : public TaskRunnerTimer<TimerFiredClass> { On 2016/10/19 03:07:47, haraken wrote: > > What's your plan in the end? Are you planning to mix FrameTimers and > TaskRunnerTimers in the code base? Or are you planning to minimize # of > TaskRunnerTimers? The idea for FrameTimer is to make it easy to see when something is associated with a frame's task queue. It's possible to tell with TaskRunnerTimer(TaskRunnerHelper::get(...), ...) but it's a little harder to search for. Since we want to associate as many things with per-frame queues as possible, that means we will probably end up trying to minimize the number of TaskRunnerTimers. That being said, if we're planning on replacing this with postCancellableTask... maybe there's not much value to doing this and we should just use TaskRunnerTimer directly? https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameTimer.h:20: Document* document, On 2016/10/19 03:07:48, haraken wrote: > On 2016/10/19 00:08:39, dcheng wrote: > > I'll add additional overloads as needed? > > Sure. > > ExecutionContext* might be better since workers will also use FrameTimers. Done.
https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameTimer.h (right): https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameTimer.h:14: class FrameTimer : public TaskRunnerTimer<TimerFiredClass> { On 2016/10/19 03:47:44, dcheng wrote: > On 2016/10/19 03:07:47, haraken wrote: > > > > What's your plan in the end? Are you planning to mix FrameTimers and > > TaskRunnerTimers in the code base? Or are you planning to minimize # of > > TaskRunnerTimers? > > The idea for FrameTimer is to make it easy to see when something is associated > with a frame's task queue. It's possible to tell with > TaskRunnerTimer(TaskRunnerHelper::get(...), ...) but it's a little harder to > search for. > > Since we want to associate as many things with per-frame queues as possible, > that means we will probably end up trying to minimize the number of > TaskRunnerTimers. > > That being said, if we're planning on replacing this with postCancellableTask... > maybe there's not much value to doing this and we should just use > TaskRunnerTimer directly? Yeah, I'd prefer not mixing postCancellableTask, FrameTimer and TaskRunnerTimer in the code base. Will the following work? - postCancellableTask for all one-shot timers - TaskRunnerTimer for other timers
haraken@chromium.org changed reviewers: + alexclarke@chromium.org, tzik@chromium.org
https://codereview.chromium.org/2430513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameTimer.h (right): https://codereview.chromium.org/2430513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameTimer.h:19: FrameTimer(TaskType taskType, I suspect it will be somewhat common for the timer class to also be an execution context so how about adding a second constructor: template <typename T> FrameTimer(TaskType taskType, T* timerFiredClassAndContext, TimerFiredFunction timerFiredFunction) : TaskRunnerTimer<TimerFiredClass>( TaskRunnerHelper::get(taskType, timerFiredClassAndContext), timerFiredClassAndContext, timerFiredFunction) {}
On 2016/10/19 09:07:12, haraken wrote: > https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameTimer.h (right): > > https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameTimer.h:14: class FrameTimer : public > TaskRunnerTimer<TimerFiredClass> { > On 2016/10/19 03:47:44, dcheng wrote: > > On 2016/10/19 03:07:47, haraken wrote: > > > > > > What's your plan in the end? Are you planning to mix FrameTimers and > > > TaskRunnerTimers in the code base? Or are you planning to minimize # of > > > TaskRunnerTimers? > > > > The idea for FrameTimer is to make it easy to see when something is associated > > with a frame's task queue. It's possible to tell with > > TaskRunnerTimer(TaskRunnerHelper::get(...), ...) but it's a little harder to > > search for. > > > > Since we want to associate as many things with per-frame queues as possible, > > that means we will probably end up trying to minimize the number of > > TaskRunnerTimers. > > > > That being said, if we're planning on replacing this with > postCancellableTask... > > maybe there's not much value to doing this and we should just use > > TaskRunnerTimer directly? > > Yeah, I'd prefer not mixing postCancellableTask, FrameTimer and TaskRunnerTimer > in the code base. > > Will the following work? > > - postCancellableTask for all one-shot timers > - TaskRunnerTimer for other timers Seems like a good idea -- +1 for making one-shot timers actually be posted one-shot tasks.
PTAL. Per the feedback, I'm just using TaskRunnerTimer directly now. On 2016/10/24 11:16:39, Sami wrote: > On 2016/10/19 09:07:12, haraken wrote: > > > https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/frame/FrameTimer.h (right): > > > > > https://codereview.chromium.org/2430513003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/frame/FrameTimer.h:14: class FrameTimer : > public > > TaskRunnerTimer<TimerFiredClass> { > > On 2016/10/19 03:47:44, dcheng wrote: > > > On 2016/10/19 03:07:47, haraken wrote: > > > > > > > > What's your plan in the end? Are you planning to mix FrameTimers and > > > > TaskRunnerTimers in the code base? Or are you planning to minimize # of > > > > TaskRunnerTimers? > > > > > > The idea for FrameTimer is to make it easy to see when something is > associated > > > with a frame's task queue. It's possible to tell with > > > TaskRunnerTimer(TaskRunnerHelper::get(...), ...) but it's a little harder to > > > search for. > > > > > > Since we want to associate as many things with per-frame queues as possible, > > > that means we will probably end up trying to minimize the number of > > > TaskRunnerTimers. > > > > > > That being said, if we're planning on replacing this with > > postCancellableTask... > > > maybe there's not much value to doing this and we should just use > > > TaskRunnerTimer directly? > > > > Yeah, I'd prefer not mixing postCancellableTask, FrameTimer and > TaskRunnerTimer > > in the code base. > > > > Will the following work? > > > > - postCancellableTask for all one-shot timers > > - TaskRunnerTimer for other timers > > Seems like a good idea -- +1 for making one-shot timers actually be posted > one-shot tasks. I'd like to separate task queue changes from the postCancellableTask work, just to minimize the number of moving parts. My previous CL to move DOMTimers to the frame timer task queue ended up exposing some interesting race conditions in the chromeos login tests.
The CQ bit was checked by dcheng@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...
LGTM
The CQ bit was unchecked by dcheng@chromium.org
The CQ bit was checked by dcheng@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 ========== Move some Document timers to frame-specific task runners. BUG=624694 R=haraken@chromium.org,skyostil@chromium.org ========== to ========== Move some Document timers to frame-specific task runners. BUG=624694 R=haraken@chromium.org,skyostil@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Move some Document timers to frame-specific task runners. BUG=624694 R=haraken@chromium.org,skyostil@chromium.org ========== to ========== Move some Document timers to frame-specific task runners. BUG=624694 R=haraken@chromium.org,skyostil@chromium.org Committed: https://crrev.com/578956728fd1653f71722be150e2a158a19fa04c Cr-Commit-Position: refs/heads/master@{#427527} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/578956728fd1653f71722be150e2a158a19fa04c Cr-Commit-Position: refs/heads/master@{#427527} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
