cc::SurfaceDependencyTracker should not crash when a Display goes away

cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2880023002
Cr-Original-Commit-Position: refs/heads/master@{#474539}
Committed: https://chromium.googlesource.com/chromium/src/+/b520968f4ac40fb89ce25329134b388ca42cca12
Review-Url: https://codereview.chromium.org/2880023002
Cr-Commit-Position: refs/heads/master@{#474783}
Committed: https://chromium.googlesource.com/chromium/src/+/d71466ad118aca05aa347c9766f80a5f77528588

[Fady Samuel, 2017-05-12 21:18:33]

Description was changed from

==========
cc::SurfaceDependencyTracker should not be a BeginFrameObserver

Previously, SurfaceDependencyTracker was a BeginFrameObserver that observed
BeginFrames from the first display. This is a problem if the first display
goes away. This CL solves this problem by no longer making
SurfaceDependencyTracker a BeginFrameObserver, and instead have it get its
BeginFrames from MojoFrameSinkManager.

MojoFrameSinkManager tracks the "primary root" which is the display whose
BeginFrames are forwarded to SurfaceDependencyTracker. If the primary root
goes away, then we pick the next root as the primary root, if one is available.
If there are no roots available, then SurfaceDependencyTracker no longer
receives BeginFrames.

BUG=672962
==========

to

==========
cc::SurfaceDependencyTracker should not be a BeginFrameObserver

Previously, SurfaceDependencyTracker was a BeginFrameObserver that observed
BeginFrames from the first display. This is a problem if the first display
goes away. This CL solves this problem by no longer making
SurfaceDependencyTracker a BeginFrameObserver, and instead have it get its
BeginFrames from MojoFrameSinkManager.

MojoFrameSinkManager tracks the "primary root" which is the display whose
BeginFrames are forwarded to SurfaceDependencyTracker. If the primary root
goes away, then we pick the next root as the primary root, if one is available.
If there are no roots available, then SurfaceDependencyTracker no longer
receives BeginFrames.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-12 21:19:51]

fsamuel@chromium.org changed reviewers:
+ jbauman@chromium.org

[Fady Samuel, 2017-05-12 21:19:51]

+jbauman@ for review. PTAL

[Fady Samuel, 2017-05-12 21:20:32]

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

[commit-bot: I haz the power, 2017-05-12 21:21:42]

Dry run: 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-05-12 22:31:14]

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

[commit-bot: I haz the power, 2017-05-12 22:31:15]

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

[Fady Samuel, 2017-05-15 18:45:59]

Description was changed from

==========
cc::SurfaceDependencyTracker should not be a BeginFrameObserver

Previously, SurfaceDependencyTracker was a BeginFrameObserver that observed
BeginFrames from the first display. This is a problem if the first display
goes away. This CL solves this problem by no longer making
SurfaceDependencyTracker a BeginFrameObserver, and instead have it get its
BeginFrames from MojoFrameSinkManager.

MojoFrameSinkManager tracks the "primary root" which is the display whose
BeginFrames are forwarded to SurfaceDependencyTracker. If the primary root
goes away, then we pick the next root as the primary root, if one is available.
If there are no roots available, then SurfaceDependencyTracker no longer
receives BeginFrames.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
ExternalBeginFrameSource instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and is an ExternalBeginFrameSourceClient. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
MojoFrameSinkManager which then adds itself as a BeginFrameObserver to
the primary root's BeginFrameSource.

When the primary root goes away, MojoFrameSinkManager removes itself
as an observer and, if necessary, adds itself as an observer 
arbitrarily to another display. Future BeginFrames get forwarded from
MojoFrameSinkManager to SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-15 18:45:59]

fsamuel@chromium.org changed reviewers:
+ vmpstr@chromium.org
- jbauman@chromium.org

[Fady Samuel, 2017-05-15 18:53:57]

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

[Fady Samuel, 2017-05-15 18:54:02]

PTAL Vlad!

[commit-bot: I haz the power, 2017-05-15 18:54:46]

Dry run: 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-05-15 19:24:22]

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

[commit-bot: I haz the power, 2017-05-15 19:24:23]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Fady Samuel, 2017-05-15 21:52:51]

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

