Refactor RendererSchedulerImpl to improve thread safety.

Refactor RendererSchedulerImpl to improve thread safety.

Puts most data members of RendererSchedulerImpl into one of three
structs that make sure they are accessed from the right thread and
where necessary within the appropriate mutex.

BUG=

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

[alex clarke (OOO till 29th), 2015-06-10 16:56:04]

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

[commit-bot: I haz the power, 2015-06-10 16:56:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177753003/1

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

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

[alex clarke (OOO till 29th), 2015-06-10 16:56:26]

Here's the refactor as requested.

[Sami, 2015-06-10 17:03:42]

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:199: // Don't access
current_policy_ directly, instead use SchedulerPolicy().
Can we remove like I mentioned in the other code review?

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:227: #if DCHECK_IS_ON()
Same question about the macros.

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:259:
CheckOnCompositorThread();
Since we don't actually know what the compositor thread is (it can be the main
thread in single threaded compositing), how about we remember the first thread
this is called on and check that we stay on it?

[alex clarke (OOO till 29th), 2015-06-10 17:21:15]

PTAL

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:199: // Don't access
current_policy_ directly, instead use SchedulerPolicy().
On 2015/06/10 17:03:42, Sami wrote:
> Can we remove like I mentioned in the other code review?

Done.

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:227: #if DCHECK_IS_ON()
On 2015/06/10 17:03:42, Sami wrote:
> Same question about the macros.

The assert's purpose is to double check things are wired up correctly.

I don't believe it's necessary do to that in release.

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:259:
CheckOnCompositorThread();
On 2015/06/10 17:03:42, Sami wrote:
> Since we don't actually know what the compositor thread is (it can be the main
> thread in single threaded compositing), how about we remember the first thread
> this is called on and check that we stay on it?

Done.

[alex clarke (OOO till 29th), 2015-06-10 17:25:42]

PTAL

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1177753003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.h:227: #if DCHECK_IS_ON()
On 2015/06/10 17:21:15, alexclarke1 wrote:
> On 2015/06/10 17:03:42, Sami wrote:
> > Same question about the macros.
> 
> The assert's purpose is to double check things are wired up correctly.
> 
> I don't believe it's necessary do to that in release.

Actually looks like most of them where not needed.

[Sami, 2015-06-10 18:16:13]

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

https://codereview.chromium.org/1177753003/diff/40001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:609:
MainThreadOnly().begin_main_frame_on_critical_path_);
Do any of these DCHECKs fire when you take a trace?

https://codereview.chromium.org/1177753003/diff/40001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1177753003/diff/40001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1669:
TEST_F(RendererSchedulerImplTest, ShutdownPreventsPostingOfNewTasks) {
Was this meant to be in this patch?

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

PTAL

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

https://codereview.chromium.org/1177753003/diff/40001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:609:
MainThreadOnly().begin_main_frame_on_critical_path_);
On 2015/06/10 18:16:13, Sami wrote:
> Do any of these DCHECKs fire when you take a trace?

They did not fire when I did a trace just now.

https://codereview.chromium.org/1177753003/diff/40001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1177753003/diff/40001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:1669:
TEST_F(RendererSchedulerImplTest, ShutdownPreventsPostingOfNewTasks) {
On 2015/06/10 18:16:13, Sami wrote:
> Was this meant to be in this patch?

I can keep it in the other patch if you'd prefer.

[Sami, 2015-06-11 13:23:36]

Thanks, lgtm with an optional suggestion.

https://codereview.chromium.org/1177753003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1177753003/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.h:256:
scoped_ptr<base::ThreadChecker> compositor_thread_checker_;
nit: Should we also move this to CompositorThreadOnly?

[alex clarke (OOO till 29th), 2015-06-11 13:31:23]

https://codereview.chromium.org/1177753003/diff/60001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1177753003/diff/60001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.h:256:
scoped_ptr<base::ThreadChecker> compositor_thread_checker_;
On 2015/06/11 13:23:36, Sami wrote:
> nit: Should we also move this to CompositorThreadOnly?

Done.

[alex clarke (OOO till 29th), 2015-06-11 13:31:28]

The CQ bit was checked by alexclarke@chromium.org

[alex clarke (OOO till 29th), 2015-06-11 13:31:28]

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/1177753003/#ps80001
(title: "Moved compositor_thread_checker_ into CompositorThreadOnly")

[commit-bot: I haz the power, 2015-06-11 13:31:48]

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

[commit-bot: I haz the power, 2015-06-11 14:28:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-11 14:29:07]

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