scheduler: Implement task observers

scheduler: Implement task observers

This patch adds a way for scheduler clients to get notified of task
execution, much like with base::MessageLoop. This is needed to allow
the execution of microtask checkpoints[1] between tasks by Blink while
letting the scheduler use a >1 work batch size.

This patch also refactors the various WebThreadImpls to reduce code
duplication removes the deprecated postTask entrypoints that don't take
the posting location as a parameter.

BUG=450977, 444764, 458990
[1] https://html.spec.whatwg.org/multipage/webappapis.html#processing-model-9

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

Committed: https://crrev.com/0da8dc886fad453b0cf4f1c718da8d44e132b618
Cr-Commit-Position: refs/heads/master@{#317852}

[Sami, 2015-02-12 16:53:40]

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

[Sami, 2015-02-12 16:53:41]

PTAL. I'm working on another patch on top of this that coalesces some of the
duplicated code between the various WebThreads.

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

https://codereview.chromium.org/922733002/diff/1/content/child/webthread_impl.cc
File content/child/webthread_impl.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/child/webthread_impl...
content/child/webthread_impl.cc:61:
base::MessageLoop::current()->AddTaskObserver(observer);
Do we need DCHECK(main_thread_checker_.CalledOnValidThread());

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:417:
FOR_EACH_OBSERVER(base::MessageLoop::TaskObserver, task_observers_,
Aren't we risking a scenario like this:

base::MessageLoop::RunTask
FOR_EACH_OBSERVER(...)
task_annotator_.RunTask()
    ...
    TaskQueueManager::ProcessTaskFromWorkQueue
    FOR_EACH_OBSERVER(...)
    task_annotator_.RunTask()
        actual task runs
    FOR_EACH_OBSERVER(...)

    x 1-4
FOR_EACH_OBSERVER(...)

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager_unittest.cc:622: EXPECT_EQ(2,
observer.CallsToWillProcess());
Should this be split up into multiple tests so it's clearer what each test is
checking?

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:70:
WebThreadImplForScheduler::~WebThreadImplForScheduler() {
nit: Shouldn't this be after the constructor?

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
File content/renderer/scheduler/webthread_impl_for_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.h:20: virtual void
postTask(const blink::WebTraceLocation& location, Task* task);
Maybe add a comment above on the lines of: implements blink::WebThread

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.h:29: virtual void
enterRunLoop() override;
nit: only one of virtual & override

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.h:32: virtual bool
isCurrentThread() const override;
Maybe add a comment above on the lines of: implements WebThreadBase

[Sami, 2015-02-13 12:09:09]

Thanks Alex. All comments addressed.

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager.cc:417:
FOR_EACH_OBSERVER(base::MessageLoop::TaskObserver, task_observers_,
On 2015/02/13 at 11:02:07, alexclarke1 wrote:
> Aren't we risking a scenario like this:
> 
> base::MessageLoop::RunTask
> FOR_EACH_OBSERVER(...)
> task_annotator_.RunTask()
>     ...
>     TaskQueueManager::ProcessTaskFromWorkQueue
>     FOR_EACH_OBSERVER(...)
>     task_annotator_.RunTask()
>         actual task runs
>     FOR_EACH_OBSERVER(...)
> 
>     x 1-4
> FOR_EACH_OBSERVER(...)

We talked about this offline. It should be fine because we're intercepting all
observers from Blink and those should be the only ones that matter w.r.t.
microtasks.

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/t...
content/renderer/scheduler/task_queue_manager_unittest.cc:622: EXPECT_EQ(2,
observer.CallsToWillProcess());
On 2015/02/13 at 11:02:07, alexclarke1 wrote:
> Should this be split up into multiple tests so it's clearer what each test is
checking?

Good idea. I've split this into one test for adding and another test for
removing the observer.

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:70:
WebThreadImplForScheduler::~WebThreadImplForScheduler() {
On 2015/02/13 at 11:02:07, alexclarke1 wrote:
> nit: Shouldn't this be after the constructor?

Fixed.

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
File content/renderer/scheduler/webthread_impl_for_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.h:20: virtual void
postTask(const blink::WebTraceLocation& location, Task* task);
On 2015/02/13 at 11:02:07, alexclarke1 wrote:
> Maybe add a comment above on the lines of: implements blink::WebThread

Done.

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.h:29: virtual void
enterRunLoop() override;
On 2015/02/13 at 11:02:07, alexclarke1 wrote:
> nit: only one of virtual & override

Well spotted. I'll keep this is virtual instead of override since the base class
comes from Blink.

https://codereview.chromium.org/922733002/diff/1/content/renderer/scheduler/w...
content/renderer/scheduler/webthread_impl_for_scheduler.h:32: virtual bool
isCurrentThread() const override;
On 2015/02/13 at 11:02:07, alexclarke1 wrote:
> Maybe add a comment above on the lines of: implements WebThreadBase

Actually these also part of blink::WebThread. I've got a patch here that cleans
some of this up but I wanted to split it out to make this review clearer:
https://codereview.chromium.org/923973002

[rmcilroy, 2015-02-13 12:53:22]

Looks great, a couple of nits and a question.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/null_renderer_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/null_renderer_scheduler.cc:99:
base::MessageLoop::current()->RemoveTaskObserver(task_observer);
nit - move above shutdown

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler.h:74: // These functions can only
be called on the main thread.
nit - could you add a comment to explain when the task observer will be
triggered.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler.h:78:
base::MessageLoop::TaskObserver* task_observer) = 0;
nit - move above Shutdown()

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

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:48:
base::MessageLoop::TaskObserver* task_observer) override;
nit - move above shutdown

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager_unittest.cc:630:
manager_->RemoveTaskObserver(&observer);
nit - could you add a test where the observer is removed from within a task, and
ensure CallsToWillProcess incremented by CallsToDidProcess didn't (no idea if we
care about this, but seems like an edge case).

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:19:
WebThreadImplForScheduler::~WebThreadImplForScheduler() {
Could we just replace WebThreadImplForMessageLoop with this?  If not, could we
just extend it and override on Add/RemoveTaskObserverInternal?

https://codereview.chromium.org/922733002/diff/20001/content/test/fake_render...
File content/test/fake_renderer_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/20001/content/test/fake_render...
content/test/fake_renderer_scheduler.h:32: base::MessageLoop::TaskObserver*
task_observer) override;
nit - above Shutdown

[Sami, 2015-02-13 14:27:27]

Thanks Ross. I've pulled in the refactor I mentioned before. PTAL.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/null_renderer_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/null_renderer_scheduler.cc:99:
base::MessageLoop::current()->RemoveTaskObserver(task_observer);
On 2015/02/13 at 12:53:21, rmcilroy wrote:
> nit - move above shutdown

Done.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler.h:74: // These functions can only
be called on the main thread.
On 2015/02/13 at 12:53:21, rmcilroy wrote:
> nit - could you add a comment to explain when the task observer will be
triggered.

Sure thing, this comment was just copied verbatim from MessageLoop.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler.h:78:
base::MessageLoop::TaskObserver* task_observer) = 0;
On 2015/02/13 at 12:53:21, rmcilroy wrote:
> nit - move above Shutdown()

Done.

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

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.h:48:
base::MessageLoop::TaskObserver* task_observer) override;
On 2015/02/13 at 12:53:22, rmcilroy wrote:
> nit - move above shutdown

