Implement WebFrameScheduler and WebPageScheduler

Implement WebFrameScheduler and WebPageScheduler

The initial implelentation creates all the task queues but ignores
setPageInBackground and setFrameVisible.

Initial blink patch: https://codereview.chromium.org/1316233010/

BUG=510398

[alex clarke (OOO till 29th), 2015-09-03 17:04:25]

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

[alex clarke (OOO till 29th), 2015-09-08 16:32:33]

The diff base for renderer_scheduler_impl.cc is slightly weird due to a
non-trivial rebase.  This wil be fixed soon once the UseCase refactor is
submitted.

[alex clarke (OOO till 29th), 2015-09-10 11:13:13]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-10 11:13:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1314903007/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1314903007/60001

[alex clarke (OOO till 29th), 2015-09-10 11:27:43]

I've fixed the diffbase.

[Sami, 2015-09-10 12:51:58]

Looks pretty good. I like the idea of keeping the origin distinct from the queue
name.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.h (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:196: std::string origin_;
Looks like origin_ is protected by the lock but it's not part of the group
above.

By the way, why do we need it to be thread safe?

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager.cc (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
components/scheduler/child/task_queue_manager.cc:345: "Run Task From Queue",
"queue", queue->GetName(),
Since you're touching this, could you change this to something like
TaskQueueManager::RunTask to match normal naming style? (I thought we already
did that in an earlier code review)

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager.h (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
components/scheduler/child/task_queue_manager.h:95: void
UnregisterTaskQueue(scoped_refptr<internal::TaskQueueImpl> task_queue);
Could this method be on the TaskQueue instead?

nit: the description doesn't really say what this function does from the
perspective of the caller.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:187:
helper_.UnregisterTaskQueue(task_queue);
Should we be removing the observers here too?

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_frame_scheduler_impl.cc (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.cc:22: visible_(false) {}
|true| would probably be a safer default.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_page_scheduler_impl.h (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_page_scheduler_impl.h:23: class
SCHEDULER_EXPORT WebPageSchedulerImpl : public blink::WebPageScheduler {
If we find out that Page isn't the right object to attach this to, perhaps
WebViewScheduler would be another option.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_page_scheduler_impl.h:32: void
Unregister(WebFrameSchedulerImpl* frame_scheduler);
nit: add a blank line before this.

[alex clarke (OOO till 29th), 2015-09-10 14:35:58]

PTAL

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
File components/scheduler/child/task_queue_impl.h (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
components/scheduler/child/task_queue_impl.h:196: std::string origin_;
On 2015/09/10 12:51:57, Sami wrote:
> Looks like origin_ is protected by the lock but it's not part of the group
> above.
> 
> By the way, why do we need it to be thread safe?

Done.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager.cc (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
components/scheduler/child/task_queue_manager.cc:345: "Run Task From Queue",
"queue", queue->GetName(),
On 2015/09/10 12:51:57, Sami wrote:
> Since you're touching this, could you change this to something like
> TaskQueueManager::RunTask to match normal naming style? (I thought we already
> did that in an earlier code review)

I'm not sure what happened there, I thought we had too :(  

DONE.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
File components/scheduler/child/task_queue_manager.h (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/ch...
components/scheduler/child/task_queue_manager.h:95: void
UnregisterTaskQueue(scoped_refptr<internal::TaskQueueImpl> task_queue);
On 2015/09/10 12:51:58, Sami wrote:
> Could this method be on the TaskQueue instead?
> 
> nit: the description doesn't really say what this function does from the
> perspective of the caller.

Yes, DONE.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_frame_scheduler_impl.cc (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_frame_scheduler_impl.cc:22: visible_(false) {}
On 2015/09/10 12:51:58, Sami wrote:
> |true| would probably be a safer default.

Done.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
File components/scheduler/renderer/web_page_scheduler_impl.h (right):

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_page_scheduler_impl.h:23: class
SCHEDULER_EXPORT WebPageSchedulerImpl : public blink::WebPageScheduler {
On 2015/09/10 12:51:58, Sami wrote:
> If we find out that Page isn't the right object to attach this to, perhaps
> WebViewScheduler would be another option.

Acknowledged.

https://codereview.chromium.org/1314903007/diff/80001/components/scheduler/re...
components/scheduler/renderer/web_page_scheduler_impl.h:32: void
Unregister(WebFrameSchedulerImpl* frame_scheduler);
On 2015/09/10 12:51:58, Sami wrote:
> nit: add a blank line before this.

Done.

[Sami, 2015-09-10 15:10:23]

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.cc:129: if (task_queue_manager_)
I think DCHECKing that we still have a manager would lead to fewer surprises.

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.cc:130:
task_queue_manager_->SetQueueObserver(this);
Should probably remove this in the destructor.

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
File components/scheduler/child/scheduler_helper.h (right):

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.h:79: // thread. If |observer| is
null, then no callbacks will occur.
nit: mention that the observer should outlive this class.

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper_unittest.cc:195: MockObserver
observer;
Hmm, how does this work -- shouldn't the observer be registered somewhere?

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager.h:101: // thread. If |observer| is
null, then no callbacks will occur.
nit: also mention the lifetime here.

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager_unittest.cc:1251: MockObserver
observer;
SetObserver call missing here too?

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/r...
components/scheduler/renderer/web_frame_scheduler_impl.cc:15: class
FrameQueueHolder;
Unused?

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/r...
components/scheduler/renderer/web_page_scheduler_impl.h:23: class
SCHEDULER_EXPORT WebPageSchedulerImpl : public blink::WebPageScheduler {
Is the contract that PageScheduler must outlive all the FrameSchedulers that it
creates? Or do we need to support ones that outlive the PageScheduler? Might
want to add dchecks either way.

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/r...
components/scheduler/renderer/web_page_scheduler_impl.h:36:
std::set<WebFrameSchedulerImpl*> frame_schedulers_;
Do we need this set for anything?

[alex clarke (OOO till 29th), 2015-09-10 15:56:24]

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.cc:129: if (task_queue_manager_)
On 2015/09/10 15:10:23, Sami wrote:
> I think DCHECKing that we still have a manager would lead to fewer surprises.

Done.

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.cc:130:
task_queue_manager_->SetQueueObserver(this);
On 2015/09/10 15:10:23, Sami wrote:
> Should probably remove this in the destructor.

Not sure that's worth doing since this class owns task_queue_manager_.  WDYT?

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
File components/scheduler/child/scheduler_helper.h (right):

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.h:79: // thread. If |observer| is
null, then no callbacks will occur.
On 2015/09/10 15:10:23, Sami wrote:
> nit: mention that the observer should outlive this class.

Done.

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper_unittest.cc:195: MockObserver
observer;
On 2015/09/10 15:10:23, Sami wrote:
> Hmm, how does this work -- shouldn't the observer be registered somewhere?

Yeah that was just completely broken :(

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager.h:101: // thread. If |observer| is
null, then no callbacks will occur.
On 2015/09/10 15:10:23, Sami wrote:
> nit: also mention the lifetime here.

Done.

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/task_queue_manager_unittest.cc:1251: MockObserver
observer;
On 2015/09/10 15:10:23, Sami wrote:
> SetObserver call missing here too?

Done.

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/r...
components/scheduler/renderer/web_frame_scheduler_impl.cc:15: class
FrameQueueHolder;
On 2015/09/10 15:10:23, Sami wrote:
> Unused?

Done.

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/r...
components/scheduler/renderer/web_page_scheduler_impl.h:23: class
SCHEDULER_EXPORT WebPageSchedulerImpl : public blink::WebPageScheduler {
On 2015/09/10 15:10:23, Sami wrote:
> Is the contract that PageScheduler must outlive all the FrameSchedulers that
it
> creates?

That's what I'm assuming.

 Or do we need to support ones that outlive the PageScheduler? Might
> want to add dchecks either way.

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/r...
components/scheduler/renderer/web_page_scheduler_impl.h:36:
std::set<WebFrameSchedulerImpl*> frame_schedulers_;
On 2015/09/10 15:10:23, Sami wrote:
> Do we need this set for anything?

We will do if we want  setPageInBackground() to throttle / unthrottle background
task queues.

[Sami, 2015-09-11 11:34:43]

Thanks, lg2m once we rename WebPageScheduler to WebFrameHostScheduler (assuming
the blink folks agree).

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

https://codereview.chromium.org/1314903007/diff/120001/components/scheduler/c...
components/scheduler/child/scheduler_helper.cc:130:
task_queue_manager_->SetQueueObserver(this);
On 2015/09/10 15:56:24, alexclarke1 wrote:
> On 2015/09/10 15:10:23, Sami wrote:
> > Should probably remove this in the destructor.
> 
> Not sure that's worth doing since this class owns task_queue_manager_.  WDYT?

Ah right I didn't notice that. Usually I like to have explicit calls but this
works too.

https://codereview.chromium.org/1314903007/diff/140001/components/scheduler/r...
File components/scheduler/renderer/web_page_scheduler_impl.cc (right):

https://codereview.chromium.org/1314903007/diff/140001/components/scheduler/r...
components/scheduler/renderer/web_page_scheduler_impl.cc:17: << "We expect
WebPageSchedulerImpl to out live it's WebFrameSchedulers";
super-nit: s/it's/its/