ArcCustomTab cleanup, part 2.

ArcCustomTabView was the lone implementation of this abstract base.
Merge the two classes and simplify the construction to happen directly,
rather than through a factory method.

Further simplify the class by making it not be a View override.
ArcCustomTabView didn't do anything related to being a View other than
call GetWidget(), which it can just as easily call on (what was) its
child NativeViewHost.  Now that view is made a direct child of the
widget ContentsView, without the ArcCustomTabView in between.  For now,
lifetimes stay the same -- the NativeViewHost becomes
set_owned_by_client() instead of |this| in order to preserve the
property that the custom tab and its |host_| are deleted together,
irrespective of the lifetime of the widget ContentsView.

Bug: none
Change-Id: I921189cf74b63cc34d3789332e1b53a627e83602
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2225493Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774344}

ash/BUILD.gn

@@ -241,8 +241,7 @@ component("ash") {
     "bluetooth_devices_observer.h",
     "cancel_mode.cc",
     "cancel_mode.h",
-    "custom_tab/arc_custom_tab_view.cc",
-    "custom_tab/arc_custom_tab_view.h",
+    "custom_tab/arc_custom_tab.cc",
     "dbus/ash_dbus_services.cc",
     "dbus/ash_dbus_services.h",
     "dbus/display_service_provider.cc",

ash/custom_tab/arc_custom_tab_view.cc → ash/custom_tab/arc_custom_tab.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "ash/custom_tab/arc_custom_tab_view.h"
+#include "ash/public/cpp/arc_custom_tab.h"
 
 #include <memory>
 #include <string>
@@ -12,7 +12,6 @@
 #include "components/exo/surface.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_targeter.h"
-#include "ui/views/layout/fill_layout.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
@@ -31,41 +30,23 @@ void EnumerateSurfaces(aura::Window* window, std::vector<exo::Surface*>* out) {
 
 }  // namespace
 
-ArcCustomTab::ArcCustomTab() = default;
-ArcCustomTab::~ArcCustomTab() = default;
-
-// static
-std::unique_ptr<ArcCustomTab> ArcCustomTab::Create(aura::Window* arc_app_window,
-                                                   int32_t surface_id,
-                                                   int32_t top_margin) {
-  views::Widget* widget =
-      views::Widget::GetWidgetForNativeWindow(arc_app_window);
-  if (!widget) {
-    LOG(ERROR) << "No widget for the 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());
-
-  return std::move(view);
-}
-
-ArcCustomTabView::ArcCustomTabView(aura::Window* arc_app_window,
-                                   int32_t surface_id,
-                                   int32_t top_margin)
+ArcCustomTab::ArcCustomTab(aura::Window* arc_app_window,
+                           int32_t surface_id,
+                           int32_t top_margin)
     : arc_app_window_(arc_app_window),
       surface_id_(surface_id),
       top_margin_(top_margin) {
-  set_owned_by_client();
   other_windows_observer_.Add(arc_app_window_);
+
+  host_->set_owned_by_client();
+  auto* const widget = views::Widget::GetWidgetForNativeWindow(arc_app_window_);
+  DCHECK(widget);
+  widget->GetContentsView()->AddChildView(host_.get());
 }
 
-ArcCustomTabView::~ArcCustomTabView() = default;
+ArcCustomTab::~ArcCustomTab() = default;
 
-void ArcCustomTabView::Attach(gfx::NativeView view) {
+void ArcCustomTab::Attach(gfx::NativeView view) {
   DCHECK(view);
   DCHECK(!GetHostView());
   host_->Attach(view);
@@ -76,34 +57,34 @@ void ArcCustomTabView::Attach(gfx::NativeView view) {
   UpdateSurfaceIfNecessary();
 }
 
-gfx::NativeView ArcCustomTabView::GetHostView() {
+gfx::NativeView ArcCustomTab::GetHostView() {
   return host_->native_view();
 }
 
-void ArcCustomTabView::OnWindowHierarchyChanged(
+void ArcCustomTab::OnWindowHierarchyChanged(
     const HierarchyChangeParams& params) {
   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) {
+void ArcCustomTab::OnWindowBoundsChanged(aura::Window* window,
+                                         const gfx::Rect& old_bounds,
+                                         const gfx::Rect& new_bounds,
+                                         ui::PropertyChangeReason reason) {
   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) {
+void ArcCustomTab::OnWindowPropertyChanged(aura::Window* window,
+                                           const void* key,
+                                           intptr_t old) {
   if (surfaces_observer_.IsObserving(window) && key == exo::kClientSurfaceIdKey)
     UpdateSurfaceIfNecessary();
 }
 
-void ArcCustomTabView::OnWindowStackingChanged(aura::Window* window) {
+void ArcCustomTab::OnWindowStackingChanged(aura::Window* window) {
   if (window == host_->GetNativeViewContainer() &&
       !weak_ptr_factory_.HasWeakPtrs()) {
     // Reordering should happen asynchronously -- some entity (like
@@ -112,12 +93,12 @@ void ArcCustomTabView::OnWindowStackingChanged(aura::Window* window) {
     // 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,
+        FROM_HERE, base::BindOnce(&ArcCustomTab::EnsureWindowOrders,
                                   weak_ptr_factory_.GetWeakPtr()));
   }
 }
 
-void ArcCustomTabView::OnWindowDestroying(aura::Window* window) {
+void ArcCustomTab::OnWindowDestroying(aura::Window* window) {
   if (surfaces_observer_.IsObserving(window))
     surfaces_observer_.Remove(window);
   if (surface_window_observer_.IsObserving(window))
@@ -126,8 +107,7 @@ void ArcCustomTabView::OnWindowDestroying(aura::Window* window) {
     other_windows_observer_.Remove(window);
 }
 
-void ArcCustomTabView::OnSurfaceBoundsMaybeChanged(
-    aura::Window* surface_window) {
+void ArcCustomTab::OnSurfaceBoundsMaybeChanged(aura::Window* surface_window) {
   DCHECK(surface_window);
   gfx::Point origin(0, top_margin_);
   gfx::Point bottom_right(surface_window->bounds().width(),
@@ -138,20 +118,20 @@ void ArcCustomTabView::OnSurfaceBoundsMaybeChanged(
                    bottom_right.y() - origin.y());
 }
 
-void ArcCustomTabView::EnsureWindowOrders() {
+void ArcCustomTab::EnsureWindowOrders() {
   aura::Window* const container = host_->GetNativeViewContainer();
   if (container)
     container->parent()->StackChildAtTop(container);
 }
 
-void ArcCustomTabView::ConvertPointFromWindow(aura::Window* window,
-                                              gfx::Point* point) {
-  aura::Window::ConvertPointToTarget(window, GetWidget()->GetNativeWindow(),
-                                     point);
-  views::View::ConvertPointFromWidget(parent(), point);
+void ArcCustomTab::ConvertPointFromWindow(aura::Window* window,
+                                          gfx::Point* point) {
+  views::Widget* const widget = host_->GetWidget();
+  aura::Window::ConvertPointToTarget(window, widget->GetNativeWindow(), point);
+  views::View::ConvertPointFromWidget(widget->GetContentsView(), point);
 }
 
-void ArcCustomTabView::UpdateSurfaceIfNecessary() {
+void ArcCustomTab::UpdateSurfaceIfNecessary() {
   std::vector<exo::Surface*> surfaces;
   EnumerateSurfaces(arc_app_window_, &surfaces);
 

ash/custom_tab/arc_custom_tab_view.h

-// Copyright 2019 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 ASH_CUSTOM_TAB_ARC_CUSTOM_TAB_VIEW_H_
-#define ASH_CUSTOM_TAB_ARC_CUSTOM_TAB_VIEW_H_
-
-#include <memory>
-
-#include "ash/public/cpp/arc_custom_tab.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 ash {
-
-// A view-based implementation of ArcCustomTab which works in the classic
-// environment.
-class ArcCustomTabView : public ArcCustomTab,
-                         public views::View,
-                         public aura::WindowObserver {
- public:
-  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:
-  // ArcCustomTab:
-  void Attach(gfx::NativeView view) override;
-  gfx::NativeView GetHostView() override;
-
-  // aura::WindowObserver:
-  void OnWindowHierarchyChanged(const HierarchyChangeParams& params) override;
-  void OnWindowBoundsChanged(aura::Window* window,
-                             const gfx::Rect& old_bounds,
-                             const gfx::Rect& new_bounds,
-                             ui::PropertyChangeReason reason) override;
-  void OnWindowPropertyChanged(aura::Window* window,
-                               const void* key,
-                               intptr_t old) override;
-  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);
-
-  // Looks for the surface with |surface_id_|, and handles resultant changes.
-  void UpdateSurfaceIfNecessary();
-
-  views::NativeViewHost* const host_ =
-      AddChildView(std::make_unique<views::NativeViewHost>());
-  aura::Window* const arc_app_window_;
-  const int32_t surface_id_, top_margin_;
-  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};
-};
-
-}  // namespace ash
-
-#endif  // ASH_CUSTOM_TAB_ARC_CUSTOM_TAB_VIEW_H_

ash/public/cpp/arc_custom_tab.h

@@ -9,30 +9,66 @@
 
 #include "ash/ash_export.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_observer.h"
 #include "ui/gfx/native_widget_types.h"
+#include "ui/views/controls/native/native_view_host.h"
 
 namespace ash {
 
 // ArcCustomTab is responsible to embed an ARC++ custom tab.
-class ASH_EXPORT ArcCustomTab {
+class ASH_EXPORT ArcCustomTab : public aura::WindowObserver {
  public:
-  // Creates a new ArcCustomTab instance. Returns null when the arguments are
-  // invalid.
-  static std::unique_ptr<ArcCustomTab> Create(aura::Window* arc_app_window,
-                                              int32_t surface_id,
-                                              int32_t top_margin);
+  ArcCustomTab(aura::Window* arc_app_window,
+               int32_t surface_id,
+               int32_t top_margin);
+  ArcCustomTab(const ArcCustomTab&) = delete;
+  ArcCustomTab& operator=(const ArcCustomTab&) = delete;
+  ~ArcCustomTab() override;
 
-  ArcCustomTab();
-  virtual ~ArcCustomTab();
-
-  virtual void Attach(gfx::NativeView view) = 0;
+  void Attach(gfx::NativeView view);
 
   // Returns the view against which a view or dialog is positioned and parented
   // in an ArcCustomTab.
-  virtual gfx::NativeView GetHostView() = 0;
+  gfx::NativeView GetHostView();
+
+  // aura::WindowObserver:
+  void OnWindowHierarchyChanged(const HierarchyChangeParams& params) override;
+  void OnWindowBoundsChanged(aura::Window* window,
+                             const gfx::Rect& old_bounds,
+                             const gfx::Rect& new_bounds,
+                             ui::PropertyChangeReason reason) override;
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override;
+  void OnWindowStackingChanged(aura::Window* window) override;
+  void OnWindowDestroying(aura::Window* window) override;
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(ArcCustomTab);
+  // 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);
+
+  // Looks for the surface with |surface_id_|, and handles resultant changes.
+  void UpdateSurfaceIfNecessary();
+
+  std::unique_ptr<views::NativeViewHost> host_ =
+      std::make_unique<views::NativeViewHost>();
+  aura::Window* const arc_app_window_;
+  const int32_t surface_id_, top_margin_;
+  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<ArcCustomTab> weak_ptr_factory_{this};
 };
 
 }  // namespace ash

chrome/browser/chromeos/arc/intent_helper/custom_tab_session_impl_browsertest.cc

@@ -100,9 +100,9 @@ IN_PROC_BROWSER_TEST_F(CustomTabSessionImplTest,
       test_window.shell_surface()->GetWidget()->GetNativeWindow();
   ASSERT_TRUE(aura_window);
 
-  auto custom_tab = ash::ArcCustomTab::Create(aura_window, /* surface_id= */ 0,
-                                              /* top_margin= */ 0);
-  ASSERT_TRUE(custom_tab);
+  auto custom_tab = std::make_unique<ash::ArcCustomTab>(aura_window,
+                                                        /* surface_id= */ 0,
+                                                        /* top_margin= */ 0);
 
   auto web_contents = arc::CreateArcCustomTabWebContents(browser()->profile(),
                                                          GURL("http://foo/"));

chrome/browser/chromeos/arc/print_spooler/arc_print_spooler_bridge.cc

@@ -110,7 +110,7 @@ void ArcPrintSpoolerBridge::OnPrintDocumentSaved(
   }
 
   auto custom_tab =
-      ash::ArcCustomTab::Create(arc_window, surface_id, top_margin);
+      std::make_unique<ash::ArcCustomTab>(arc_window, surface_id, top_margin);
   auto web_contents = CreateArcCustomTabWebContents(profile_, url);
 
   // TODO(crbug.com/955171): Remove this temporary conversion to InterfacePtr

chrome/browser/chromeos/arc/print_spooler/print_session_impl.cc

@@ -212,7 +212,8 @@ mojo::PendingRemote<mojom::PrintSessionHost> PrintSessionImpl::Create(
     std::unique_ptr<content::WebContents> web_contents,
     std::unique_ptr<ash::ArcCustomTab> custom_tab,
     mojom::PrintSessionInstancePtr instance) {
-  if (!custom_tab || !instance)
+  DCHECK(custom_tab);
+  if (!instance)
     return mojo::NullRemote();
 
   // This object will be deleted when the mojo connection is closed.

chrome/browser/ui/ash/chrome_new_window_client.cc

@@ -523,7 +523,7 @@ void ChromeNewWindowClient::OpenArcCustomTab(
   }
 
   auto custom_tab =
-      ash::ArcCustomTab::Create(arc_window, surface_id, top_margin);
+      std::make_unique<ash::ArcCustomTab>(arc_window, surface_id, top_margin);
   auto web_contents = arc::CreateArcCustomTabWebContents(profile, url);
 
   // |custom_tab_browser| will be destroyed when its tab strip becomes empty,