Done.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager_unittest.cc:630:
manager_->RemoveTaskObserver(&observer);
On 2015/02/13 at 12:53:22, rmcilroy wrote:
> nit - could you add a test where the observer is removed from within a task,
and ensure CallsToWillProcess incremented by CallsToDidProcess didn't (no idea
if we care about this, but seems like an edge case).

Good idea, done.

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:19:
WebThreadImplForScheduler::~WebThreadImplForScheduler() {
On 2015/02/13 at 12:53:22, rmcilroy wrote:
> Could we just replace WebThreadImplForMessageLoop with this?  If not, could we
just extend it and override on Add/RemoveTaskObserverInternal?

Ok, I'll roll my WebThread refactor into this patch since the end result is much
cleaner that way :)

https://codereview.chromium.org/922733002/diff/20001/content/test/fake_render...
File content/test/fake_renderer_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/20001/content/test/fake_render...
content/test/fake_renderer_scheduler.h:32: base::MessageLoop::TaskObserver*
task_observer) override;
On 2015/02/13 at 12:53:22, rmcilroy wrote:
> nit - above Shutdown

Done.

[alex clarke (OOO till 29th), 2015-02-13 14:42:19]

https://codereview.chromium.org/922733002/diff/60001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/60001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager_unittest.cc:591: void
WillProcessTask(const base::PendingTask& task) override {
This is OK however gmock does have native support for this kind of thing which
we should probably prefer.

class MockTaskObserver : public base::MessageLoop::TaskObserver {
  MOCK_METHOD1(DidProcessTask, void(const base::PendingTask& task));
  MOCK_METHOD1(WillProcessTask, void(const base::PendingTask& task));
};

...

TEST_F(TaskQueueManagerTest, TaskObserverAdding) {
  MockTaskObserver observer;
  ....

  EXPECT_CALL(observer, DidProcessTask(_)).times(2);
  EXPECT_CALL(observer, WillProcessTask(_)).times(2);
}

[Sami, 2015-02-13 15:54:31]

https://codereview.chromium.org/922733002/diff/60001/content/renderer/schedul...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/60001/content/renderer/schedul...
content/renderer/scheduler/task_queue_manager_unittest.cc:591: void
WillProcessTask(const base::PendingTask& task) override {
On 2015/02/13 14:42:18, alexclarke1 wrote:
> This is OK however gmock does have native support for this kind of thing which
> we should probably prefer.
> 
> class MockTaskObserver : public base::MessageLoop::TaskObserver {
>   MOCK_METHOD1(DidProcessTask, void(const base::PendingTask& task));
>   MOCK_METHOD1(WillProcessTask, void(const base::PendingTask& task));
> };
> 
> ...
> 
> TEST_F(TaskQueueManagerTest, TaskObserverAdding) {
>   MockTaskObserver observer;
>   ....
> 
>   EXPECT_CALL(observer, DidProcessTask(_)).times(2);
>   EXPECT_CALL(observer, WillProcessTask(_)).times(2);
> }

Thanks for the suggestion. It's a bit neater this way.

[alex clarke (OOO till 29th), 2015-02-13 15:58:21]

lgtm

[rmcilroy, 2015-02-13 16:14:01]

Looks great - only real comment is to do the task batching in a different CL
(and update description appropriately).

lgtm, thanks.

https://codereview.chromium.org/922733002/diff/80001/content/child/webthread_...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/922733002/diff/80001/content/child/webthread_...
content/child/webthread_impl.h:83: class WebThreadImplForMessageLoop : public
WebThreadBase {
comment: Don't do this for this change, but in a future change would we be able
to get rid of WebThreadImplForMessageLoop and/or replace WebThreadImpl with
WebThreadImplForMessageLoop - they don't seem significantly different

https://codereview.chromium.org/922733002/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/922733002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:51:
task_queue_manager_->SetWorkBatchSize(4);
Let's do this in a different patch (just in case we need to revert it again, at
least it will be easier to re-land).  Also remove the TODO above when you
actually do this.

[Sami, 2015-02-13 16:21:54]

New patchsets have been uploaded after l-g-t-m from
alexclarke@chromium.org,rmcilroy@chromium.org

[Sami, 2015-02-13 16:22:34]

Thanks!

https://codereview.chromium.org/922733002/diff/80001/content/child/webthread_...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/922733002/diff/80001/content/child/webthread_...
content/child/webthread_impl.h:83: class WebThreadImplForMessageLoop : public
WebThreadBase {
On 2015/02/13 16:14:00, rmcilroy wrote:
> comment: Don't do this for this change, but in a future change would we be
able
> to get rid of WebThreadImplForMessageLoop and/or replace WebThreadImpl with
> WebThreadImplForMessageLoop - they don't seem significantly different

I thought about that too, but there's actually a crucial difference: WebThread
owns a Thread and manages its lifetime but WebThreadImplForMessageLoop does not.
I couldn't think of a clean way to reconcile this so I left them separate.

https://codereview.chromium.org/922733002/diff/80001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/922733002/diff/80001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:51:
task_queue_manager_->SetWorkBatchSize(4);
On 2015/02/13 at 16:14:00, rmcilroy wrote:
> Let's do this in a different patch (just in case we need to revert it again,
at least it will be easier to re-land).  Also remove the TODO above when you
actually do this.

Oops, I wasn't actually meaning to do it here. My bad, thanks for spotting.

[Sami, 2015-02-13 18:23:08]

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

[Sami, 2015-02-13 18:23:09]

jam@, could you take a peek at these files:

    content/child/threaded_data_provider.cc
    content/child/webthread_impl.cc
    content/child/webthread_impl.h
    content/renderer/renderer_blink_platform_impl.cc
    content/renderer/renderer_blink_platform_impl.h

[jam, 2015-02-14 01:20:02]

rs lgtm

[Sami, 2015-02-16 10:38:32]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-02-16 10:39:26]

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

[commit-bot: I haz the power, 2015-02-16 11:37:55]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-16 11:38:29]

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

