CrOS Shelf: Move shelf component ownership from shelf widget -> shelf

Also fix a memory management issue that was latent beforehand,
use the |DeleteDelegate| pattern that the shelf widget is already using,
for the navigation widget, since a widget delegate must outlive its
widget.

This makes it more clear that all the shelf components (nav widget,
hotseat, status area, shelf widget) are now meant to play a similar
role in the class hierarchy.

This change isn't expected to have any impact on user-visible behavior.

Change-Id: I78688ec4f4fda7185dfcdf9e260c94f083d94923
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032188
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Andrew Xu <andrewxu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737214}

ash/shelf/shelf.cc

@@ -8,6 +8,7 @@
 
 #include "ash/animation/animation_change_type.h"
 #include "ash/app_list/app_list_controller_impl.h"
+#include "ash/focus_cycler.h"
 #include "ash/public/cpp/ash_features.h"
 #include "ash/public/cpp/ash_switches.h"
 #include "ash/public/cpp/keyboard/keyboard_controller_observer.h"
@@ -15,14 +16,17 @@
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/root_window_controller.h"
+#include "ash/shelf/hotseat_widget.h"
 #include "ash/shelf/scrollable_shelf_view.h"
 #include "ash/shelf/shelf_controller.h"
 #include "ash/shelf/shelf_focus_cycler.h"
 #include "ash/shelf/shelf_layout_manager.h"
+#include "ash/shelf/shelf_navigation_widget.h"
 #include "ash/shelf/shelf_observer.h"
 #include "ash/shelf/shelf_tooltip_manager.h"
 #include "ash/shelf/shelf_widget.h"
 #include "ash/shell.h"
+#include "ash/system/status_area_layout_manager.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/wm/work_area_insets.h"
 #include "base/bind_helpers.h"
@@ -200,6 +204,34 @@ void Shelf::ActivateShelfItemOnDisplay(int item_index, int64_t display_id) {
                               base::DoNothing());
 }
 
