Make SystemTrayView owned by the view hierarchy

Previously, I have added SystemTrayView class which was owned by SystemTray class (crrev.com/727379). But the views in the tray are assumed to be destroyed with the parent view and the bubble, so that caused some issues.
This patch changes that and makes SystemTrayView owned by the view hierarchy. Then the issues are solved.

This patch also updates the newly-added code in NetworkStateListDetaiedView because the code didn't assume that lifetime of views.

Bug: 790716, 789833
Change-Id: I62eca328780cabb7c5d68967cc1ddee7a1e23b9a
Reviewed-on: https://chromium-review.googlesource.com/813153
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Reviewed-by: Yuki Awano <yawano@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523701}

ash/system/network/network_state_list_detailed_view.cc

@@ -72,12 +72,14 @@ class NetworkStateListDetailedView::InfoBubble
   }
 
   ~InfoBubble() override {
-    // Anchor view can be destructed before info bubble is destructed. Call
-    // OnInfoBubbleDestroyed only if anchor view is live.
-    if (GetAnchorView())
+    // The detailed view can be destructed before info bubble is destructed.
+    // Call OnInfoBubbleDestroyed only if the detailed view is live.
+    if (detailed_view_)
       detailed_view_->OnInfoBubbleDestroyed();
   }
 
+  void OnNetworkStateListDetailedViewIsDeleting() { detailed_view_ = nullptr; }
+
  private:
   // View:
   gfx::Size CalculatePreferredSize() const override {
@@ -176,6 +178,8 @@ NetworkStateListDetailedView::NetworkStateListDetailedView(
       info_bubble_(nullptr) {}
 
 NetworkStateListDetailedView::~NetworkStateListDetailedView() {
+  if (info_bubble_)
+    info_bubble_->OnNetworkStateListDetailedViewIsDeleting();
   ResetInfoBubble();
 }
 

ash/system/network/network_state_list_detailed_view.h

@@ -13,8 +13,6 @@
 #include "base/memory/weak_ptr.h"
 
 namespace views {
-class BubbleDialogDelegateView;
-class Button;
 class Button;
 }
 
@@ -87,7 +85,7 @@ class ASH_EXPORT NetworkStateListDetailedView
   views::Button* settings_button_;
 
   // A small bubble for displaying network info.
-  views::BubbleDialogDelegateView* info_bubble_;
+  InfoBubble* info_bubble_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkStateListDetailedView);
 };

ash/system/tray/system_tray.cc

