scheduler: Implement WebFrameScheduler and WebPageScheduler

scheduler: Implement WebFrameScheduler and WebPageScheduler

The initial implementations creates all the frame specific task queues but ignores
setPageInBackground and setFrameVisible.

This patch also posts DOMTimers on the corresponding LocalFrame's timer queue.

Each LocalFrame now has a WebFrameScheduler, with lazily created
loading and timer queues.  Currently all these queues share the same
priority so the task execution order remains the same.  However this
patch makes it possible for the scheduler to take control later.

Original patch by Alex Clarke <alexclarke@chromium.org>;

BUG=510398
TBR=jochen@chromium.org

Committed: https://crrev.com/ba3f8bd1d44fbb135c1c35e917240915a6a95985
Cr-Commit-Position: refs/heads/master@{#350813}

[Sami, 2015-09-17 14:10:08]

skyostil@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Sami, 2015-09-17 14:20:15]

Ross, mind having a look while Alex is out?

[rmcilroy, 2015-09-18 13:15:47]

Looks decent, a couple of comments though.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper_unittest.cc:186: namespace {
nit - newline below "namespace {" and above closing bracket.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/task_queue_manager.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager.cc:89:
observer_->OnUnregisterTaskQueue(task_queue);
Rather than adding this observer interface chain, would it not be simpler to
have RendererScheduler::UnregisterTaskRunner(...) and have the blink side call
that and it would call TaskQueueManager::UnregisterTaskQueue, then the observer
shouldn't be necessary since the scheduler is the entry point of this call?

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager_unittest.cc:1269: namespace {
ditto

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:57:
default_loading_task_runner_ = NewLoadingTaskRunner("loading_tq");
nit - "default_loading_tq" and same for timer_tq below?

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:195: }
Should we DCHECK that we find it in one or the other of loading / timer task
queues?

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:761: for (const auto&
runner : timer_task_runners_) {
Can you make this for loop only run in DEBUG since it is only used for DCHECKs

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:217:
std::set<scoped_refptr<TaskQueue>> timer_task_runners_;
Any reason for this to be a set rather than a vector?

[Sami, 2015-09-21 10:52:36]

Thanks for the comments!

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/scheduler_helper_unittest.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper_unittest.cc:186: namespace {
On 2015/09/18 13:15:46, rmcilroy wrote:
> nit - newline below "namespace {" and above closing bracket.

Done.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/task_queue_manager.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager.cc:89:
observer_->OnUnregisterTaskQueue(task_queue);
On 2015/09/18 13:15:46, rmcilroy wrote:
> Rather than adding this observer interface chain, would it not be simpler to
> have RendererScheduler::UnregisterTaskRunner(...) and have the blink side call
> that and it would call TaskQueueManager::UnregisterTaskQueue, then the
observer
> shouldn't be necessary since the scheduler is the entry point of this call?

That was the other alternative we thought about. The problem is that there's an
Unregister() on the TaskQueue which anyone could call. In order to hide that
method we'd need to have a forwarding class like RendererTaskQueue that
delegates everything to the real task. We decided the observer chain would be
the lesser evil but I think I could be convinced to have a forwarding class
instead. WDYT?

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager_unittest.cc:1269: namespace {
On 2015/09/18 13:15:46, rmcilroy wrote:
> ditto

Done.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:57:
default_loading_task_runner_ = NewLoadingTaskRunner("loading_tq");
On 2015/09/18 13:15:46, rmcilroy wrote:
> nit - "default_loading_tq" and same for timer_tq below?

Good idea, done.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:195: }
On 2015/09/18 13:15:46, rmcilroy wrote:
> Should we DCHECK that we find it in one or the other of loading / timer task
> queues?

I had exactly the same thought first, but the problem is all task queues  get
unregistered at teardown, hitting that DCHECK.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:761: for (const auto&
runner : timer_task_runners_) {
On 2015/09/18 13:15:46, rmcilroy wrote:
> Can you make this for loop only run in DEBUG since it is only used for DCHECKs

Sure. I was hoping the compiler would do that for me but no harm in being
explicit.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:217:
std::set<scoped_refptr<TaskQueue>> timer_task_runners_;
On 2015/09/18 13:15:46, rmcilroy wrote:
> Any reason for this to be a set rather than a vector?

I think Alex wanted to keep the removals efficient -- but I doubt in practice it
makes a difference either way. I'm leaning toward keeping these as sets since it
we don't want any duplicates in there -- but happy to turn into vectors is you
prefer?

(unordered_set is still banned it seems.)

[rmcilroy, 2015-09-23 09:44:25]

lgtm, thanks!

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
File components/scheduler/child/task_queue_manager.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager.cc:89:
observer_->OnUnregisterTaskQueue(task_queue);
On 2015/09/21 10:52:36, Sami wrote:
> On 2015/09/18 13:15:46, rmcilroy wrote:
> > Rather than adding this observer interface chain, would it not be simpler to
> > have RendererScheduler::UnregisterTaskRunner(...) and have the blink side
call
> > that and it would call TaskQueueManager::UnregisterTaskQueue, then the
> observer
> > shouldn't be necessary since the scheduler is the entry point of this call?
> 
> That was the other alternative we thought about. The problem is that there's
an
> Unregister() on the TaskQueue which anyone could call. In order to hide that
> method we'd need to have a forwarding class like RendererTaskQueue that
> delegates everything to the real task. We decided the observer chain would be
> the lesser evil but I think I could be convinced to have a forwarding class
> instead. WDYT?

Ahh, I didn't realize Unregister() was a TaskQueue method.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:195: }
On 2015/09/21 10:52:36, Sami wrote:
> On 2015/09/18 13:15:46, rmcilroy wrote:
> > Should we DCHECK that we find it in one or the other of loading / timer task
> > queues?
> 
> I had exactly the same thought first, but the problem is all task queues  get
> unregistered at teardown, hitting that DCHECK.

Acknowledged.

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1340343003/diff/120001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.h:217:
std::set<scoped_refptr<TaskQueue>> timer_task_runners_;
On 2015/09/21 10:52:36, Sami wrote:
> On 2015/09/18 13:15:46, rmcilroy wrote:
> > Any reason for this to be a set rather than a vector?
> 
> I think Alex wanted to keep the removals efficient -- but I doubt in practice
it
> makes a difference either way. I'm leaning toward keeping these as sets since
it
> we don't want any duplicates in there -- but happy to turn into vectors is you
> prefer?
> 
> (unordered_set is still banned it seems.)

This is fine.

[Sami, 2015-09-23 09:49:12]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-09-23 09:49:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/140001

[commit-bot: I haz the power, 2015-09-23 10:58:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-23 10:58:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sami, 2015-09-23 11:07:33]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-09-23 11:07:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1340343003/#ps160001
(title: "Windows build fix.")

[commit-bot: I haz the power, 2015-09-23 11:07:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/160001

[commit-bot: I haz the power, 2015-09-23 11:17:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-23 11:17:29]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2015-09-23 11:25:42]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-09-23 11:25:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1340343003/#ps180001
(title: "Another build fix.")

[commit-bot: I haz the power, 2015-09-23 11:26:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/180001

[commit-bot: I haz the power, 2015-09-23 12:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-23 12:41:57]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Sami, 2015-09-23 16:16:05]

Patchset #10 (id:180001) has been deleted

[Sami, 2015-09-23 16:16:11]

Patchset #9 (id:160001) has been deleted

[Sami, 2015-09-23 16:49:32]

So it turns out this patch failed to link on Windows because nothing was
including the new WebFrame{Host}Scheduler.h headers in Blink. To avoid adding
temporary hacks to make sure all symbols are defined, I decided to take
advantage of the brave new world we live in and roll
https://codereview.chromium.org/1334243004/ (which was already reviewed by
jochen@) into this patch.

[Sami, 2015-09-23 16:49:40]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-09-23 16:49:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1340343003/#ps200001
(title: "Merge with https://codereview.chromium.org/1334243004/")

[commit-bot: I haz the power, 2015-09-23 16:49:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/200001

[commit-bot: I haz the power, 2015-09-23 17:37:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-23 17:37:41]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sami, 2015-09-23 18:33:01]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-09-23 18:37:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/200001

[commit-bot: I haz the power, 2015-09-23 20:02:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-23 20:02:32]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sami, 2015-09-24 09:40:43]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-09-24 09:41:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/200001

[commit-bot: I haz the power, 2015-09-24 10:21:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 10:21:55]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sami, 2015-09-24 13:55:52]

skyostil@chromium.org changed reviewers:
+ sandersd@chromium.org

[Sami, 2015-09-24 13:56:22]

sandersd@, could you please check the test runner changes under media/?

[sandersd (OOO until July 31), 2015-09-25 00:27:12]

On 2015/09/24 13:56:22, Sami wrote:
> sandersd@, could you please check the test runner changes under media/?

media/ changes LGTM, although we are not supposed to depend on components/. I
wish there was a way to say 'only for testing' in DEPS.

[Sami, 2015-09-25 11:47:44]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-09-25 11:47:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1340343003/#ps320001
(title: "Rebased + fixed gn build.")

[commit-bot: I haz the power, 2015-09-25 11:48:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1340343003/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1340343003/320001

[commit-bot: I haz the power, 2015-09-25 13:26:36]

Committed patchset #15 (id:320001)

[commit-bot: I haz the power, 2015-09-25 13:27:09]

Patchset 15 (id:??) landed as
https://crrev.com/ba3f8bd1d44fbb135c1c35e917240915a6a95985
Cr-Commit-Position: refs/heads/master@{#350813}