+void Shelf::CreateNavigationWidget(aura::Window* container) {
+  DCHECK(container);
+  DCHECK(!navigation_widget_);
+  navigation_widget_ = std::make_unique<ShelfNavigationWidget>(
+      this, hotseat_widget()->GetShelfView());
+  navigation_widget_->Initialize(container);
+  Shell::Get()->focus_cycler()->AddWidget(navigation_widget_.get());
+}
+
+void Shelf::CreateHotseatWidget(aura::Window* container) {
+  DCHECK(container);
+  DCHECK(!hotseat_widget_);
+  hotseat_widget_ = std::make_unique<HotseatWidget>();
+  hotseat_widget_->Initialize(container, this);
+  shelf_widget_->RegisterHotseatWidget(hotseat_widget());
+}
+
+void Shelf::CreateStatusAreaWidget(aura::Window* status_container) {
+  DCHECK(status_container);
+  DCHECK(!status_area_widget_);
+  status_area_widget_ =
+      std::make_unique<StatusAreaWidget>(status_container, this);
+  status_area_widget_->Initialize();
+  Shell::Get()->focus_cycler()->AddWidget(status_area_widget_.get());
+  status_container->SetLayoutManager(
+      new StatusAreaLayoutManager(shelf_widget()));
+}
+
 void Shelf::CreateShelfWidget(aura::Window* root) {
   DCHECK(!shelf_widget_);
   aura::Window* shelf_container =
@@ -210,27 +242,31 @@ void Shelf::CreateShelfWidget(aura::Window* root) {
   shelf_layout_manager_ = shelf_widget_->shelf_layout_manager();
   shelf_layout_manager_->AddObserver(this);
 
-  DCHECK(!shelf_widget_->hotseat_widget());
+  // Create the various shelf components.
   aura::Window* control_container =
       root->GetChildById(kShellWindowId_ShelfControlContainer);
-  shelf_widget_->CreateHotseatWidget(control_container);
 
-  DCHECK(!shelf_widget_->navigation_widget());
-  shelf_widget_->CreateNavigationWidget(control_container);
+  CreateHotseatWidget(control_container);
+  CreateNavigationWidget(control_container);
 
   // Must occur after |shelf_widget_| is constructed because the system tray
   // constructors call back into Shelf::shelf_widget().
-  DCHECK(!shelf_widget_->status_area_widget());
   aura::Window* status_container =
       root->GetChildById(kShellWindowId_ShelfControlContainer);
-  shelf_widget_->CreateStatusAreaWidget(status_container);
+  CreateStatusAreaWidget(status_container);
   shelf_widget_->Initialize(shelf_container);
 
   // The Hotseat should be above everything in the shelf.
-  shelf_widget_->hotseat_widget()->StackAtTop();
+  hotseat_widget()->StackAtTop();
 }
 
 void Shelf::ShutdownShelfWidget() {
+  // The contents view of the hotseat widget may rely on the status area widget.
+  // So do explicit destruction here.
+  hotseat_widget_.reset();
+  status_area_widget_.reset();
+  navigation_widget_.reset();
+
   shelf_widget_->Shutdown();
 }
 
@@ -266,6 +302,9 @@ void Shelf::SetAlignment(ShelfAlignment alignment) {
 
   ShelfAlignment old_alignment = alignment_;
   alignment_ = alignment;
+  // Check added for http://crbug.com/738011.
+  CHECK(status_area_widget_);
+  status_area_widget()->UpdateAfterShelfAlignmentChange();
   // The ShelfWidget notifies the ShelfView of the alignment change.
   shelf_widget_->OnShelfAlignmentChanged();
   tooltip_->Close();

ash/shelf/shelf.h

@@ -34,10 +34,12 @@ class View;
 namespace ash {
 
 enum class AnimationChangeType;
+class HotseatWidget;
 class ShelfFocusCycler;
 class ShelfLayoutManager;
 class ShelfLayoutManagerTest;
 class ShelfLockingManager;
+class ShelfNavigationWidget;
 class ShelfView;
 class ShelfWidget;
 class StatusAreaWidget;
@@ -85,6 +87,9 @@ class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
   // on the display identified by |display_id|.
   static void ActivateShelfItemOnDisplay(int item_index, int64_t display_id);
 
+  void CreateNavigationWidget(aura::Window* container);
+  void CreateHotseatWidget(aura::Window* container);
+  void CreateStatusAreaWidget(aura::Window* status_container);
   void CreateShelfWidget(aura::Window* root);
   void ShutdownShelfWidget();
   void DestroyShelfWidget();
@@ -194,7 +199,17 @@ class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
   ShelfLayoutManager* shelf_layout_manager() const {
     return shelf_layout_manager_;
   }
+
+  // Getters for the various shelf components.
   ShelfWidget* shelf_widget() const { return shelf_widget_.get(); }
+  ShelfNavigationWidget* navigation_widget() const {
+    return navigation_widget_.get();
+  }
+  HotseatWidget* hotseat_widget() const { return hotseat_widget_.get(); }
+  StatusAreaWidget* status_area_widget() const {
+    return status_area_widget_.get();
+  }
+
   ShelfAlignment alignment() const { return alignment_; }
   ShelfAutoHideBehavior auto_hide_behavior() const {
     return auto_hide_behavior_;
@@ -238,6 +253,10 @@ class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
   // ShelfWidget and lifetimes are managed by the container windows themselves.
   ShelfLayoutManager* shelf_layout_manager_ = nullptr;
 
+  // Pointers to shelf components.
+  std::unique_ptr<ShelfNavigationWidget> navigation_widget_;
+  std::unique_ptr<HotseatWidget> hotseat_widget_;
+  std::unique_ptr<StatusAreaWidget> status_area_widget_;
   // Null during display teardown, see WindowTreeHostManager::DeleteHost() and
   // RootWindowController::CloseAllChildWindows().
   std::unique_ptr<ShelfWidget> shelf_widget_;

ash/shelf/shelf_navigation_widget.cc

@@ -117,6 +117,7 @@ class ShelfNavigationWidget::Delegate : public views::AccessiblePaneView,
   View* GetDefaultFocusableChild() override;
 
   // views::WidgetDelegate:
+  void DeleteDelegate() override;
   bool CanActivate() const override;
 
   BackButton* back_button() const { return back_button_; }
@@ -143,6 +144,7 @@ class ShelfNavigationWidget::Delegate : public views::AccessiblePaneView,
 
 ShelfNavigationWidget::Delegate::Delegate(Shelf* shelf, ShelfView* shelf_view)
     : opaque_background_(ui::LAYER_SOLID_COLOR) {
+  set_owned_by_client();  // Deleted by DeleteDelegate().
   set_allow_deactivate_on_esc(true);
 
   const int control_size = ShelfConfig::Get()->control_size();
@@ -211,6 +213,10 @@ bool ShelfNavigationWidget::Delegate::CanActivate() const {
   return Shell::Get()->focus_cycler()->widget_activating() == GetWidget();
 }
 
+void ShelfNavigationWidget::Delegate::DeleteDelegate() {
+  delete this;
+}
+
 views::FocusTraversable*
 ShelfNavigationWidget::Delegate::GetPaneFocusTraversable() {
   return this;

ash/shelf/shelf_widget.cc

@@ -25,14 +25,12 @@
 #include "ash/shelf/overflow_bubble.h"
 #include "ash/shelf/overflow_bubble_view.h"
 #include "ash/shelf/scrollable_shelf_view.h"
-#include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_background_animator_observer.h"
 #include "ash/shelf/shelf_layout_manager.h"
 #include "ash/shelf/shelf_navigation_widget.h"
 #include "ash/shelf/shelf_view.h"
 #include "ash/shell.h"
 #include "ash/style/ash_color_provider.h"
-#include "ash/system/status_area_layout_manager.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/command_line.h"
@@ -480,7 +478,7 @@ ShelfWidget::~ShelfWidget() {
   Shell::Get()->accessibility_controller()->RemoveObserver(this);
 
   // Must call Shutdown() before destruction.
-  DCHECK(!status_area_widget_);
+  DCHECK(!status_area_widget());
 }
 
 void ShelfWidget::Initialize(aura::Window* shelf_container) {
@@ -533,10 +531,9 @@ void ShelfWidget::Shutdown() {
   // access it later in shutdown.
   shelf_layout_manager_->PrepareForShutdown();
 
-  Shell::Get()->focus_cycler()->RemoveWidget(status_area_widget_.get());
-
-  Shell::Get()->focus_cycler()->RemoveWidget(navigation_widget_.get());
-  Shell::Get()->focus_cycler()->RemoveWidget(hotseat_widget_.get());
+  Shell::Get()->focus_cycler()->RemoveWidget(shelf_->status_area_widget());
+  Shell::Get()->focus_cycler()->RemoveWidget(navigation_widget());
+  Shell::Get()->focus_cycler()->RemoveWidget(hotseat_widget());
 
   // Don't need to update the shelf background during shutdown.
   background_animator_.RemoveObserver(delegate_view_);
@@ -545,43 +542,6 @@ void ShelfWidget::Shutdown() {
   // Don't need to observe focus/activation during shutdown.
   Shell::Get()->focus_cycler()->RemoveWidget(this);
   SetFocusCycler(nullptr);
-
-  // The contents view of |hotseat_widget_| may rely on |status_area_widget_|.
-  // So do explicit destruction here.
-  hotseat_widget_.reset();
-
-  status_area_widget_.reset();
-}
-
-void ShelfWidget::CreateNavigationWidget(aura::Window* container) {
-  DCHECK(container);
-  DCHECK(!navigation_widget_);
-  navigation_widget_ = std::make_unique<ShelfNavigationWidget>(
-      shelf_, hotseat_widget()->GetShelfView());
-  navigation_widget_->Initialize(container);
-  Shell::Get()->focus_cycler()->AddWidget(navigation_widget_.get());
-}
-
-void ShelfWidget::CreateHotseatWidget(aura::Window* container) {
-  DCHECK(container);
-  DCHECK(!hotseat_widget_);
-  hotseat_widget_ = std::make_unique<HotseatWidget>();
-  hotseat_widget_->Initialize(container, shelf_);
-
-  // Show a context menu for right clicks anywhere on the shelf widget.
-  delegate_view_->set_context_menu_controller(hotseat_widget_->GetShelfView());
-  hotseat_transition_animator_.reset(new HotseatTransitionAnimator(this));
-  hotseat_transition_animator_->AddObserver(delegate_view_);
-}
-
-void ShelfWidget::CreateStatusAreaWidget(aura::Window* status_container) {
-  DCHECK(status_container);
-  DCHECK(!status_area_widget_);
-  status_area_widget_ =
-      std::make_unique<StatusAreaWidget>(status_container, shelf_);
-  status_area_widget_->Initialize();
-  Shell::Get()->focus_cycler()->AddWidget(status_area_widget_.get());
-  status_container->SetLayoutManager(new StatusAreaLayoutManager(this));
 }
 
 ShelfBackgroundType ShelfWidget::GetBackgroundType() const {
@@ -593,10 +553,14 @@ int ShelfWidget::GetBackgroundAlphaValue(
   return SkColorGetA(background_animator_.GetBackgroundColor(background_type));
 }
 
+void ShelfWidget::RegisterHotseatWidget(HotseatWidget* hotseat_widget) {
+  // Show a context menu for right clicks anywhere on the shelf widget.
+  delegate_view_->set_context_menu_controller(hotseat_widget->GetShelfView());
+  hotseat_transition_animator_.reset(new HotseatTransitionAnimator(this));
+  hotseat_transition_animator_->AddObserver(delegate_view_);
+}
+
 void ShelfWidget::OnShelfAlignmentChanged() {
-  // Check added for http://crbug.com/738011.
-  CHECK(status_area_widget_);
-  status_area_widget_->UpdateAfterShelfAlignmentChange();
   // This call will in turn trigger a call to delegate_view_->SchedulePaint().
   delegate_view_->UpdateOpaqueBackground();
 }

ash/shelf/shelf_widget.h

@@ -13,6 +13,7 @@
 #include "ash/session/session_observer.h"
 #include "ash/shelf/hotseat_transition_animator.h"
 #include "ash/shelf/hotseat_widget.h"
+#include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_background_animator.h"
 #include "ash/shelf/shelf_component.h"
 #include "ash/shelf/shelf_layout_manager_observer.h"
@@ -57,10 +58,6 @@ class ASH_EXPORT ShelfWidget : public AccessibilityObserver,
   // Returns true if the views-based shelf is being shown.
   static bool IsUsingViewsShelf();
 
-  void CreateNavigationWidget(aura::Window* container);
-  void CreateHotseatWidget(aura::Window* container);
-  void CreateStatusAreaWidget(aura::Window* status_container);
-
   void OnShelfAlignmentChanged();
 
   void OnTabletModeChanged();
@@ -71,18 +68,18 @@ class ASH_EXPORT ShelfWidget : public AccessibilityObserver,
   int GetBackgroundAlphaValue(ShelfBackgroundType background_type) const;
 
   const Shelf* shelf() const { return shelf_; }
+  void RegisterHotseatWidget(HotseatWidget* hotseat_widget);
   ShelfLayoutManager* shelf_layout_manager() { return shelf_layout_manager_; }
 
-  // TODO(manucornet): Move these three getters directly to |Shelf| to make it
-  // clear that they are on the same level as the shelf widget.
+  // TODO(manucornet): Remove these getters once all callers get the shelf
+  // components from the shelf directly.
   ShelfNavigationWidget* navigation_widget() const {
-    return navigation_widget_.get();
+    return shelf_->navigation_widget();
   }
-  HotseatWidget* hotseat_widget() const { return hotseat_widget_.get(); }
+  HotseatWidget* hotseat_widget() const { return shelf_->hotseat_widget(); }
   StatusAreaWidget* status_area_widget() const {
-    return status_area_widget_.get();
+    return shelf_->status_area_widget();
   }
-
   void PostCreateShelf();
 
   bool IsShowingAppList() const;
@@ -186,11 +183,6 @@ class ASH_EXPORT ShelfWidget : public AccessibilityObserver,
   // Owned by the shelf container's window.
   ShelfLayoutManager* shelf_layout_manager_;
 
-  // Pointers to widgets that are visually part of the shelf.
-  std::unique_ptr<ShelfNavigationWidget> navigation_widget_;
-  std::unique_ptr<HotseatWidget> hotseat_widget_;
-  std::unique_ptr<StatusAreaWidget> status_area_widget_;
-
   // |delegate_view_| is the contents view of this widget and is cleaned up
   // during CloseChildWindows of the associated RootWindowController.
   DelegateView* delegate_view_;