Fix the issue that no shelf icons show on the external display

Scrollable shelf updates its layout when a shelf icon is added to
shelf view as a child. When an external display is connected, shelf
view of the external display should add shelf icons during shelf view
initialization. However, ShelfView::Init is called before shelf view
becomes the grand child of ScrollableShelfView. As a result,
scrollable shelf cannot perceive the change in view hierarchy.

This CL makes the initialization of shelf view be the last step of
ScrollableShelfView::Init. It also fixes the side effect brought by
reordering of child views' initialization.

Bug: 1035596
Change-Id: Ice08606d34b5af725157602c0038d995df2d36ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1984725
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728252}

ash/shelf/hotseat_widget.cc

@@ -104,15 +104,11 @@ class HotseatWidget::DelegateView : public views::WidgetDelegateView,
   // Updates the hotseat background.
   void UpdateOpaqueBackground();
 
+  void SetOpaqueBackground(const gfx::Rect& opaque_background_bounds);
+
   // Updates the hotseat background when tablet mode changes.
   void OnTabletModeChanged();
 
-  // Hides |opaque_background_| immediately or with animation.
-  void HideOpaqueBackground(bool animate);
-
-  // Shows |opaque_background_| immediately.
-  void ShowOpaqueBackground();
-
   // views::WidgetDelegateView:
   bool CanActivate() const override;
   void ReorderChildLayers(ui::Layer* parent_layer) override;
@@ -125,9 +121,6 @@ class HotseatWidget::DelegateView : public views::WidgetDelegateView,
   }
 
  private:
-  // Returns whether the hotseat background should be shown.
-  bool ShouldShowHotseatBackground() const;
-
   void SetParentLayer(ui::Layer* layer);
 
   FocusCycler* focus_cycler_ = nullptr;
