Hook up BeginFrameSource to SurfaceFactoryClient via SurfaceManager

cc/surfaces/surface_manager.cc

Index: cc/surfaces/surface_manager.cc
diff --git a/cc/surfaces/surface_manager.cc b/cc/surfaces/surface_manager.cc
index 03951e41145fc7468f72abfbd8e9c78ae9ce0889..7769e21c32daf89be6ddb1897111d2bd558e2763 100644
--- a/cc/surfaces/surface_manager.cc
+++ b/cc/surfaces/surface_manager.cc
@@ -9,10 +9,19 @@
 
 #include "base/logging.h"
 #include "cc/surfaces/surface.h"
+#include "cc/surfaces/surface_factory_client.h"
 #include "cc/surfaces/surface_id_allocator.h"
 
 namespace cc {
 
+SurfaceManager::ClientSourceMapping::ClientSourceMapping()
+    : client(nullptr), source(nullptr) {}
+
+SurfaceManager::ClientSourceMapping::~ClientSourceMapping() {
+  DCHECK(is_empty()) << "client: " << client
+                     << ", children: " << children.size();
+}
+
 SurfaceManager::SurfaceManager() {
   thread_checker_.DetachFromThread();
 }
@@ -32,6 +41,7 @@ void SurfaceManager::RegisterSurface(Surface* surface) {
   DCHECK(surface);
   DCHECK(!surface_map_.count(surface->surface_id()));
   surface_map_[surface->surface_id()] = surface;
+  // TODO(enne): verify surface namespace is registered
 }
 
 void SurfaceManager::DeregisterSurface(SurfaceId surface_id) {
@@ -39,6 +49,7 @@ void SurfaceManager::DeregisterSurface(SurfaceId surface_id) {
   SurfaceMap::iterator it = surface_map_.find(surface_id);
   DCHECK(it != surface_map_.end());
   surface_map_.erase(it);
+  // TODO(enne): verify surface namespace is registered
 }
 
 void SurfaceManager::Destroy(scoped_ptr<Surface> surface) {
@@ -120,6 +131,137 @@ void SurfaceManager::GarbageCollectSurfaces() {
   }
 }
 
+void SurfaceManager::RegisterSurfaceFactoryClient(
+    uint32_t id_namespace,
+    SurfaceFactoryClient* client) {

[brianderson, 2016/02/18 23:01:07]

DCHECK namespace is valid?

+  if (client) {
+    DCHECK(!namespace_client_map_[id_namespace].client);
+    namespace_client_map_[id_namespace].client = client;
+    if (BeginFrameSource* source = namespace_client_map_[id_namespace].source) {

[brianderson, 2016/02/18 23:01:07]

Cache map lookup?

+      client->SetBeginFrameSource(source);
+    }
+  } else {
+    DCHECK_EQ(namespace_client_map_.count(id_namespace), 1u);
+    auto iter = namespace_client_map_.find(id_namespace);
+    if (iter->second.source)
+      iter->second.client->SetBeginFrameSource(nullptr);
+    iter->second.client = nullptr;
+
+    // The begin frame source should be removed prior to removing this
+    // client.
+    for (auto iter : registered_sources_)

[brianderson, 2016/02/18 23:01:07]

Maybe use a name other than "iter".

+      DCHECK_NE(iter.second, id_namespace);
+
+    if (iter->second.is_empty())
+      namespace_client_map_.erase(iter);

[brianderson, 2016/02/18 23:01:07]

Does it make a difference whether this is erased here vs. in
UnregisterSurfaceIdNamespace? If it were there, could you DCHECK is_empty()
instead?

+  }
+}
+
+void SurfaceManager::RegisterBeginFrameSource(BeginFrameSource* source,
+                                              uint32_t id_namespace) {
+  DCHECK(source);
+  DCHECK_EQ(namespace_client_map_.count(id_namespace), 1u);
+  DCHECK_EQ(registered_sources_.count(source), 0u);
+
+  registered_sources_[source] = id_namespace;
+  RecursivelyAttachBeginFrameSource(id_namespace, source);
+}
+
+void SurfaceManager::UnregisterBeginFrameSource(BeginFrameSource* source) {
+  DCHECK(source);
+  DCHECK_EQ(registered_sources_.count(source), 1u);
+
+  uint32_t id_namespace = registered_sources_[source];
+  DCHECK_EQ(namespace_client_map_.count(id_namespace), 1u);
+  registered_sources_.erase(source);
+
+  // TODO(enne): these walks could be done in one step.
+  // Remove this begin frame source from its subtree.
+  RecursivelyDetachBeginFrameSource(id_namespace, source);
+  // Then flush every remaining registered source to set anything null.
+  for (auto iter : registered_sources_)

[brianderson, 2016/02/18 23:01:07]

Nice.

+    RecursivelyAttachBeginFrameSource(iter.second, iter.first);
+}
+
+void SurfaceManager::RecursivelyAttachBeginFrameSource(
+    uint32_t id_namespace,
+    BeginFrameSource* source) {
+  auto& data = namespace_client_map_[id_namespace];

[jbauman, 2016/02/24 19:52:34]

One thing we'll have to be careful of is to make sure we don't have infinite
recursion here, either by keeping a set of namespaces that we've already looked
at in the current recursion, or by just being certain
RegisterSurfaceNamespaceHierarchy is only ever called by trusted code.

[enne (OOO), 2016/02/24 21:19:03]

Mmm, that's a really good point.  Rather than keeping a list, I *think* it would
be sufficient to just pass along the parent of the newly added hierarchy into
the recursion, and if that parent is not seen in the recursion, then no new
loops have been added.

+  if (!data.source) {

[brianderson, 2016/02/18 23:01:07]

Can you add a comment regarding the purpose of this if? It's not immediately
obvious without understanding how UnregisterBeginFrameSource works.

+    data.source = source;
+    if (data.client)
+      data.client->SetBeginFrameSource(source);
+  }
+  for (size_t i = 0; i < data.children.size(); ++i) {
+    RecursivelyAttachBeginFrameSource(data.children[i], source);
+  }
+}
+
+void SurfaceManager::RecursivelyDetachBeginFrameSource(
+    uint32_t id_namespace,
+    BeginFrameSource* source) {

[brianderson, 2016/02/18 23:01:07]

Maybe add some DCHECKS like you have in the other methods.

+  auto iter = namespace_client_map_.find(id_namespace);
+  if (iter->second.source == source) {
+    iter->second.source = nullptr;
+    if (iter->second.client)
+      iter->second.client->SetBeginFrameSource(nullptr);
+  }
+  std::vector<uint32_t>& children = iter->second.children;
+  for (size_t i = 0; i < children.size(); ++i) {
+    RecursivelyDetachBeginFrameSource(children[i], source);
+  }
+}
+
+void SurfaceManager::RegisterSurfaceNamespaceHierarchy(
+    uint32_t parent_namespace,
+    uint32_t child_namespace) {
+  std::vector<uint32_t>& children =
+      namespace_client_map_[parent_namespace].children;
+  for (size_t i = 0; i < children.size(); ++i)
+    DCHECK_NE(children[i], child_namespace);
+  children.push_back(child_namespace);
+
+  BeginFrameSource* parent_source =
+      namespace_client_map_[parent_namespace].source;
+  if (!parent_source)
+    return;
+
+  DCHECK_EQ(registered_sources_.count(parent_source), 1u);
+  RecursivelyAttachBeginFrameSource(child_namespace, parent_source);
+}
+
+void SurfaceManager::UnregisterSurfaceNamespaceHierarchy(
+    uint32_t parent_namespace,
+    uint32_t child_namespace) {
+  DCHECK_EQ(namespace_client_map_.count(parent_namespace), 1u);
+
+  auto iter = namespace_client_map_.find(parent_namespace);
+
+  std::vector<uint32_t>& children = iter->second.children;
+  bool found_child = false;
+  for (size_t i = 0; i < children.size(); ++i) {
+    if (children[i] == child_namespace) {
+      found_child = true;
+      children[i] = children[children.size() - 1];
+      children.resize(children.size() - 1);
+      break;
+    }
+  }
+  DCHECK(found_child);
+
+  BeginFrameSource* parent_source = iter->second.source;
+  if (!parent_source)
+    return;
+
+  if (iter->second.is_empty())
+    namespace_client_map_.erase(iter);
+
+  // TODO(enne): these walks could be done in one step.
+  RecursivelyDetachBeginFrameSource(child_namespace, parent_source);
+  for (auto iter : registered_sources_)
+    RecursivelyAttachBeginFrameSource(iter.second, iter.first);
+}
+
 Surface* SurfaceManager::GetSurfaceForId(SurfaceId surface_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
   SurfaceMap::iterator it = surface_map_.find(surface_id);
@@ -129,7 +271,7 @@ Surface* SurfaceManager::GetSurfaceForId(SurfaceId surface_id) {
 }
 
 bool SurfaceManager::SurfaceModified(SurfaceId surface_id) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  CHECK(thread_checker_.CalledOnValidThread());
   bool changed = false;
   FOR_EACH_OBSERVER(SurfaceDamageObserver, observer_list_,
                     OnSurfaceDamaged(surface_id, &changed));