@@ -159,39 +159,48 @@ class PaddingTrayItem : public SystemTrayItem {
 class SystemBubbleWrapper {
  public:
   // Takes ownership of |bubble|.
-  explicit SystemBubbleWrapper(SystemTrayBubble* bubble)
-      : bubble_(bubble), is_persistent_(false) {}
+  SystemBubbleWrapper() = default;
+
+  ~SystemBubbleWrapper() {
+    if (system_tray_view())
+      system_tray_view()->DestroyItemViews();
+  }
 
   // Initializes the bubble view and creates |bubble_wrapper_|.
-  void InitView(TrayBackgroundView* tray,
+  void InitView(SystemTray* tray,
                 views::View* anchor,
                 const gfx::Insets& anchor_insets,
+                const std::vector<ash::SystemTrayItem*>& items,
+                SystemTrayView::SystemTrayType system_tray_type,
                 TrayBubbleView::InitParams* init_params,
                 bool is_persistent) {
     DCHECK(anchor);
 
+    bubble_ = std::make_unique<SystemTrayBubble>(tray);
     is_persistent_ = is_persistent;
 
     const LoginStatus login_status =
         Shell::Get()->session_controller()->login_status();
-    bubble_->InitView(anchor, login_status, init_params);
+    bubble_->InitView(anchor, items, system_tray_type, login_status,
+                      init_params);
     bubble_->bubble_view()->set_anchor_view_insets(anchor_insets);
     bubble_wrapper_ = std::make_unique<TrayBubbleWrapper>(
         tray, bubble_->bubble_view(), is_persistent);
   }
 
   // Convenience accessors:
-  SystemTrayBubble* bubble() const { return bubble_.get(); }
+  SystemTrayBubble* bubble() { return bubble_.get(); }
+  SystemTrayView* system_tray_view() { return bubble_->system_tray_view(); }
   SystemTrayView::SystemTrayType system_tray_type() const {
     return bubble_->system_tray_view()->system_tray_type();
   }
-  TrayBubbleView* bubble_view() const { return bubble_->bubble_view(); }
+  TrayBubbleView* bubble_view() { return bubble_->bubble_view(); }
   bool is_persistent() const { return is_persistent_; }
 
  private:
   std::unique_ptr<SystemTrayBubble> bubble_;
   std::unique_ptr<TrayBubbleWrapper> bubble_wrapper_;
-  bool is_persistent_;
+  bool is_persistent_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(SystemBubbleWrapper);
 };
@@ -208,9 +217,15 @@ SystemTray::SystemTray(Shelf* shelf) : TrayBackgroundView(shelf) {
 }
 
 SystemTray::~SystemTray() {
+  // On shutdown, views in the bubble might be destroyed after this. Clear the
+  // list of SystemTrayItems in the SystemTrayView, since they are owned by
+  // this class.
+  if (system_bubble_ && system_bubble_->system_tray_view())
+    system_bubble_->system_tray_view()->set_items(
+        std::vector<SystemTrayItem*>());
+
   // Destroy any child views that might have back pointers before ~View().
   system_bubble_.reset();
-  system_tray_view_.reset();
   for (const auto& item : items_)
     item->OnTrayViewDestroyed();
 }
@@ -462,15 +477,10 @@ void SystemTray::ShowItems(const std::vector<SystemTrayItem*>& items,
     } else {
       init_params.bg_color = kHeaderBackgroundColor;
     }
-    system_tray_view_ =
-        std::make_unique<SystemTrayView>(system_tray_type, items);
-    system_tray_view_->set_owned_by_client();
 
-    SystemTrayBubble* bubble =
-        new SystemTrayBubble(this, system_tray_view_.get());
-    system_bubble_ = std::make_unique<SystemBubbleWrapper>(bubble);
+    system_bubble_ = std::make_unique<SystemBubbleWrapper>();
     system_bubble_->InitView(this, GetBubbleAnchor(), GetBubbleAnchorInsets(),
-                             &init_params, persistent);
+                             items, system_tray_type, &init_params, persistent);
 
     // Record metrics for the system menu when the default view is invoked.
     if (!detailed)
@@ -597,7 +607,6 @@ views::TrayBubbleView* SystemTray::GetBubbleView() {
 
 void SystemTray::BubbleViewDestroyed() {
   if (system_bubble_) {
-    system_tray_view_->DestroyItemViews();
     system_bubble_->bubble()->BubbleViewDestroyed();
   }
 }
@@ -649,8 +658,6 @@ void SystemTray::ActivateBubble() {
 
 void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
   system_bubble_.reset();
-  if (system_tray_view_)
-    system_tray_view_->DestroyItemViews();
   // When closing a system bubble with the alternate shelf layout, we need to
   // turn off the active tinting of the shelf.
   if (full_system_tray_menu_) {
@@ -661,9 +668,9 @@ void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
 
 void SystemTray::RecordSystemMenuMetrics() {
   DCHECK(system_bubble_);
-  DCHECK(system_tray_view_);
 
-  system_tray_view_->RecordVisibleRowMetrics();
+  if (system_bubble_->system_tray_view())
+    system_bubble_->system_tray_view()->RecordVisibleRowMetrics();
 
   TrayBubbleView* bubble_view = system_bubble_->bubble_view();
   int num_rows = 0;

ash/system/tray/system_tray.h

@@ -202,9 +202,6 @@ class ASH_EXPORT SystemTray : public TrayBackgroundView {
   // Bubble for SystemTrayViews.
   std::unique_ptr<SystemBubbleWrapper> system_bubble_;
 
-  // View for system tray content. This lifetime is same as |system_bubble_|.
-  std::unique_ptr<SystemTrayView> system_tray_view_;
-
   // Keep track of the default view height so that when we create detailed
   // views directly (e.g. from a notification) we know what height to use.
   int default_bubble_height_ = 0;

ash/system/tray/system_tray_bubble.cc

@@ -68,11 +68,8 @@ class AnimationObserverDeleteLayer : public ui::ImplicitAnimationObserver {
 
 // SystemTrayBubble
 
-SystemTrayBubble::SystemTrayBubble(SystemTray* tray, SystemTrayView* view)
-    : tray_(tray),
-      system_tray_view_(view),
-      bubble_view_(nullptr),
-      autoclose_delay_(0) {}
+SystemTrayBubble::SystemTrayBubble(SystemTray* tray)
+    : tray_(tray), autoclose_delay_(0) {}
 
 SystemTrayBubble::~SystemTrayBubble() {
   // Reset the host pointer in bubble_view_ in case its destruction is deferred.
@@ -182,17 +179,20 @@ void SystemTrayBubble::UpdateView(
 }
 
 void SystemTrayBubble::InitView(views::View* anchor,
+                                const std::vector<SystemTrayItem*>& items,
+                                SystemTrayView::SystemTrayType system_tray_type,
                                 LoginStatus login_status,
                                 TrayBubbleView::InitParams* init_params) {
   DCHECK(anchor);
   DCHECK(!bubble_view_);
 
-  if (system_tray_view_->system_tray_type() ==
-          SystemTrayView::SYSTEM_TRAY_TYPE_DETAILED &&
+  if (system_tray_type == SystemTrayView::SYSTEM_TRAY_TYPE_DETAILED &&
       init_params->max_height < GetDetailedBubbleMaxHeight()) {
     init_params->max_height = GetDetailedBubbleMaxHeight();
   }
 
+  system_tray_view_ = new SystemTrayView(system_tray_type, items);
+
   init_params->delegate = tray_;
   // Place the bubble on same display as this system tray.
   init_params->parent_window = tray_->GetBubbleWindowContainer();
@@ -210,6 +210,7 @@ void SystemTrayBubble::InitView(views::View* anchor,
 
 void SystemTrayBubble::BubbleViewDestroyed() {
   bubble_view_ = nullptr;
+  system_tray_view_ = nullptr;
 }
 
 void SystemTrayBubble::StartAutoCloseTimer(int seconds) {

ash/system/tray/system_tray_bubble.h

@@ -22,7 +22,7 @@ class SystemTrayView;
 
 class SystemTrayBubble {
  public:
-  SystemTrayBubble(SystemTray* tray, SystemTrayView* view);
+  SystemTrayBubble(SystemTray* tray);
   virtual ~SystemTrayBubble();
 
   // Change the items displayed in the bubble.
@@ -32,6 +32,8 @@ class SystemTrayBubble {
   // Creates |bubble_view_| and a child views for each member of |items_|.
   // Also creates |bubble_wrapper_|. |init_params| may be modified.
   void InitView(views::View* anchor,
+                const std::vector<ash::SystemTrayItem*>& items,
+                SystemTrayView::SystemTrayType system_tray_type,
                 LoginStatus login_status,
                 views::TrayBubbleView::InitParams* init_params);
 
@@ -57,8 +59,12 @@ class SystemTrayBubble {
   void CreateItemViews(LoginStatus login_status);
 
   ash::SystemTray* tray_;
-  SystemTrayView* system_tray_view_;
-  views::TrayBubbleView* bubble_view_;
+
+  // View for system tray content. This is only the direct child of
+  // |bubble_view_|.
+  SystemTrayView* system_tray_view_ = nullptr;
+  // Content view of the bubble.
+  views::TrayBubbleView* bubble_view_ = nullptr;
 
   // Tracks the views created in the last call to CreateItemViews().
   std::map<SystemTrayItem::UmaType, views::View*> tray_item_view_map_;