[Sami, 2015-02-16 14:41:28]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/930063002/ by skyostil@chromium.org.

The reason for reverting is: Broke a CrOS test:

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[Sami, 2015-02-18 18:47:48]

New patchsets have been uploaded after l-g-t-m from jam@chromium.org

[Sami, 2015-02-18 18:53:12]

The bug was that we were no longer notifying Blink of top-level tasks in the
MessageLoop which didn't go through the scheduler. I've fixed it by adding a
task observer which is triggered by both the MessageLoop and the scheduler (with
appropriate filtering of dupes). Ross and Alex, could you take a look? Check the
diff from 8 to 9 to see the changes.

[alex clarke (OOO till 29th), 2015-02-19 10:35:35]

lgtm

Just nits

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:49:
NestedTaskObserver* self_;
nit: it can't be 'self' since c++ inner classes are like static ones in java,
maybe 'parent' or 'owner' or 'nested_task_observer_'?

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc
(right):

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc:68: }
nit: stylistically it might be better to put this close bracket after the
EatDefaultTask call.

[Sami, 2015-02-19 11:24:55]

New patchsets have been uploaded after l-g-t-m from alexclarke@chromium.org

[Sami, 2015-02-19 11:28:41]

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:49:
NestedTaskObserver* self_;
On 2015/02/19 10:35:35, alexclarke1 wrote:
> nit: it can't be 'self' since c++ inner classes are like static ones in java,
> maybe 'parent' or 'owner' or 'nested_task_observer_'? 

