[scheduler] Remove blink::scheduler::TaskQueue from public API.

[scheduler] Remove blink::scheduler::TaskQueue from public API.

Use base::SingleThreadTaskRunner to expose task runners from
ChildScheduler and RendererScheduler to content.

Rename {Timer}TaskRunner to {Timer}TaskQueue for methods which return
TaskQueue (internal to blink).

R=haraken@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
BUG=710063
Review-Url: https://codereview.chromium.org/2812313002
Cr-Commit-Position: refs/heads/master@{#466133}
Committed: https://chromium.googlesource.com/chromium/src/+/df8804b62d42666ac585a090d745c09462ffe54e

[altimin, 2017-04-12 16:14:29]

PTAL

[altimin, 2017-04-12 16:14:44]

Description was changed from

==========
[scheduler] Remove blink::scheduler::TaskQueue from public API.

Use base::SingleThreadTaskRunner to expose task runners from
ChildScheduler and RendererScheduler to content.

Rename {Timer}TaskRunner to {Timer}TaskQueue for methods which return
TaskQueue (internal to blink).

R=haraken@chromium.org,skyosil@chromium.org,alexclarke@chromium.org
BUG=710063
==========

to

==========
[scheduler] Remove blink::scheduler::TaskQueue from public API.

Use base::SingleThreadTaskRunner to expose task runners from
ChildScheduler and RendererScheduler to content.

Rename {Timer}TaskRunner to {Timer}TaskQueue for methods which return
TaskQueue (internal to blink).

R=haraken@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
BUG=710063
==========

[altimin, 2017-04-12 16:14:44]

altimin@chromium.org changed reviewers:
+ skyostil@chromium.org
- skyosil@chromium.org

[altimin, 2017-04-12 16:15:02]

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

[commit-bot: I haz the power, 2017-04-12 16:15:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[altimin, 2017-04-12 16:24:05]

Mistyped Sami's login. :(
+Sami

[commit-bot: I haz the power, 2017-04-12 16:31:53]

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

[commit-bot: I haz the power, 2017-04-12 16:31:54]

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

[Sami, 2017-04-12 16:45:21]

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:48: //
Returns the default task runner.
Please update these comments.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc:67:
task_runner_ = worker_scheduler_->DefaultTaskQueue();
Could we either rename all of these or none of these? This leaves us at a weird
halfway point.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/worker_scheduler.h:29:
virtual scoped_refptr<TaskQueue> DefaultTaskQueue() = 0;
Hmm, why?-) And how do we make sure someone doesn't get this wrong?

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:147:
// Blink clients should use RendereSchedulerImpl::*TaskQueue
This seems a little too confusing. Could we figure out a better interface?

[altimin, 2017-04-12 17:02:08]

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

[altimin, 2017-04-12 17:02:26]

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:48: //
Returns the default task runner.
On 2017/04/12 16:45:21, Sami wrote:
> Please update these comments.

Done.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc:67:
task_runner_ = worker_scheduler_->DefaultTaskQueue();
On 2017/04/12 16:45:21, Sami wrote:
> Could we either rename all of these or none of these? This leaves us at a
weird
> halfway point.

I believe that it's mostly consistent (there will be a couple more patches
polishing everything).

*TaskQueue methods return TaskQueue and *TaskRunner methods return task runner.
In this case it's SingleThreadIdleTaskRunner (which is exposed to content/).

The thing happening here is that IdleTaskRunner() is a very different thing (I
wonder if we should change it) and this change only highlights existing pattern.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/scheduler/child/worker_scheduler.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/worker_scheduler.h:29:
virtual scoped_refptr<TaskQueue> DefaultTaskQueue() = 0;
On 2017/04/12 16:45:21, Sami wrote:
> Hmm, why?-) And how do we make sure someone doesn't get this wrong?

They're essentially the same, TaskQueue is a subclass of SingleThreadTaskRunner.
TaskQueue exposes more blink-specific methods and there are a lot of places in
blink where task queue is expected. This is pretty impossible to get wrong
(compiler will enforce it)

And scheduler->DefaultTaskQueue()->PostTask and
scheduler->DefaultTaskRunner()->PostTask do exactly the same thing.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:147:
// Blink clients should use RendereSchedulerImpl::*TaskQueue
On 2017/04/12 16:45:21, Sami wrote:
> This seems a little too confusing. Could we figure out a better interface?

Given that these methods are essentially the same (like GetRendererScheduler vs
GetRendererSchedulerImpl), one could think that it's fine.

[commit-bot: I haz the power, 2017-04-12 17:03:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sami, 2017-04-12 17:42:03]

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc:67:
task_runner_ = worker_scheduler_->DefaultTaskQueue();
On 2017/04/12 17:02:25, altimin wrote:
> On 2017/04/12 16:45:21, Sami wrote:
> > Could we either rename all of these or none of these? This leaves us at a
> weird
> > halfway point.
> 
> I believe that it's mostly consistent (there will be a couple more patches
> polishing everything).
> 
> *TaskQueue methods return TaskQueue and *TaskRunner methods return task
runner.
> In this case it's SingleThreadIdleTaskRunner (which is exposed to content/).
> 
> The thing happening here is that IdleTaskRunner() is a very different thing (I
> wonder if we should change it) and this change only highlights existing
pattern.

I was mainly referring to the fact that the variable names don't match the
getters (queue vs. runner). Ideally we'd just talk about one kind of thing since
the distinction isn't really relevant at this level.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:147:
// Blink clients should use RendereSchedulerImpl::*TaskQueue
On 2017/04/12 17:02:25, altimin wrote:
> On 2017/04/12 16:45:21, Sami wrote:
> > This seems a little too confusing. Could we figure out a better interface?
> 
> Given that these methods are essentially the same (like GetRendererScheduler
vs
> GetRendererSchedulerImpl), one could think that it's fine.

