aura: Make aura responsible for FrameSinkId registration.

Consolidate the frame-sink registration in one place in aura (Window),
for aura. It used to be spread between aura (WindowPort*), and content
(DelegatedFrameHost), which made it tricky to follow. This then makes
it easier to move all FrameSinkId management inside the aura::Window
(from the various WindowPort implementations).

BUG=none

Change-Id: I37943eace983a31485dc6c0587d90619781b7021
Reviewed-on: https://chromium-review.googlesource.com/935490
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Sadrul Chowdhury <sadrul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539253}

content/browser/renderer_host/browser_compositor_view_mac.mm

@@ -188,7 +188,8 @@ void OnCompositingShuttingDown(ui::Compositor* compositor) override {}
   root_layer_.reset(new ui::Layer(ui::LAYER_SOLID_COLOR));
   delegated_frame_host_.reset(new DelegatedFrameHost(
       frame_sink_id, this, features::IsSurfaceSynchronizationEnabled(),
-      base::FeatureList::IsEnabled(features::kVizDisplayCompositor)));
+      base::FeatureList::IsEnabled(features::kVizDisplayCompositor),
+      true /* should_register_frame_sink_id */));
 
   dfh_display_ = display::Screen::GetScreen()->GetDisplayNearestView(nil);
 

content/browser/renderer_host/delegated_frame_host.cc

@@ -37,11 +37,13 @@ namespace content {
 DelegatedFrameHost::DelegatedFrameHost(const viz::FrameSinkId& frame_sink_id,
                                        DelegatedFrameHostClient* client,
                                        bool enable_surface_synchronization,
-                                       bool enable_viz)
+                                       bool enable_viz,
+                                       bool should_register_frame_sink_id)
     : frame_sink_id_(frame_sink_id),
       client_(client),
       enable_surface_synchronization_(enable_surface_synchronization),
       enable_viz_(enable_viz),