@@ -163,13 +156,20 @@ void HotseatWidget::DelegateView::Init(
 }
 
 void HotseatWidget::DelegateView::UpdateOpaqueBackground() {
-  if (!ShouldShowHotseatBackground()) {
+  if (!HotseatWidget::ShouldShowHotseatBackground()) {
     opaque_background_.SetVisible(false);
     if (features::IsBackgroundBlurEnabled())
       opaque_background_.SetBackgroundBlur(0);
     return;
   }
 
+  SetOpaqueBackground(scrollable_shelf_view_->GetHotseatBackgroundBounds());
+}
+
+void HotseatWidget::DelegateView::SetOpaqueBackground(
+    const gfx::Rect& background_bounds) {
+  DCHECK(HotseatWidget::ShouldShowHotseatBackground());
+
   opaque_background_.SetVisible(true);
   opaque_background_.SetColor(ShelfConfig::Get()->GetDefaultShelfColor());
 
@@ -178,8 +178,6 @@ void HotseatWidget::DelegateView::UpdateOpaqueBackground() {
   if (opaque_background_.rounded_corner_radii() != rounded_corners)
     opaque_background_.SetRoundedCornerRadius(rounded_corners);
 
-  gfx::Rect background_bounds =
-      scrollable_shelf_view_->GetHotseatBackgroundBounds();
   if (opaque_background_.bounds() != background_bounds)
     opaque_background_.SetBounds(background_bounds);
 
@@ -211,12 +209,6 @@ void HotseatWidget::DelegateView::OnWallpaperColorsChanged() {
   UpdateOpaqueBackground();
 }
 
-bool HotseatWidget::DelegateView::ShouldShowHotseatBackground() const {
-  return chromeos::switches::ShouldShowShelfHotseat() &&
-         Shell::Get()->tablet_mode_controller() &&
-         Shell::Get()->tablet_mode_controller()->InTabletMode();
-}
-
 void HotseatWidget::DelegateView::SetParentLayer(ui::Layer* layer) {
   layer->Add(&opaque_background_);
   ReorderLayers();
@@ -231,6 +223,12 @@ HotseatWidget::~HotseatWidget() {
   ShelfConfig::Get()->RemoveObserver(this);
 }
 
+bool HotseatWidget::ShouldShowHotseatBackground() {
+  return chromeos::switches::ShouldShowShelfHotseat() &&
+         Shell::Get()->tablet_mode_controller() &&
+         Shell::Get()->tablet_mode_controller()->InTabletMode();
+}
+
 void HotseatWidget::Initialize(aura::Window* container, Shelf* shelf) {
   DCHECK(container);
   DCHECK(shelf);
@@ -337,8 +335,9 @@ float HotseatWidget::CalculateOpacity() {
                                               : target_opacity;
 }
 
-void HotseatWidget::UpdateOpaqueBackground() {
-  delegate_view_->UpdateOpaqueBackground();
+void HotseatWidget::SetOpaqueBackground(
+    const gfx::Rect& opaque_background_bounds) {
+  delegate_view_->SetOpaqueBackground(opaque_background_bounds);
 }
 
 void HotseatWidget::UpdateLayout(bool animate) {

ash/shelf/hotseat_widget.h

@@ -27,6 +27,9 @@ class ASH_EXPORT HotseatWidget : public views::Widget,
   HotseatWidget();
   ~HotseatWidget() override;
 
+  // Returns whether the hotseat background should be shown.
+  static bool ShouldShowHotseatBackground();
+
   // Initializes the widget, sets its contents view and basic properties.
   void Initialize(aura::Window* container, Shelf* shelf);
 
@@ -57,8 +60,9 @@ class ASH_EXPORT HotseatWidget : public views::Widget,
   // Returns the target opacity (between 0 and 1) given current conditions.
   float CalculateOpacity();
 
-  // Updates the opaque background which functions as the hotseat background.
-  void UpdateOpaqueBackground();
+  // Sets the bounds of the opaque background which functions as the hotseat
+  // background.
+  void SetOpaqueBackground(const gfx::Rect& background_bounds);
 
   // Updates this widget's layout according to current conditions.
   void UpdateLayout(bool animate);

ash/shelf/scrollable_shelf_view.cc

@@ -443,8 +443,6 @@ ScrollableShelfView::~ScrollableShelfView() {
 }
 
 void ScrollableShelfView::Init() {
-  shelf_view_->Init();
-
   // Although there is no animation for ScrollableShelfView, a layer is still
   // needed. Otherwise, the child view without its own layer will be painted on
   // RootView and RootView is beneath |opaque_background_| in ShelfWidget. As a
@@ -479,6 +477,10 @@ void ScrollableShelfView::Init() {
   GetShelf()->tooltip()->set_shelf_tooltip_delegate(this);
 
   set_context_menu_controller(this);
+
+  // Initializes |shelf_view_| after scrollable shelf view's children are
+  // initialized.
+  shelf_view_->Init();
 }
 
 void ScrollableShelfView::OnFocusRingActivationChanged(bool activated) {
@@ -1677,7 +1679,10 @@ void ScrollableShelfView::UpdateAvailableSpace() {
 
   // The hotseat uses |available_space_| to determine where to show its
   // background, so notify it when it is recalculated.
-  GetShelf()->shelf_widget()->hotseat_widget()->UpdateOpaqueBackground();
+  if (HotseatWidget::ShouldShowHotseatBackground()) {
+    GetShelf()->shelf_widget()->hotseat_widget()->SetOpaqueBackground(
+        GetHotseatBackgroundBounds());
+  }
 
   // Paddings are within the shelf view. It makes sure that |shelf_view_|'s
   // bounds are not changed by adding/removing the shelf icon under the same

ash/shelf/scrollable_shelf_view_unittest.cc

@@ -5,10 +5,12 @@
 #include "ash/shelf/scrollable_shelf_view.h"
 
 #include "ash/public/cpp/shelf_config.h"
+#include "ash/root_window_controller.h"
 #include "ash/shelf/shelf_test_util.h"
 #include "ash/shelf/shelf_tooltip_manager.h"
 #include "ash/shelf/shelf_view_test_api.h"
 #include "ash/shelf/shelf_widget.h"
+#include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/test/scoped_feature_list.h"
@@ -444,6 +446,31 @@ class HotseatScrollableShelfViewTest : public ScrollableShelfViewTest {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+// Verifies that after adding the second display, shelf icons showing on
+// the primary display are also visible on the second display
+// (https://crbug.com/1035596).
+TEST_F(HotseatScrollableShelfViewTest, CheckTappableIndicesOnSecondDisplay) {
+  constexpr int icon_number = 5;
+  for (int i = 0; i < icon_number; i++)
+    AddAppShortcut();
+
+  // Adds the second display.
+  UpdateDisplay("600x800,600x800");
+
+  Shelf* secondary_shelf =
+      Shell::GetRootWindowControllerWithDisplayId(GetSecondaryDisplay().id())
+          ->shelf();
+  ScrollableShelfView* secondary_scrollable_shelf_view =
+      secondary_shelf->shelf_widget()
+          ->hotseat_widget()
+          ->scrollable_shelf_view();
+
+  // Verifies that the all icons are visible on the secondary display.
+  EXPECT_EQ(icon_number - 1,
+            secondary_scrollable_shelf_view->last_tappable_app_index());
+  EXPECT_EQ(0, secondary_scrollable_shelf_view->first_tappable_app_index());
+}
+
 // Verifies that the scrollable shelf in oveflow mode has the correct layout
 // after switching to tablet mode (https://crbug.com/1017979).
 TEST_F(HotseatScrollableShelfViewTest, CorrectUIAfterSwitchingToTablet) {