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

cc/surfaces/primary_begin_frame_source.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CC_SURFACES_PRIMARY_BEGIN_FRAME_SOURCE_H_
+#define CC_SURFACES_PRIMARY_BEGIN_FRAME_SOURCE_H_
+
+#include "cc/scheduler/begin_frame_source.h"
+#include "cc/surfaces/frame_sink_observer.h"
+
+namespace cc {
+
+class FrameSinkManager;
+
+// PrimaryBeginFrameSource echos the first BeginFrameSource in the system.
+// If the first source goes away then it will echo the new first
+// BeginFrameSource.
+class PrimaryBeginFrameSource : public BeginFrameSource,
+                                public BeginFrameObserver,
+                                public ExternalBeginFrameSourceClient,
+                                public FrameSinkObserver {
+ public:
+  PrimaryBeginFrameSource();
+  ~PrimaryBeginFrameSource() override;
+
+  void Init(FrameSinkManager* frame_sink_manager);
+
+  // BeginFrameObserver implementation.
+  void OnBeginFrame(const BeginFrameArgs& args) override;
+  const BeginFrameArgs& LastUsedBeginFrameArgs() const override;
+  void OnBeginFrameSourcePausedChanged(bool paused) override;
+
+  // BeginFrameSource implementation.
+  void DidFinishFrame(BeginFrameObserver* obs,
+                      const BeginFrameAck& ack) override;
+  void AddObserver(BeginFrameObserver* obs) override;
+  void RemoveObserver(BeginFrameObserver* obs) override;
+  bool IsThrottled() const override;
+
+  // ExternalBeginFrameSourceClient implementation.
+  void OnNeedsBeginFrames(bool needs_begin_frames) override;
+
+  // FrameSinkObserver implementation.
+  void OnBeginFrameSourceAdded(BeginFrameSource* begin_frame_source) override;

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

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.

[Fady Samuel, 2017/05/24 18:36:51]

Done.

+  void OnBeginFrameSourceRemoved(BeginFrameSource* begin_frame_source) override;
+
+ private:
+  FrameSinkManager* frame_sink_manager_ = nullptr;
+  std::unique_ptr<ExternalBeginFrameSource> begin_frame_source_;

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

Maybe just make this a member instead of a heap allocation?

[Fady Samuel, 2017/05/24 18:36:51]

Done.

+  BeginFrameSource* primary_begin_frame_source_ = nullptr;

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

There's a lot of "primary" verbage everywhere.  Maybe s/primary/current/ here
for clarity?

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

Done.

+
+  // The last begin frame args generated by the begin frame source.
+  BeginFrameArgs last_begin_frame_args_;
+
+  bool needs_begin_frames_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(PrimaryBeginFrameSource);
+};
+
+}  // namespace cc
+
+#endif  // CC_SURFACES_PRIMARY_BEGIN_FRAME_SOURCE_H_