[commit-bot: I haz the power, 2017-05-15 21:54:08]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-05-15 22:12:25]

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

[Fady Samuel, 2017-05-15 22:13:00]

fsamuel@chromium.org changed reviewers:
+ enne@chromium.org

[Fady Samuel, 2017-05-15 22:13:00]

+enne@ for viz changes.

[commit-bot: I haz the power, 2017-05-15 22:13:03]

Dry run: CQ is trying da patch.

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

[enne (OOO), 2017-05-15 22:23:41]

Is there a test that covers this behavior of removing primary root frame sink
ids and also whether all the plumbing and needs_begin_frame stuff is all hooked
up correctly?

[commit-bot: I haz the power, 2017-05-15 22:54:31]

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

[commit-bot: I haz the power, 2017-05-15 22:54:32]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Fady Samuel, 2017-05-23 23:37:39]

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

[Fady Samuel, 2017-05-23 23:39:09]

Description was changed from

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
ExternalBeginFrameSource instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and is an ExternalBeginFrameSourceClient. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
MojoFrameSinkManager which then adds itself as a BeginFrameObserver to
the primary root's BeginFrameSource.

When the primary root goes away, MojoFrameSinkManager removes itself
as an observer and, if necessary, adds itself as an observer 
arbitrarily to another display. Future BeginFrames get forwarded from
MojoFrameSinkManager to SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver to
the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource removes
itself
as an observer and, if necessary, adds itself as an observer 
arbitrarily to another BeginFrameSource. Future BeginFrames get forwarded from
the PrimaryBeginFrameSource to SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-23 23:39:44]

Description was changed from

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver to
the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource removes
itself
as an observer and, if necessary, adds itself as an observer 
arbitrarily to another BeginFrameSource. Future BeginFrames get forwarded from
the PrimaryBeginFrameSource to SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Fady Samuel, 2017-05-23 23:40:28]

PTAL Enne! I've moved the functionality from MojoFrameSinkManager to a
"PrimaryBeginFrameSource" that is easily unit testable. I've added a unit test.
Thanks!

[Fady Samuel, 2017-05-23 23:40:29]

PTAL Enne! I've moved the functionality from MojoFrameSinkManager to a
"PrimaryBeginFrameSource" that is easily unit testable. I've added a unit test.
Thanks!

[commit-bot: I haz the power, 2017-05-23 23:41:55]

Dry run: 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-05-24 00:10:26]

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

[commit-bot: I haz the power, 2017-05-24 00:10:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Fady Samuel, 2017-05-24 01:31:02]

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

[commit-bot: I haz the power, 2017-05-24 01:33:33]

Dry run: 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-05-24 02:42:29]

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

[commit-bot: I haz the power, 2017-05-24 02:42:30]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[Fady Samuel, 2017-05-24 02:56:13]

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

[commit-bot: I haz the power, 2017-05-24 02:57:06]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-05-24 03:19:31]

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

[commit-bot: I haz the power, 2017-05-24 03:20:25]

Dry run: CQ is trying da patch.

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

[Fady Samuel, 2017-05-24 03:21:19]

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

[commit-bot: I haz the power, 2017-05-24 03:22:05]

Dry run: 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-05-24 04:22:30]

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

[commit-bot: I haz the power, 2017-05-24 04:22:31]

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

[Fady Samuel, 2017-05-24 04:38:03]

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

[commit-bot: I haz the power, 2017-05-24 04:38:59]

Dry run: 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-05-24 06:26:55]

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

[commit-bot: I haz the power, 2017-05-24 06:26:56]

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

[Fady Samuel, 2017-05-24 14:50:46]

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

[commit-bot: I haz the power, 2017-05-24 14:51:13]

Dry run: 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-05-24 15:30:03]

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

[commit-bot: I haz the power, 2017-05-24 15:30:04]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-05-24 17:55:17]

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
File cc/surfaces/primary_begin_frame_source.h (right):

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
cc/surfaces/primary_begin_frame_source.h:44: void
OnBeginFrameSourceAdded(BeginFrameSource* begin_frame_source) override;
This is a tiny design nit in this class, but I feel like there's some redundancy
here with a frame_sink_manager_ pointer as well as
OnBeginFrameSourceAdded/Removed functions.

