Simplify/clean up ArcCustomTabView.

Bug: none
Change-Id: Iced87f066a842db371a7936ecff0644caac2e7b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218680
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773794}

ash/custom_tab/arc_custom_tab_view.cc

@@ -12,7 +12,6 @@
 #include "components/exo/surface.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_targeter.h"
-#include "ui/views/controls/native/native_view_host.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
@@ -46,11 +45,10 @@ std::unique_ptr<ArcCustomTab> ArcCustomTab::Create(aura::Window* arc_app_window,
     return nullptr;
   }
   auto* parent = widget->widget_delegate()->GetContentsView();
+  parent->SetLayoutManager(std::make_unique<views::FillLayout>());
   auto view = std::make_unique<ArcCustomTabView>(arc_app_window, surface_id,
                                                  top_margin);
   parent->AddChildView(view.get());
-  parent->SetLayoutManager(std::make_unique<views::FillLayout>());
-  parent->Layout();
 
   return std::move(view);
 }
@@ -58,136 +56,92 @@ std::unique_ptr<ArcCustomTab> ArcCustomTab::Create(aura::Window* arc_app_window,
 ArcCustomTabView::ArcCustomTabView(aura::Window* arc_app_window,
                                    int32_t surface_id,
                                    int32_t top_margin)
-    : host_(new views::NativeViewHost()),
-      arc_app_window_(arc_app_window),
+    : arc_app_window_(arc_app_window),
       surface_id_(surface_id),
       top_margin_(top_margin) {
-  AddChildView(host_);
-  arc_app_window_->AddObserver(this);
   set_owned_by_client();
+  other_windows_observer_.Add(arc_app_window_);
 }
 
-ArcCustomTabView::~ArcCustomTabView() {
-  for (auto* window : observed_surfaces_)
-    window->RemoveObserver(this);
-  arc_app_window_->RemoveObserver(this);
-
-  if (surface_window_)
-    surface_window_->RemoveObserver(this);
-  if (native_view_container_)
-    native_view_container_->RemoveObserver(this);
-}
+ArcCustomTabView::~ArcCustomTabView() = default;
 
 void ArcCustomTabView::Attach(gfx::NativeView view) {
   DCHECK(view);
-  DCHECK(!host_->native_view());
+  DCHECK(!GetHostView());
   host_->Attach(view);
-  native_view_container_ = host_->GetNativeViewContainer();
-  native_view_container_->SetEventTargeter(
-      std::make_unique<aura::WindowTargeter>());
-  native_view_container_->parent()->StackChildAtTop(native_view_container_);
-  native_view_container_->AddObserver(this);
+  aura::Window* const container = host_->GetNativeViewContainer();
+  container->SetEventTargeter(std::make_unique<aura::WindowTargeter>());
+  other_windows_observer_.Add(container);
+  EnsureWindowOrders();
+  UpdateSurfaceIfNecessary();
 }
 
 gfx::NativeView ArcCustomTabView::GetHostView() {
   return host_->native_view();
 }
 
-void ArcCustomTabView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  if (previous_bounds.size() != size()) {
-    InvalidateLayout();
-    host_->InvalidateLayout();
-  }
-}
-
-void ArcCustomTabView::Layout() {
-  exo::Surface* surface = FindSurface();
-  if (!surface)
-    return;
-  DCHECK(observed_surfaces_.empty());
-  aura::Window* surface_window = surface->window();
-  if (surface_window_ != surface_window) {
-    if (surface_window_)
-      surface_window_->RemoveObserver(this);
-    surface_window->AddObserver(this);
-    surface_window_ = surface_window;
-  }
-  gfx::Point topleft(0, top_margin_),
-      bottomright(surface_window->bounds().width(),
-                  surface_window->bounds().height());
-  ConvertPointFromWindow(surface_window, &topleft);
-  ConvertPointFromWindow(surface_window, &bottomright);
-  gfx::Rect bounds(topleft, gfx::Size(bottomright.x() - topleft.x(),
-                                      bottomright.y() - topleft.y()));
-  host_->SetBoundsRect(bounds);
-}
-
 void ArcCustomTabView::OnWindowHierarchyChanged(
     const HierarchyChangeParams& params) {
-  if (params.receiver == arc_app_window_) {
-    auto* surface = exo::Surface::AsSurface(params.target);
-    if (surface && params.new_parent != nullptr) {
-      // Call Layout() aggressively without checking the surface ID to start
-      // observing surface ID updates in case it's not set yet.
-      Layout();
-    }
-  }
+  if ((params.receiver == arc_app_window_) &&
+      exo::Surface::AsSurface(params.target) && params.new_parent)
+    UpdateSurfaceIfNecessary();
 }
 
 void ArcCustomTabView::OnWindowBoundsChanged(aura::Window* window,
                                              const gfx::Rect& old_bounds,
                                              const gfx::Rect& new_bounds,
                                              ui::PropertyChangeReason reason) {
-  if (window == surface_window_ && old_bounds.size() != new_bounds.size()) {
-    InvalidateLayout();
-    host_->InvalidateLayout();
-  }
+  if (surface_window_observer_.IsObserving(window) &&
+      old_bounds.size() != new_bounds.size())
+    OnSurfaceBoundsMaybeChanged(window);
 }
 
 void ArcCustomTabView::OnWindowPropertyChanged(aura::Window* window,
                                                const void* key,
                                                intptr_t old) {
-  if (observed_surfaces_.contains(window)) {
-    if (key == exo::kClientSurfaceIdKey) {
-      // Client surface ID was updated. Try to find the surface again.
-      Layout();
-    }
-  }
+  if (surfaces_observer_.IsObserving(window) && key == exo::kClientSurfaceIdKey)
+    UpdateSurfaceIfNecessary();
 }
 
 void ArcCustomTabView::OnWindowStackingChanged(aura::Window* window) {
-  if (window != host_->GetNativeViewContainer() || reorder_scheduled_)
-    return;
-  reorder_scheduled_ = true;
-  // Reordering should happen asynchronously -- some entity (like
-  // views::WindowReorderer) changes the window orders, and then ensures layer
-  // orders later. Changing order here synchronously leads to inconsistent
-  // window/layer ordering and causes weird graphical effects.
-  // TODO(hashimoto): fix the views ordering and remove this handling.
-  base::SequencedTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(&ArcCustomTabView::EnsureWindowOrders,
-                                weak_ptr_factory_.GetWeakPtr()));
+  if (window == host_->GetNativeViewContainer() &&
+      !weak_ptr_factory_.HasWeakPtrs()) {
+    // Reordering should happen asynchronously -- some entity (like
+    // views::WindowReorderer) changes the window orders, and then ensures layer
+    // orders later. Changing order here synchronously leads to inconsistent
+    // window/layer ordering and causes weird graphical effects.
+    // TODO(hashimoto): fix the views ordering and remove this handling.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(&ArcCustomTabView::EnsureWindowOrders,
+                                  weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 void ArcCustomTabView::OnWindowDestroying(aura::Window* window) {
-  if (observed_surfaces_.contains(window)) {
-    window->RemoveObserver(this);
-    observed_surfaces_.erase(window);
-  }
-  if (window == surface_window_) {
-    window->RemoveObserver(this);
-    surface_window_ = nullptr;
-  }
-  if (window == native_view_container_) {
-    window->RemoveObserver(this);
-    native_view_container_ = nullptr;
-  }
+  if (surfaces_observer_.IsObserving(window))
+    surfaces_observer_.Remove(window);
+  if (surface_window_observer_.IsObserving(window))
+    surface_window_observer_.Remove(window);
+  if (other_windows_observer_.IsObserving(window))
+    other_windows_observer_.Remove(window);
+}
+
+void ArcCustomTabView::OnSurfaceBoundsMaybeChanged(
+    aura::Window* surface_window) {
+  DCHECK(surface_window);
+  gfx::Point origin(0, top_margin_);
+  gfx::Point bottom_right(surface_window->bounds().width(),
+                          surface_window->bounds().height());
+  ConvertPointFromWindow(surface_window, &origin);
+  ConvertPointFromWindow(surface_window, &bottom_right);
+  host_->SetBounds(origin.x(), origin.y(), bottom_right.x() - origin.x(),
+                   bottom_right.y() - origin.y());
 }
 
 void ArcCustomTabView::EnsureWindowOrders() {
-  reorder_scheduled_ = false;
-  if (native_view_container_)
-    native_view_container_->parent()->StackChildAtTop(native_view_container_);
+  aura::Window* const container = host_->GetNativeViewContainer();
+  if (container)
+    container->parent()->StackChildAtTop(container);
 }
 
 void ArcCustomTabView::ConvertPointFromWindow(aura::Window* window,
@@ -197,28 +151,31 @@ void ArcCustomTabView::ConvertPointFromWindow(aura::Window* window,
   views::View::ConvertPointFromWidget(parent(), point);
 }
 
-exo::Surface* ArcCustomTabView::FindSurface() {
+void ArcCustomTabView::UpdateSurfaceIfNecessary() {
   std::vector<exo::Surface*> surfaces;
   EnumerateSurfaces(arc_app_window_, &surfaces);
 
   // Try to find the surface.
-  for (auto* surface : surfaces) {
-    if (surface->GetClientSurfaceId() == surface_id_) {
-      // Stop observing surfaces for ID updates.
-      for (auto* window : observed_surfaces_)
-        window->RemoveObserver(this);
-      observed_surfaces_.clear();
-      return surface;
+  const auto it = std::find_if(surfaces.cbegin(), surfaces.cend(),
+                               [id = surface_id_](const auto* surface) {
+                                 return surface->GetClientSurfaceId() == id;
+                               });
+  if (it == surfaces.cend()) {
+    for (auto* surface : surfaces) {
+      if (!surface->GetClientSurfaceId() &&
+          !surfaces_observer_.IsObserving(surface->window()))
+        surfaces_observer_.Add(surface->window());
     }
-  }
-  // Surface not found. Start observing surfaces for ID updates.
-  for (auto* surface : surfaces) {
-    if (surface->GetClientSurfaceId() == 0) {
-      if (observed_surfaces_.insert(surface->window()).second)
-        surface->window()->AddObserver(this);
+  } else {
+    surfaces_observer_.RemoveAll();
+
+    auto* const window = (*it)->window();
+    if (!surface_window_observer_.IsObserving(window)) {
+      surface_window_observer_.RemoveAll();
+      surface_window_observer_.Add(window);
+      OnSurfaceBoundsMaybeChanged(window);
     }
   }
-  return nullptr;
 }
 
 }  // namespace ash

ash/custom_tab/arc_custom_tab_view.h

@@ -8,19 +8,12 @@
 #include <memory>
 
 #include "ash/public/cpp/arc_custom_tab.h"
-#include "base/containers/flat_set.h"
-#include "base/macros.h"
+#include "base/scoped_observer.h"
+#include "ui/aura/window.h"
 #include "ui/aura/window_observer.h"
+#include "ui/views/controls/native/native_view_host.h"
 #include "ui/views/view.h"
 
-namespace exo {
-class Surface;
-}
-
-namespace views {
-class NativeViewHost;
-}
-
 namespace ash {
 
 // A view-based implementation of ArcCustomTab which works in the classic
@@ -32,6 +25,8 @@ class ArcCustomTabView : public ArcCustomTab,
   ArcCustomTabView(aura::Window* arc_app_window,
                    int32_t surface_id,
                    int32_t top_margin);
+  ArcCustomTabView(const ArcCustomTabView&) = delete;
+  ArcCustomTabView& operator=(const ArcCustomTabView&) = delete;
   ~ArcCustomTabView() override;
 
  private:
@@ -39,10 +34,6 @@ class ArcCustomTabView : public ArcCustomTab,
   void Attach(gfx::NativeView view) override;
   gfx::NativeView GetHostView() override;
 
-  // views::View:
-  void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
-  void Layout() override;
-
   // aura::WindowObserver:
   void OnWindowHierarchyChanged(const HierarchyChangeParams& params) override;
   void OnWindowBoundsChanged(aura::Window* window,
@@ -55,27 +46,29 @@ class ArcCustomTabView : public ArcCustomTab,
   void OnWindowStackingChanged(aura::Window* window) override;
   void OnWindowDestroying(aura::Window* window) override;
 
+  // Updates |host_|'s bounds to deal with changes in the bounds of the
+  // associated |surface_window|.
+  void OnSurfaceBoundsMaybeChanged(aura::Window* surface_window);
+
   // Ensures the window/layer orders for the NativeViewHost.
   void EnsureWindowOrders();
 
   // Converts the point from the given window to this view.
   void ConvertPointFromWindow(aura::Window* window, gfx::Point* point);
 
-  // Tries to find the surface.
-  exo::Surface* FindSurface();
+  // Looks for the surface with |surface_id_|, and handles resultant changes.
+  void UpdateSurfaceIfNecessary();
 
-  views::NativeViewHost* const host_;
+  views::NativeViewHost* const host_ =
+      AddChildView(std::make_unique<views::NativeViewHost>());
   aura::Window* const arc_app_window_;
   const int32_t surface_id_, top_margin_;
-  aura::Window* surface_window_ = nullptr;
-  base::flat_set<aura::Window*> observed_surfaces_;
-  aura::Window* native_view_container_ = nullptr;
-
-  bool reorder_scheduled_ = false;
-
+  ScopedObserver<aura::Window, aura::WindowObserver> surfaces_observer_{this};
+  ScopedObserver<aura::Window, aura::WindowObserver> surface_window_observer_{
+      this};
+  ScopedObserver<aura::Window, aura::WindowObserver> other_windows_observer_{
+      this};
   base::WeakPtrFactory<ArcCustomTabView> weak_ptr_factory_{this};
-
-  DISALLOW_COPY_AND_ASSIGN(ArcCustomTabView);
 };
 
 }  // namespace ash