cc: Remove BeginFrameSourcesConstructor.

cc: Remove BeginFrameSourcesConstructor.

Customizing begin frame sources is easier using a static Create method
instead of using a special class for that. Also get rid of
VSyncParameterObserver as a result.

R=brianderson,mithro
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[sunnyps, 2015-06-20 02:13:55]

PTAL

This CL gets rid of BeginFrameSourcesConstructor which is an inelegant hack when
a much better solution exists - static Create method. The Scheduler now
interacts with the SyntheticBeginFrameSource directly so there's no need for the
VSyncParameterObserver interface.

Aside from this, the external begin frame source is now held as an unowned
pointer and so are the task runners. I can split this part out if necessary.

Thanks!

[brianderson, 2015-06-21 02:29:28]

lgtm. Although please associate with a bug first and mention other cleanups in
the subject.

We should also consider removing the BeginFrameSourceMultiplexer. It was
previously used for background ticking, which Tim removed. We were keeping it
around for dynamic vsync enable/disable, but Brandon doesn't have any plans to
work on that again any time soon.

[mithro-old, 2015-06-22 11:49:39]

Generally LGTM, some minor comments. 

I believe there is already a bug open about removing this (after background
ticking was removed, the hack wasn't needed anymore).

I think we should remove the multiplexer in a separate patch incase we want to
resurrect that bit.

Tim 'mithro' Ansell

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:28: if (!settings.use_external_begin_frame_source) {
Can you please keep some type of tracing which signifies which BeginFrameSources
are being created.

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:36: synthetic_frame_source.Pass(),
unthrottled_frame_source.Pass()));
It's kind of weird that you can end up "passing a null pointer" here but I guess
it's better then previously.

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.h#ne...
cc/scheduler/scheduler.h:225: if (settings_.use_external_begin_frame_source)
Should you DCHECK external_frame_source_ here?

https://codereview.chromium.org/1201573002/diff/1/cc/surfaces/display_schedul...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/1201573002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler_unittest.cc:42: base::NullTaskRunner* task_runner,
Why is this changing to a NullTaskRunner verses a SingleThreadTaskRunner?

[sunnyps, 2015-06-22 20:07:36]

On 2015/06/21 02:29:28, brianderson wrote:
> lgtm. Although please associate with a bug first and mention other cleanups in
> the subject.

I tried looking for an existing bug for this but couldn't find any. I don't
think creating a new (and short lived) bug is necessary because there's really
no other context to this fix other than the CL description here.

> 
> We should also consider removing the BeginFrameSourceMultiplexer. It was
> previously used for background ticking, which Tim removed. We were keeping it
> around for dynamic vsync enable/disable, but Brandon doesn't have any plans to
> work on that again any time soon.

I'll file a bug for removing BFSMultiplexer.

[sunnyps, 2015-06-22 20:32:41]

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:28: if (!settings.use_external_begin_frame_source) {
On 2015/06/22 11:49:39, mithro wrote:
> Can you please keep some type of tracing which signifies which
BeginFrameSources
> are being created.

I'm enabling the scheduler constructor trace which also logs the settings.
Looking at settings tells us if the scheduler created a synthetic frame source.

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:36: synthetic_frame_source.Pass(),
unthrottled_frame_source.Pass()));
On 2015/06/22 11:49:39, mithro wrote:
> It's kind of weird that you can end up "passing a null pointer" here but I
guess
> it's better then previously.

We're passing in a scoped_ptr that wraps a nullptr. You have to call Pass on the
scoped_ptr even if it contains a nullptr because it's copy constructor is
private.

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1201573002/diff/1/cc/scheduler/scheduler.h#ne...
cc/scheduler/scheduler.h:225: if (settings_.use_external_begin_frame_source)
On 2015/06/22 11:49:39, mithro wrote:
> Should you DCHECK external_frame_source_ here?

Done.

https://codereview.chromium.org/1201573002/diff/1/cc/surfaces/display_schedul...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/1201573002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler_unittest.cc:42: base::NullTaskRunner* task_runner,
On 2015/06/22 11:49:39, mithro wrote:
> Why is this changing to a NullTaskRunner verses a SingleThreadTaskRunner?

No particular reason but I like keeping tests specific as far as possible.

[sunnyps, 2015-06-22 20:32:47]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2015-06-22 20:32:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org, mithro@mithis.com
Link to the patchset: https://codereview.chromium.org/1201573002/#ps40001
(title: "mithro's review")

[commit-bot: I haz the power, 2015-06-22 20:34:33]

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

[commit-bot: I haz the power, 2015-06-22 20:55:13]

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

[commit-bot: I haz the power, 2015-06-22 20:55:14]

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

[sunnyps, 2015-06-22 22:27:44]

I reverted the refptr changes from this CL. PTAL

[mithro-old, 2015-06-22 23:17:08]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-06-22 23:17:09]

lgtm

[mithro-old, 2015-06-22 23:17:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/1201573002/#ps60001
(title: "Remove refptr changes")

[commit-bot: I haz the power, 2015-06-22 23:17:41]

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

[commit-bot: I haz the power, 2015-06-23 00:09:51]

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

[commit-bot: I haz the power, 2015-06-23 00:09:52]

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

[mithro-old, 2015-06-23 00:11:00]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-06-23 00:11:01]

lgtm

The failure seems unrelated to this change. Sending to the commit queue again.

[commit-bot: I haz the power, 2015-06-23 00:11:48]

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

[commit-bot: I haz the power, 2015-06-23 00:44:33]

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

[commit-bot: I haz the power, 2015-06-23 00:44:33]

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

[sunnyps, 2015-06-23 01:19:18]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2015-06-23 01:20:10]

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

[commit-bot: I haz the power, 2015-06-23 01:47:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-23 01:47:56]

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