Do you expect that there will be more FrameSinkObservers in the future?

If you think there's only one, here are some suggestions to clean up
interdependencies here:
* FrameSinkManager owns the PrimaryBeginFrameSource (also doesn't need to be
heap allocated)
* on begin frame source added in FSM, call these PBFS OnBFSAdded functions
directly (no need for observer)
* PBFS doesn't need to know about FrameSinkManager, it's just used by it
* PBFS can store all of the added BFS in some flat set (and can use this to get
the next one when the current one is removed)

This makes PBFS just something that provides a stable BFS interface to a
potentially changing set of BFS rather that's tied directly into
FrameSinkManager.

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
cc/surfaces/primary_begin_frame_source.h:49:
std::unique_ptr<ExternalBeginFrameSource> begin_frame_source_;
Maybe just make this a member instead of a heap allocation?

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
cc/surfaces/primary_begin_frame_source.h:50: BeginFrameSource*
primary_begin_frame_source_ = nullptr;
There's a lot of "primary" verbage everywhere.  Maybe s/primary/current/ here
for clarity?

[Fady Samuel, 2017-05-24 18:36:52]

PTAL Enne! Thanks!

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
File cc/surfaces/primary_begin_frame_source.h (right):

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
cc/surfaces/primary_begin_frame_source.h:44: void
OnBeginFrameSourceAdded(BeginFrameSource* begin_frame_source) override;
On 2017/05/24 17:55:17, enne wrote:
> This is a tiny design nit in this class, but I feel like there's some
redundancy
> here with a frame_sink_manager_ pointer as well as
> OnBeginFrameSourceAdded/Removed functions.
> 
> Do you expect that there will be more FrameSinkObservers in the future?
> 
> If you think there's only one, here are some suggestions to clean up
> interdependencies here:
> * FrameSinkManager owns the PrimaryBeginFrameSource (also doesn't need to be
> heap allocated)
> * on begin frame source added in FSM, call these PBFS OnBFSAdded functions
> directly (no need for observer)
> * PBFS doesn't need to know about FrameSinkManager, it's just used by it
> * PBFS can store all of the added BFS in some flat set (and can use this to
get
> the next one when the current one is removed)
> 
> This makes PBFS just something that provides a stable BFS interface to a
> potentially changing set of BFS rather that's tied directly into
> FrameSinkManager.

Done.

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
cc/surfaces/primary_begin_frame_source.h:49:
std::unique_ptr<ExternalBeginFrameSource> begin_frame_source_;
On 2017/05/24 17:55:17, enne wrote:
> Maybe just make this a member instead of a heap allocation?

Done.

https://codereview.chromium.org/2880023002/diff/280001/cc/surfaces/primary_be...
cc/surfaces/primary_begin_frame_source.h:50: BeginFrameSource*
primary_begin_frame_source_ = nullptr;
On 2017/05/24 17:55:17, enne wrote:
> There's a lot of "primary" verbage everywhere.  Maybe s/primary/current/ here
> for clarity?

Done.

[Fady Samuel, 2017-05-24 18:36:57]

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

[commit-bot: I haz the power, 2017-05-24 18:37:46]

Dry run: CQ is trying da patch.

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

[enne (OOO), 2017-05-24 18:46:08]

Thanks, that's a lot simpler to my eyes.  :)

lgtm

[commit-bot: I haz the power, 2017-05-24 23:05:40]

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

[commit-bot: I haz the power, 2017-05-24 23:05:41]

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

[Fady Samuel, 2017-05-24 23:07:44]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2017-05-24 23:08:40]

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-05-25 03:10:50]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495667264628500,
"parent_rev": "d69b01d30756ec0e44d820dadf6685ba67328e64", "commit_rev":
"b520968f4ac40fb89ce25329134b388ca42cca12"}

[commit-bot: I haz the power, 2017-05-25 03:11:07]

Description was changed from

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[commit-bot: I haz the power, 2017-05-25 03:11:08]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/b520968f4ac40fb89ce25329134b...

[Ken Rockot(use gerrit already), 2017-05-25 15:01:32]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2908473002/ by rockot@chromium.org.

The reason for reverting is: I think this is probably responsible for new flake
like this:

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_....

[Fady Samuel, 2017-05-25 15:31:14]

