[content] Add support for Non nestable idle tasks the the RendererScheduler

[content] Add support for Non nestable idle tasks the the RendererScheduler

Adds support for PostNonNestableIdleTask to the SingleThreadIdleTaskRunner.
To do this the TaskQueueManager is changed to query whether it is running
on a nested message loop and only re-post nested tasks if it is running
in a nested run loop.

Blink side of this patch is: https://codereview.chromium.org/976723002/

BUG=455713
Committed: https://crrev.com/c9df442b1d32b8bfd16b40b57eaa412047f6fa55
Cr-Commit-Position: refs/heads/master@{#320575}

[rmcilroy, 2015-03-03 19:32:56]

Patchset #2 (id:20001) has been deleted

[rmcilroy, 2015-03-03 19:33:45]

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

[rmcilroy, 2015-03-03 19:33:46]

Sami: Could you take a look please, thanks.

[Sami, 2015-03-04 12:19:53]

Functionally looks great but I think the callback business is a little
unfortunate. I bet you considered these already, but how about either just
tracking if DoWork itself is nested, or making TQM check the MessageLoop for the
current thread automatically if one exists?

[rmcilroy, 2015-03-05 12:22:21]

On 2015/03/04 12:19:53, Sami wrote:
> Functionally looks great but I think the callback business is a little
> unfortunate. I bet you considered these already, but how about either just
> tracking if DoWork itself is nested,

This wouldn't work if we were in a nested task which was posted directly to the
message loop (not through the scheduler) unfortunately.

> or making TQM check the MessageLoop for the
> current thread automatically if one exists?

This might work (although I'm not sure if it would work for tests which don't
use a message loop), however it makes me very queasy to rely on checking
something which we may not be running on with no real method of specifying it at
initialization time (i.e., there is nothing in the TQM right now which says way
are running on a message loop, and this would add a hidden assumption buried
deep in the code if we did this without the callback.

WDYT?

[Sami, 2015-03-05 12:49:31]

On 2015/03/05 12:22:21, rmcilroy wrote:
> On 2015/03/04 12:19:53, Sami wrote:
> > Functionally looks great but I think the callback business is a little
> > unfortunate. I bet you considered these already, but how about either just
> > tracking if DoWork itself is nested,
> 
> This wouldn't work if we were in a nested task which was posted directly to
the
> message loop (not through the scheduler) unfortunately.

Right. I'm guessing that shouldn't really happen but we have no way of actually
enforcing that so it wouldn't be great.
 
> > or making TQM check the MessageLoop for the
> > current thread automatically if one exists?
> 
> This might work (although I'm not sure if it would work for tests which don't
> use a message loop), however it makes me very queasy to rely on checking
> something which we may not be running on with no real method of specifying it
at
> initialization time (i.e., there is nothing in the TQM right now which says
way
> are running on a message loop, and this would add a hidden assumption buried
> deep in the code if we did this without the callback.
> 
> WDYT?

Yeah, implicit dependencies can be evil. How about this: let's add a
TaskQueueManagerDelegate interface (or something to that effect) that has the
method for checking the nesting level and then an implementation of that that
uses the message loop and possibly another one for testing. That should bring a
little structure and avoid having to pass 7 callbacks to the constructors down
the road.

[rmcilroy, 2015-03-12 17:40:47]

On 2015/03/05 12:49:31, Sami wrote:
> On 2015/03/05 12:22:21, rmcilroy wrote:
> > On 2015/03/04 12:19:53, Sami wrote:
> > > Functionally looks great but I think the callback business is a little
> > > unfortunate. I bet you considered these already, but how about either just
> > > tracking if DoWork itself is nested,
> > 
> > This wouldn't work if we were in a nested task which was posted directly to
> the
> > message loop (not through the scheduler) unfortunately.
> 
> Right. I'm guessing that shouldn't really happen but we have no way of
actually
> enforcing that so it wouldn't be great.
>  
> > > or making TQM check the MessageLoop for the
> > > current thread automatically if one exists?
> > 
> > This might work (although I'm not sure if it would work for tests which
don't
> > use a message loop), however it makes me very queasy to rely on checking
> > something which we may not be running on with no real method of specifying
it
> at
> > initialization time (i.e., there is nothing in the TQM right now which says
> way
> > are running on a message loop, and this would add a hidden assumption buried
> > deep in the code if we did this without the callback.
> > 
> > WDYT?
> 
> Yeah, implicit dependencies can be evil. How about this: let's add a
> TaskQueueManagerDelegate interface (or something to that effect) that has the
> method for checking the nesting level and then an implementation of that that
> uses the message loop and possibly another one for testing. That should bring
a
> little structure and avoid having to pass 7 callbacks to the constructors down
> the road.

Reimplemented with a delagate as discussed offline. PTAL.

[Sami, 2015-03-12 18:53:11]

Thanks, lgtm, looks pretty neat now.

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:13: #include
"content/renderer/scheduler/nestable_single_thread_task_runner.h"
nit: Can you forward declare this instead?

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:76:
DCHECK(!mock_task_runner_.get() || !message_loop_.get());
Missed a golden opportunity to use ^ :)

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager.h:18: #include
"content/renderer/scheduler/nestable_single_thread_task_runner.h"
Forward decl here too?

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/web_scheduler_impl.cc (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/web_scheduler_impl.cc:53: tracked_objects::Location
location(web_location.functionName(),
Future cleanup: add a helper to convert a WebTraceLocation to a TraceLocation.

[rmcilroy, 2015-03-13 10:56:53]

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:13: #include
"content/renderer/scheduler/nestable_single_thread_task_runner.h"
On 2015/03/12 18:53:11, Sami wrote:
> nit: Can you forward declare this instead?

Yup, done.

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:76:
DCHECK(!mock_task_runner_.get() || !message_loop_.get());
On 2015/03/12 18:53:11, Sami wrote:
> Missed a golden opportunity to use ^ :)

Ahh the seldom used bitwise XOR operator.  I think I will save my XOR allowance
for another time ;).

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager.h (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager.h:18: #include
"content/renderer/scheduler/nestable_single_thread_task_runner.h"
On 2015/03/12 18:53:11, Sami wrote:
> Forward decl here too?

Done.

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
File content/renderer/scheduler/web_scheduler_impl.cc (right):

https://codereview.chromium.org/970473004/diff/70001/content/renderer/schedul...
content/renderer/scheduler/web_scheduler_impl.cc:53: tracked_objects::Location
location(web_location.functionName(),
On 2015/03/12 18:53:11, Sami wrote:
> Future cleanup: add a helper to convert a WebTraceLocation to a TraceLocation.

Acknowledged.

[rmcilroy, 2015-03-13 10:56:58]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-03-13 10:56:58]

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/970473004/#ps90001 (title:
"Address Sami's comments")

[commit-bot: I haz the power, 2015-03-13 10:57:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/970473004/90001

[commit-bot: I haz the power, 2015-03-13 12:34:36]

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

[commit-bot: I haz the power, 2015-03-13 12:34:37]

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

[rmcilroy, 2015-03-13 13:54:26]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-03-13 13:54:26]

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/970473004/#ps110001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-13 13:54:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/970473004/110001

[commit-bot: I haz the power, 2015-03-13 16:19:30]

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

[commit-bot: I haz the power, 2015-03-13 16:19:31]

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

[rmcilroy, 2015-03-13 19:26:08]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-03-13 19:26:09]

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/970473004/#ps130001
(title: "Fix contentperftests")

[commit-bot: I haz the power, 2015-03-13 19:26:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/970473004/130001

[commit-bot: I haz the power, 2015-03-13 21:22:38]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2015-03-13 21:23:21]

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