Right, parent_ sgtm.

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc
(right):

https://codereview.chromium.org/922733002/diff/160001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc:68: }
On 2015/02/19 10:35:35, alexclarke1 wrote:
> nit: stylistically it might be better to put this close bracket after the
> EatDefaultTask call.

Done.

[rmcilroy, 2015-02-19 11:47:49]

One question about nested message loops, but otherwise looks good.

https://codereview.chromium.org/922733002/diff/200001/content/child/webthread...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/922733002/diff/200001/content/child/webthread...
content/child/webthread_impl.h:57: base::MessageLoop::TaskObserver* observer);
Can we remove the virtual AddTaskObserverInternal now since you are simply
overriding addTaskObserver/removeTaskObserver now?

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:12: // Observes tasks
both from a MessageLoop and a RendererScheduler which is
nit - newline above comment

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:69: if
(!inside_task_) {
How does this work with nested message loops?  It looks like we won't get task
observer notifications for any tasks in a nested message loop - is this the case
with the current message_loop implementation?

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.h:27: virtual void
removeTaskObserver(TaskObserver* observer);
Should these be marked override instead (they are overriding WebThreadBase, not
implementing blink::WebThread)?

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc
(right):

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc:14: const
int kWorkBatchSize = 2;
nit - newline above

[Sami, 2015-02-19 16:31:09]

Thanks Ross. Addressed the nested run loop case. PTAL.

https://codereview.chromium.org/922733002/diff/200001/content/child/webthread...
File content/child/webthread_impl.h (right):

https://codereview.chromium.org/922733002/diff/200001/content/child/webthread...
content/child/webthread_impl.h:57: base::MessageLoop::TaskObserver* observer);
On 2015/02/19 11:47:49, rmcilroy wrote:
> Can we remove the virtual AddTaskObserverInternal now since you are simply
> overriding addTaskObserver/removeTaskObserver now?