+      should_register_frame_sink_id_(should_register_frame_sink_id),
       tick_clock_(base::DefaultTickClock::GetInstance()),
       frame_evictor_(std::make_unique<viz::FrameEvictor>(this)) {
   ImageTransportFactory* factory = ImageTransportFactory::GetInstance();
@@ -639,7 +641,8 @@ void DelegatedFrameHost::SetCompositor(ui::Compositor* compositor) {
     return;
   compositor_ = compositor;
   compositor_->AddObserver(this);
-  compositor_->AddFrameSink(frame_sink_id_);
+  if (should_register_frame_sink_id_)
+    compositor_->AddFrameSink(frame_sink_id_);
 }
 
 void DelegatedFrameHost::ResetCompositor() {
@@ -648,7 +651,8 @@ void DelegatedFrameHost::ResetCompositor() {
   resize_lock_.reset();
   if (compositor_->HasObserver(this))
     compositor_->RemoveObserver(this);
-  compositor_->RemoveFrameSink(frame_sink_id_);
+  if (should_register_frame_sink_id_)
+    compositor_->RemoveFrameSink(frame_sink_id_);
   compositor_ = nullptr;
 }
 
@@ -674,7 +678,7 @@ void DelegatedFrameHost::CreateCompositorFrameSinkSupport() {
                  ->GetHostFrameSinkManager()
                  ->CreateCompositorFrameSinkSupport(this, frame_sink_id_,
                                                     is_root, needs_sync_points);
-  if (compositor_)
+  if (compositor_ && should_register_frame_sink_id_)
     compositor_->AddFrameSink(frame_sink_id_);
   if (needs_begin_frame_)
     support_->SetNeedsBeginFrame(true);
@@ -683,7 +687,7 @@ void DelegatedFrameHost::CreateCompositorFrameSinkSupport() {
 void DelegatedFrameHost::ResetCompositorFrameSinkSupport() {
   if (!support_)
     return;
-  if (compositor_)
+  if (compositor_ && should_register_frame_sink_id_)
     compositor_->RemoveFrameSink(frame_sink_id_);
   support_.reset();
 }

content/browser/renderer_host/delegated_frame_host.h

@@ -76,10 +76,15 @@ class CONTENT_EXPORT DelegatedFrameHost
       public viz::mojom::CompositorFrameSinkClient,
       public viz::HostFrameSinkClient {
  public:
+  // |should_register_frame_sink_id| flag indicates whether DelegatedFrameHost
+  // is responsible for registering the associated FrameSinkId with the
+  // compositor or not. This is set only on non-aura platforms, since aura is
+  // responsible for doing the appropriate [un]registration.
   DelegatedFrameHost(const viz::FrameSinkId& frame_sink_id,
                      DelegatedFrameHostClient* client,
                      bool enable_surface_synchronization,
-                     bool enable_viz);
+                     bool enable_viz,
+                     bool should_register_frame_sink_id);
   ~DelegatedFrameHost() override;
 
   // ui::CompositorObserver implementation.
@@ -226,6 +231,7 @@ class CONTENT_EXPORT DelegatedFrameHost
   DelegatedFrameHostClient* const client_;
   const bool enable_surface_synchronization_;
   const bool enable_viz_;
+  const bool should_register_frame_sink_id_;
   ui::Compositor* compositor_ = nullptr;
 
   // The surface id that was most recently activated by

content/browser/renderer_host/render_widget_host_view_aura.cc

@@ -1904,8 +1904,6 @@ void RenderWidgetHostViewAura::CreateAuraWindow(aura::client::WindowType type) {
   DCHECK(!window_);
   DCHECK(!is_mus_browser_plugin_guest_);
   window_ = new aura::Window(this);
-  if (frame_sink_id_.is_valid())
-    window_->set_embed_frame_sink_id(frame_sink_id_);
   window_->SetName("RenderWidgetHostViewAura");
   window_->SetProperty(aura::client::kEmbedType,
                        aura::client::WindowEmbedType::EMBED_IN_OWNER);
@@ -1920,6 +1918,10 @@ void RenderWidgetHostViewAura::CreateAuraWindow(aura::client::WindowType type) {
   window_->SetType(type);
   window_->Init(ui::LAYER_SOLID_COLOR);
   window_->layer()->SetColor(background_color_);
+  // This needs to happen only after |window_| has been initialized using
+  // Init(), because it needs to have the layer.
+  if (frame_sink_id_.is_valid())
+    window_->SetEmbedFrameSinkId(frame_sink_id_);
 
   if (!features::IsMusEnabled())
     return;
@@ -1948,7 +1950,8 @@ void RenderWidgetHostViewAura::CreateDelegatedFrameHostClient() {
       base::FeatureList::IsEnabled(features::kVizDisplayCompositor);
   delegated_frame_host_ = std::make_unique<DelegatedFrameHost>(
       frame_sink_id_, delegated_frame_host_client_.get(),
-      features::IsSurfaceSynchronizationEnabled(), enable_viz);
+      features::IsSurfaceSynchronizationEnabled(), enable_viz,
+      false /* should_register_frame_sink_id */);
   if (!create_frame_sink_callback_.is_null())
     std::move(create_frame_sink_callback_).Run(frame_sink_id_);
 

ui/aura/hit_test_data_provider_aura_unittest.cc

@@ -190,7 +190,7 @@ TEST_F(HitTestDataProviderAuraTest, Stacking) {
 // Tests that the hit-test regions get expanded with a custom event targeter.
 TEST_F(HitTestDataProviderAuraTest, CustomTargeter) {
   window3()->SetEventTargeter(std::make_unique<TestWindowTargeter>());
-  window2()->set_embed_frame_sink_id(viz::FrameSinkId(1, 2));
+  window2()->SetEmbedFrameSinkId(viz::FrameSinkId(1, 2));
   const auto hit_test_data =
       hit_test_data_provider()->GetHitTestData(compositor_frame_);
   ASSERT_TRUE(hit_test_data);
@@ -321,7 +321,7 @@ TEST_F(HitTestDataProviderAuraTest, DoNotSubmit) {
   ASSERT_TRUE(hit_test_data);
   EXPECT_EQ(hit_test_data->regions.size(), 2u);
 
-  window3()->set_embed_frame_sink_id(viz::FrameSinkId(1, 3));
+  window3()->SetEmbedFrameSinkId(viz::FrameSinkId(1, 3));
   hit_test_data = hit_test_data_provider()->GetHitTestData(compositor_frame_);
   ASSERT_TRUE(hit_test_data);
   EXPECT_EQ(hit_test_data->regions.size(), 1u);
@@ -334,7 +334,7 @@ TEST_F(HitTestDataProviderAuraTest, DoNotSubmit) {
   hit_test_data = hit_test_data_provider()->GetHitTestData(compositor_frame_);
   ASSERT_TRUE(hit_test_data);
   EXPECT_EQ(hit_test_data->regions.size(), 1u);
-  root()->set_embed_frame_sink_id(viz::FrameSinkId(1, 1));
+  root()->SetEmbedFrameSinkId(viz::FrameSinkId(1, 1));
   hit_test_data = hit_test_data_provider()->GetHitTestData(compositor_frame_);
   ASSERT_TRUE(hit_test_data);
   EXPECT_EQ(hit_test_data->regions.size(), 0u);

ui/aura/local/window_port_local.cc

@@ -125,12 +125,11 @@ WindowPortLocal::CreateLayerTreeFrameSink() {
   auto frame_sink = std::make_unique<LayerTreeFrameSinkLocal>(
       frame_sink_id_, context_factory_private->GetHostFrameSinkManager(),
       window_->GetName());
+  window_->SetEmbedFrameSinkId(frame_sink_id_);
   frame_sink->SetSurfaceChangedCallback(base::Bind(
       &WindowPortLocal::OnSurfaceChanged, weak_factory_.GetWeakPtr()));
   frame_sink_ = frame_sink->GetWeakPtr();
   AllocateLocalSurfaceId();
-  if (window_->GetRootWindow())
-    window_->layer()->GetCompositor()->AddFrameSink(frame_sink_id_);
   return std::move(frame_sink);
 }
 
@@ -152,16 +151,6 @@ viz::FrameSinkId WindowPortLocal::GetFrameSinkId() const {
   return frame_sink_id_;
 }
 
-void WindowPortLocal::OnWindowAddedToRootWindow() {
-  if (frame_sink_id_.is_valid())
-    window_->layer()->GetCompositor()->AddFrameSink(frame_sink_id_);
-}
-
-void WindowPortLocal::OnWillRemoveWindowFromRootWindow() {
-  if (frame_sink_id_.is_valid())
-    window_->layer()->GetCompositor()->RemoveFrameSink(frame_sink_id_);
-}
-
 void WindowPortLocal::OnEventTargetingPolicyChanged() {}
 
 void WindowPortLocal::OnSurfaceChanged(const viz::SurfaceInfo& surface_info) {

ui/aura/local/window_port_local.h

@@ -49,8 +49,6 @@ class AURA_EXPORT WindowPortLocal : public WindowPort {
   void AllocateLocalSurfaceId() override;
   const viz::LocalSurfaceId& GetLocalSurfaceId() override;
   viz::FrameSinkId GetFrameSinkId() const override;
-  void OnWindowAddedToRootWindow() override;
-  void OnWillRemoveWindowFromRootWindow() override;
   void OnEventTargetingPolicyChanged() override;
   bool ShouldRestackTransientChildren() override;
 

ui/aura/mus/window_port_mus.cc

@@ -303,7 +303,7 @@ void WindowPortMus::SetFrameSinkIdFromServer(
     const viz::FrameSinkId& frame_sink_id) {
   DCHECK(window_mus_type() == WindowMusType::TOP_LEVEL_IN_WM ||
          window_mus_type() == WindowMusType::EMBED_IN_OWNER);
-  window_->set_embed_frame_sink_id(frame_sink_id);
+  window_->SetEmbedFrameSinkId(frame_sink_id);
   UpdatePrimarySurfaceId();
 }
 
@@ -334,7 +334,7 @@ void WindowPortMus::SetFallbackSurfaceInfo(
     // |primary_surface_id_| shold not be valid, since we didn't know the
     // |window_->embed_frame_sink_id()|.
     DCHECK(!primary_surface_id_.is_valid());
-    window_->set_embed_frame_sink_id(surface_info.id().frame_sink_id());
+    window_->SetEmbedFrameSinkId(surface_info.id().frame_sink_id());
     UpdatePrimarySurfaceId();
   }
 
@@ -579,10 +579,7 @@ WindowPortMus::CreateLayerTreeFrameSink() {
             window_->GetName());
     layer_tree_frame_sink_local->SetSurfaceChangedCallback(base::BindRepeating(
         &WindowPortMus::OnSurfaceChanged, weak_ptr_factory_.GetWeakPtr()));
-    if (window_->layer()->GetCompositor()) {
-      window_->layer()->GetCompositor()->AddFrameSink(GetFrameSinkId());
-      is_frame_sink_id_added_to_compositor_ = true;
-    }
+    window_->SetEmbedFrameSinkId(frame_sink_id);
     local_layer_tree_frame_sink_ = layer_tree_frame_sink_local->GetWeakPtr();
     frame_sink = std::move(layer_tree_frame_sink_local);
   }
@@ -595,25 +592,6 @@ WindowPortMus::CreateLayerTreeFrameSink() {
   return frame_sink;
 }
 
-void WindowPortMus::OnWindowAddedToRootWindow() {
-  if (base::FeatureList::IsEnabled(features::kMash))
-    return;
-  if (local_layer_tree_frame_sink_) {
-    DCHECK(!is_frame_sink_id_added_to_compositor_);
-    window_->layer()->GetCompositor()->AddFrameSink(GetFrameSinkId());
-    is_frame_sink_id_added_to_compositor_ = true;
-  }
-}
-
-void WindowPortMus::OnWillRemoveWindowFromRootWindow() {
-  if (base::FeatureList::IsEnabled(features::kMash))
-    return;
-  if (is_frame_sink_id_added_to_compositor_) {
-    window_->layer()->GetCompositor()->RemoveFrameSink(GetFrameSinkId());
-    is_frame_sink_id_added_to_compositor_ = false;
-  }
-}
-
 void WindowPortMus::OnEventTargetingPolicyChanged() {
   SetEventTargetingPolicy(window_->event_targeting_policy());
 }

ui/aura/mus/window_port_mus.h

@@ -277,8 +277,6 @@ class AURA_EXPORT WindowPortMus : public WindowPort, public WindowMus {
   std::unique_ptr<cc::LayerTreeFrameSink> CreateLayerTreeFrameSink() override;
   void AllocateLocalSurfaceId() override;
   const viz::LocalSurfaceId& GetLocalSurfaceId() override;
-  void OnWindowAddedToRootWindow() override;
-  void OnWillRemoveWindowFromRootWindow() override;
   void OnEventTargetingPolicyChanged() override;
   bool ShouldRestackTransientChildren() override;
 

ui/aura/mus/window_port_mus_unittest.cc

@@ -35,7 +35,7 @@ TEST_F(WindowPortMusTest, LayerTreeFrameSinkGetsCorrectLocalSurfaceId) {
   window.SetBounds(gfx::Rect(300, 300));
   // Notify the window that it will embed an external client, so that it
   // correctly generates LocalSurfaceId.
-  window.set_embed_frame_sink_id(viz::FrameSinkId(0, 1));
+  window.SetEmbedFrameSinkId(viz::FrameSinkId(0, 1));
 
   viz::LocalSurfaceId local_surface_id = window.GetLocalSurfaceId();
   ASSERT_TRUE(local_surface_id.is_valid());
@@ -64,7 +64,7 @@ TEST_F(WindowPortMusTest, ClientSurfaceEmbedderUpdatesLayer) {
   Window window(nullptr);
   window.Init(ui::LAYER_NOT_DRAWN);
   window.SetBounds(gfx::Rect(300, 300));
-  window.set_embed_frame_sink_id(viz::FrameSinkId(0, 1));
+  window.SetEmbedFrameSinkId(viz::FrameSinkId(0, 1));
 
   // Allocate a new LocalSurfaceId. The ui::Layer should be updated.
   window.AllocateLocalSurfaceId();

ui/aura/mus/window_tree_client.cc

@@ -1203,6 +1203,8 @@ void WindowTreeClient::OnCaptureChanged(ui::Id new_capture_window_id,
 void WindowTreeClient::OnFrameSinkIdAllocated(
     ui::Id window_id,
     const viz::FrameSinkId& frame_sink_id) {
+  if (!base::FeatureList::IsEnabled(features::kMash))
+    return;
   WindowMus* window = GetWindowByServerId(window_id);
   if (!window)
     return;

ui/aura/test/window_test_api.cc

@@ -27,5 +27,9 @@ bool WindowTestApi::ContainsMouse() const {
              host->dispatcher()->GetLastMouseLocationInRoot());
 }
 
+void WindowTestApi::DisableFrameSinkRegistration() {
+  window_->disable_frame_sink_id_registration_ = true;
+}
+
 }  // namespace test
 }  // namespace aura

ui/aura/test/window_test_api.h

@@ -21,6 +21,8 @@ class WindowTestApi {
 
   bool ContainsMouse() const;
 
+  void DisableFrameSinkRegistration();
+
  private:
   Window* window_;
 

ui/aura/window.cc

@@ -957,7 +957,8 @@ void Window::OnStackingChanged() {
 }
 
 void Window::NotifyRemovingFromRootWindow(Window* new_root) {
-  port_->OnWillRemoveWindowFromRootWindow();
+  if (IsEmbeddingClient())
+    UnregisterFrameSinkId();
   for (WindowObserver& observer : observers_)
     observer.OnWindowRemovingFromRootWindow(this, new_root);
   for (Window::Windows::const_iterator it = children_.begin();
@@ -967,7 +968,8 @@ void Window::NotifyRemovingFromRootWindow(Window* new_root) {
 }
 
 void Window::NotifyAddedToRootWindow() {
-  port_->OnWindowAddedToRootWindow();
+  if (IsEmbeddingClient())
+    RegisterFrameSinkId();
   for (WindowObserver& observer : observers_)
     observer.OnWindowAddedToRootWindow(this);
   for (Window::Windows::const_iterator it = children_.begin();
@@ -1109,6 +1111,12 @@ viz::FrameSinkId Window::GetFrameSinkId() const {
   return port_->GetFrameSinkId();
 }
 
+void Window::SetEmbedFrameSinkId(const viz::FrameSinkId& frame_sink_id) {
+  DCHECK(frame_sink_id.is_valid());
+  embed_frame_sink_id_ = frame_sink_id;
+  RegisterFrameSinkId();
+}
+
 bool Window::IsEmbeddingClient() const {
   return embed_frame_sink_id_.is_valid();
 }
@@ -1250,4 +1258,23 @@ void Window::UpdateLayerName() {
 #endif
 }
 
+void Window::RegisterFrameSinkId() {
+  DCHECK(embed_frame_sink_id_.is_valid());
+  DCHECK(IsEmbeddingClient());
+  if (registered_frame_sink_id_ || disable_frame_sink_id_registration_)
+    return;
+  if (auto* compositor = layer()->GetCompositor()) {
+    compositor->AddFrameSink(embed_frame_sink_id_);
+    registered_frame_sink_id_ = true;
+  }
+}
+
+void Window::UnregisterFrameSinkId() {
+  if (!registered_frame_sink_id_)
+    return;
+  registered_frame_sink_id_ = false;
+  if (auto* compositor = layer()->GetCompositor())
+    compositor->RemoveFrameSink(embed_frame_sink_id_);
+}
+
 }  // namespace aura

ui/aura/window.h

@@ -397,9 +397,8 @@ class AURA_EXPORT Window : public ui::LayerDelegate,
   const viz::FrameSinkId& embed_frame_sink_id() const {
     return embed_frame_sink_id_;
   }
-  void set_embed_frame_sink_id(const viz::FrameSinkId& embed_frame_sink_id) {
-    embed_frame_sink_id_ = embed_frame_sink_id;
-  }
+  void SetEmbedFrameSinkId(const viz::FrameSinkId& embed_frame_sink_id);
+
   // Returns whether this window is an embed window.
   bool IsEmbeddingClient() const;
 
@@ -529,6 +528,11 @@ class AURA_EXPORT Window : public ui::LayerDelegate,
   // Updates the layer name based on the window's name and id.
   void UpdateLayerName();
 
+  void RegisterFrameSinkId();
+  void UnregisterFrameSinkId();
+  bool registered_frame_sink_id_ = false;
+  bool disable_frame_sink_id_registration_ = false;
+
   // Window owns its corresponding WindowPort, but the ref is held as a raw
   // pointer in |port_| so that it can still be accessed during destruction.
   // This is important as deleting the WindowPort may result in trying to lookup

ui/aura/window_port.h

@@ -98,9 +98,6 @@ class AURA_EXPORT WindowPort {
   // This can return invalid FrameSinkId.
   virtual viz::FrameSinkId GetFrameSinkId() const = 0;
 
-  virtual void OnWindowAddedToRootWindow() = 0;
-  virtual void OnWillRemoveWindowFromRootWindow() = 0;
-
   virtual void OnEventTargetingPolicyChanged() = 0;
 
   // See description of function with same name in transient_window_client.

ui/aura/window_port_for_shutdown.cc

@@ -67,10 +67,6 @@ viz::FrameSinkId WindowPortForShutdown::GetFrameSinkId() const {
   return frame_sink_id_;
 }
 
-void WindowPortForShutdown::OnWindowAddedToRootWindow() {}
-
-void WindowPortForShutdown::OnWillRemoveWindowFromRootWindow() {}
-
 void WindowPortForShutdown::OnEventTargetingPolicyChanged() {}
 
 bool WindowPortForShutdown::ShouldRestackTransientChildren() {