Description was changed from

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

to

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

[Fady Samuel, 2017-05-25 15:32:27]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-05-25 15:32:28]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2880023002/#ps320001
(title: "Fixed flake")

[commit-bot: I haz the power, 2017-05-25 15:33:04]

CQ is trying da patch.

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

[Fady Samuel, 2017-05-25 15:41:59]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-05-25 15:41:59]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2880023002/#ps340001
(title: "Fixed typo")

[commit-bot: I haz the power, 2017-05-25 15:42:19]

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-05-25 16:25:37]

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

[commit-bot: I haz the power, 2017-05-25 16:25:38]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Fady Samuel, 2017-05-25 18:53:26]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2017-05-25 18:53:27]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2880023002/#ps360001
(title: "Fix LayerTreeHostImpl unit tests")

[commit-bot: I haz the power, 2017-05-25 18:54:07]

CQ is trying da patch.

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

[Fady Samuel, 2017-05-25 18:55:26]

On 2017/05/25 16:25:38, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

I've had to make a change to how we populate activation_dependencies because
there were cases where we were incorrectly placing surface IDs in
activation_dependencies causing surface synchronization to kick in. That has
been fixed in SurfaceLayerImpl and I've updated unit tests.

I've also added a check to surface.cc to avoid crashing (following a null
pointer) in the case a client gives it an invalid set of
activation_dependencies.

[Fady Samuel, 2017-05-25 18:55:27]

On 2017/05/25 16:25:38, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

I've had to make a change to how we populate activation_dependencies because
there were cases where we were incorrectly placing surface IDs in
activation_dependencies causing surface synchronization to kick in. That has
been fixed in SurfaceLayerImpl and I've updated unit tests.

I've also added a check to surface.cc to avoid crashing (following a null
pointer) in the case a client gives it an invalid set of
activation_dependencies.

[Fady Samuel, 2017-05-25 18:55:28]

On 2017/05/25 16:25:38, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

I've had to make a change to how we populate activation_dependencies because
there were cases where we were incorrectly placing surface IDs in
activation_dependencies causing surface synchronization to kick in. That has
been fixed in SurfaceLayerImpl and I've updated unit tests.

I've also added a check to surface.cc to avoid crashing (following a null
pointer) in the case a client gives it an invalid set of
activation_dependencies.

[commit-bot: I haz the power, 2017-05-25 20:35:26]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1495738406720430,
"parent_rev": "2144d0f2e6603b21c50df07a318fc37cf6b6d513", "commit_rev":
"d71466ad118aca05aa347c9766f80a5f77528588"}

[commit-bot: I haz the power, 2017-05-25 20:35:57]

Description was changed from

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

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

to

==========
cc::SurfaceDependencyTracker should not crash when a Display goes away

Previously, SurfaceDependencyTracker was a BeginFrameObserver that 
directly observed BeginFrames from the first display. This is a problem 
if the first display goes away. This CL solves this problem by making
SurfaceDependencyTracker observe an independent 
"PrimaryBeginFrameSource" instead. MojoFrameSinkManager owns 
SurfaceDependencyTracker and the PrimaryBeginFrameSource. When
SurfaceDependencyTracker needs BeginFrames, that request goes to
PrimaryBeginFrameSource which then adds itself as a BeginFrameObserver 
to the primary BeginFrameSource.

When the primary BeginFrameSource goes away, PrimaryBeginFrameSource 
removes itself as an observer and, if necessary, adds itself as an 
observer arbitrarily to another BeginFrameSource. Future BeginFrames 
get forwarded from the PrimaryBeginFrameSource to 
SurfaceDependencyTracker.

BUG=672962
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2880023002
Cr-Original-Commit-Position: refs/heads/master@{#474539}
Committed:
https://chromium.googlesource.com/chromium/src/+/b520968f4ac40fb89ce25329134b...
Review-Url: https://codereview.chromium.org/2880023002
Cr-Commit-Position: refs/heads/master@{#474783}
Committed:
https://chromium.googlesource.com/chromium/src/+/d71466ad118aca05aa347c9766f8...
==========

[commit-bot: I haz the power, 2017-05-25 20:36:01]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/d71466ad118aca05aa347c9766f8...