Ah yes, no need for that anymore. Done.

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler.cc (right):

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:12: // Observes tasks
both from a MessageLoop and a RendererScheduler which is
On 2015/02/19 11:47:49, rmcilroy wrote:
> nit - newline above comment

Done.

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.cc:69: if
(!inside_task_) {
On 2015/02/19 11:47:49, rmcilroy wrote:
> How does this work with nested message loops?  It looks like we won't get task
> observer notifications for any tasks in a nested message loop - is this the
case
> with the current message_loop implementation?

Good point. Actually with this code we'd get notifications for all but the first
task in the nested message loop because DidProcessTask clears the inside_task_
flag. I'm not convinced this will make a big difference either way, but I think
I can add code to make sure we send all notifications inside a nested run loop.

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler.h (right):

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler.h:27: virtual void
removeTaskObserver(TaskObserver* observer);
On 2015/02/19 11:47:49, rmcilroy wrote:
> Should these be marked override instead (they are overriding WebThreadBase,
not
> implementing blink::WebThread)?

True, I guess I was being overly paranoid. Fixed.

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc
(right):

https://codereview.chromium.org/922733002/diff/200001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc:14: const
int kWorkBatchSize = 2;
On 2015/02/19 11:47:49, rmcilroy wrote:
> nit - newline above

Done.

[rmcilroy, 2015-02-20 23:03:07]

https://codereview.chromium.org/922733002/diff/240001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc
(right):

https://codereview.chromium.org/922733002/diff/240001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc:181: //
loop.
I worry that we might see test fail due to the double pair of calls for nested
message loops.

Rethinking this again, wouldn't it be simpler just to always skip the first
willProcessTask and the last didProcessTask in TaskQueueManager::DoWork given
that we know that the message_loop will have already sent these callbacks for
the DoWork task itself?  That way we could accurately only send one observation
per task, and wouldn't have to worry about nesting, etc.

[Sami, 2015-02-23 15:11:02]

Thanks Ross!

https://codereview.chromium.org/922733002/diff/240001/content/renderer/schedu...
File content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc
(right):

https://codereview.chromium.org/922733002/diff/240001/content/renderer/schedu...
content/renderer/scheduler/webthread_impl_for_scheduler_unittest.cc:181: //
loop.
On 2015/02/20 23:03:06, rmcilroy wrote:
> I worry that we might see test fail due to the double pair of calls for nested
> message loops.

FWIW, a form of the double calls already happens because the the nested
MessageLoop is started from within an already executing task.
 
> Rethinking this again, wouldn't it be simpler just to always skip the first
> willProcessTask and the last didProcessTask in TaskQueueManager::DoWork given
> that we know that the message_loop will have already sent these callbacks for
> the DoWork task itself?  That way we could accurately only send one
observation
> per task, and wouldn't have to worry about nesting, etc.

Thanks, that's a great idea to make this simpler. It's not quite as
straightforward as I'd like because we need to keep the previously executed task
around a little longer, but I think I prefer it to this post-processing
spaghetti. PTAL!

[rmcilroy, 2015-02-23 22:41:44]

Looks much simpler without the NestedObserver, thanks!

One last comment, otherwise looks good.

https://codereview.chromium.org/922733002/diff/260001/content/renderer/schedu...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/260001/content/renderer/schedu...
content/renderer/scheduler/task_queue_manager_unittest.cc:610: // notifications
are filtered out.
These test results now look a bit strange - looking at them I think it might
confuse people that to get all task observer messages they need to register a
task observer on both the TQM and the underlying message loop.  Could we have
the TQM register the task observer on the underlying message loop as well, thus
allowing observation of all tasks just by registering on the TQM?

[Sami, 2015-02-24 14:39:20]

https://codereview.chromium.org/922733002/diff/260001/content/renderer/schedu...
File content/renderer/scheduler/task_queue_manager_unittest.cc (right):

https://codereview.chromium.org/922733002/diff/260001/content/renderer/schedu...
content/renderer/scheduler/task_queue_manager_unittest.cc:610: // notifications
are filtered out.
On 2015/02/23 22:41:43, rmcilroy wrote:
> These test results now look a bit strange - looking at them I think it might
> confuse people that to get all task observer messages they need to register a
> task observer on both the TQM and the underlying message loop.  Could we have
> the TQM register the task observer on the underlying message loop as well,
thus
> allowing observation of all tasks just by registering on the TQM?

Yeah, I suppose that would be clearer because generally you want both types of
notifications. Done.

[rmcilroy, 2015-02-24 15:32:00]

One last optional comment but otherwise lgtm, thanks!

https://codereview.chromium.org/922733002/diff/300001/content/renderer/schedu...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/922733002/diff/300001/content/renderer/schedu...
content/renderer/scheduler/task_queue_manager.cc:393:
previous_task_.task.Reset();
One last nit (feel free to ignore) - could you just make this a scoped_ptr local
variable, and pass it as an argument to ProcessTaskFromWorkQueue - that way as
soon as it goes out of scope at the end of this function it is automatically
reset (and also doesn't add another field to TQM).

[Sami, 2015-02-24 17:04:44]

https://codereview.chromium.org/922733002/diff/300001/content/renderer/schedu...
File content/renderer/scheduler/task_queue_manager.cc (right):

https://codereview.chromium.org/922733002/diff/300001/content/renderer/schedu...
content/renderer/scheduler/task_queue_manager.cc:393:
previous_task_.task.Reset();
On 2015/02/24 15:32:00, rmcilroy wrote:
> One last nit (feel free to ignore) - could you just make this a scoped_ptr
local
> variable, and pass it as an argument to ProcessTaskFromWorkQueue - that way as
> soon as it goes out of scope at the end of this function it is automatically
> reset (and also doesn't add another field to TQM).

I wanted to avoid heap allocations and made it a member for that reason. I've
now made it a stack variable instead which achieves the same goal. As a small
optimization I'm also resetting the closure right after running it so we don't
make a useless copy of it (the task observer shouldn't really do anything with
the closure).

[rmcilroy, 2015-02-24 17:35:36]

On 2015/02/24 17:04:44, Sami wrote:
>
https://codereview.chromium.org/922733002/diff/300001/content/renderer/schedu...
> File content/renderer/scheduler/task_queue_manager.cc (right):
> 
>
https://codereview.chromium.org/922733002/diff/300001/content/renderer/schedu...
> content/renderer/scheduler/task_queue_manager.cc:393:
> previous_task_.task.Reset();
> On 2015/02/24 15:32:00, rmcilroy wrote:
> > One last nit (feel free to ignore) - could you just make this a scoped_ptr
> local
> > variable, and pass it as an argument to ProcessTaskFromWorkQueue - that way
as
> > soon as it goes out of scope at the end of this function it is automatically
> > reset (and also doesn't add another field to TQM).
> 
> I wanted to avoid heap allocations and made it a member for that reason. I've
> now made it a stack variable instead which achieves the same goal. As a small
> optimization I'm also resetting the closure right after running it so we don't
> make a useless copy of it (the task observer shouldn't really do anything with
> the closure).

Great, thanks! LGTM

[Sami, 2015-02-24 17:36:23]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-02-24 17:36:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/922733002/#ps340001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-02-24 17:37:33]

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

[commit-bot: I haz the power, 2015-02-24 18:33:15]

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

[commit-bot: I haz the power, 2015-02-24 18:33:16]

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

[Sami, 2015-02-24 18:35:33]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-02-24 18:35:33]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
rmcilroy@chromium.org, alexclarke@chromium.org
Link to the patchset: https://codereview.chromium.org/922733002/#ps360001
(title: "Build fix.")

[commit-bot: I haz the power, 2015-02-24 18:36:11]

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

[commit-bot: I haz the power, 2015-02-24 19:36:15]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-02-24 19:37:00]

Patchset 19 (id:??) landed as
https://crrev.com/0da8dc886fad453b0cf4f1c718da8d44e132b618
Cr-Commit-Position: refs/heads/master@{#317852}