Could we get away with just exposing TaskQueue here and have some other way of
letting content/ get at the underlying STTR? I think this is baking in a little
too many assumptions about how all this will be used.

[commit-bot: I haz the power, 2017-04-12 18:40:12]

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

[commit-bot: I haz the power, 2017-04-12 18:40:13]

Dry run: This issue passed the CQ dry run.

[altimin, 2017-04-13 12:40:17]

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

[commit-bot: I haz the power, 2017-04-13 12:40:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[altimin, 2017-04-13 12:41:50]

PTAL

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc:67:
task_runner_ = worker_scheduler_->DefaultTaskQueue();
On 2017/04/12 17:42:02, Sami wrote:
> On 2017/04/12 17:02:25, altimin wrote:
> > On 2017/04/12 16:45:21, Sami wrote:
> > > Could we either rename all of these or none of these? This leaves us at a
> > weird
> > > halfway point.
> > 
> > I believe that it's mostly consistent (there will be a couple more patches
> > polishing everything).
> > 
> > *TaskQueue methods return TaskQueue and *TaskRunner methods return task
> runner.
> > In this case it's SingleThreadIdleTaskRunner (which is exposed to content/).
> > 
> > The thing happening here is that IdleTaskRunner() is a very different thing
(I
> > wonder if we should change it) and this change only highlights existing
> pattern.
> 
> I was mainly referring to the fact that the variable names don't match the
> getters (queue vs. runner). Ideally we'd just talk about one kind of thing
since
> the distinction isn't really relevant at this level.

Member variable names will be updated in a separate CL.

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
(right):

https://codereview.chromium.org/2812313002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:147:
// Blink clients should use RendereSchedulerImpl::*TaskQueue
On 2017/04/12 17:42:02, Sami wrote:
> On 2017/04/12 17:02:25, altimin wrote:
> > On 2017/04/12 16:45:21, Sami wrote:
> > > This seems a little too confusing. Could we figure out a better interface?
> > 
> > Given that these methods are essentially the same (like GetRendererScheduler
> vs
> > GetRendererSchedulerImpl), one could think that it's fine.
> 
> Could we get away with just exposing TaskQueue here and have some other way of
> letting content/ get at the underlying STTR? I think this is baking in a
little
> too many assumptions about how all this will be used.

Moved overriden methods to protected section.

[commit-bot: I haz the power, 2017-04-13 12:44:20]

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

[commit-bot: I haz the power, 2017-04-13 12:44:21]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Sami, 2017-04-19 17:19:43]

lgtm

https://codereview.chromium.org/2812313002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
(right):

https://codereview.chromium.org/2812313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:148:
// Returns a new loading task runner. This queue is intended for tasks related
s/runner/queue/

https://codereview.chromium.org/2812313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:152:
// Returns a new timer task runner. This queue is intended for DOM Timers.
nit: we use this for way more than just DOM timers.

[altimin, 2017-04-20 17:01:17]

Kentaro-san, can you please take a look?

https://codereview.chromium.org/2812313002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h
(right):

https://codereview.chromium.org/2812313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:148:
// Returns a new loading task runner. This queue is intended for tasks related
On 2017/04/19 17:19:43, Sami wrote:
> s/runner/queue/

Done.

https://codereview.chromium.org/2812313002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h:152:
// Returns a new timer task runner. This queue is intended for DOM Timers.
On 2017/04/19 17:19:43, Sami wrote:
> nit: we use this for way more than just DOM timers.

But it is _intended_ for DOM timers. We can use it for things that look like DOM
timers and can be treated as such.

[haraken, 2017-04-20 17:29:09]

LGTM

[altimin, 2017-04-20 17:33:41]

The CQ bit was checked by altimin@chromium.org

[altimin, 2017-04-20 17:33:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/2812313002/#ps60001
(title: "Rebased & addressed skyostil@'s comment")

[commit-bot: I haz the power, 2017-04-20 17:34:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-20 21:13:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492709621967040,
"parent_rev": "51afc830d2cb4e3c15efb393ab8c8bb689159d20", "commit_rev":
"df8804b62d42666ac585a090d745c09462ffe54e"}

[commit-bot: I haz the power, 2017-04-20 21:13:55]

Description was changed from

==========
[scheduler] Remove blink::scheduler::TaskQueue from public API.

Use base::SingleThreadTaskRunner to expose task runners from
ChildScheduler and RendererScheduler to content.

Rename {Timer}TaskRunner to {Timer}TaskQueue for methods which return
TaskQueue (internal to blink).

R=haraken@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
BUG=710063
==========

to

==========
[scheduler] Remove blink::scheduler::TaskQueue from public API.

Use base::SingleThreadTaskRunner to expose task runners from
ChildScheduler and RendererScheduler to content.

Rename {Timer}TaskRunner to {Timer}TaskQueue for methods which return
TaskQueue (internal to blink).

R=haraken@chromium.org,skyostil@chromium.org,alexclarke@chromium.org
BUG=710063

Review-Url: https://codereview.chromium.org/2812313002
Cr-Commit-Position: refs/heads/master@{#466133}
Committed:
https://chromium.googlesource.com/chromium/src/+/df8804b62d42666ac585a090d745...
==========

[commit-bot: I haz the power, 2017-04-20 21:13:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/df8804b62d42666ac585a090d745...