cros: Fix invisible shelf when external display attached at lock screen

When an external display is attached a new RootWindowController and new
shelf are created for that display. Shelf alignment is loaded from prefs.
However, if the external display is marked as primary then the internal
and external displays are swapped. This means that the (new) shelf now
on the internal display never reloads its alignment from prefs. Since
the screen was locked at attach time, the shelf stays in the
bottom_locked state and never becomes visible.

Fix this by reloaded shelf alignment from prefs on display swap.

Bug: 748291
Test: added to ash_unittests
Change-Id: I5780dd3ca3b796b6f46fea9bc46fe875d123701e
Reviewed-on: https://chromium-review.googlesource.com/597410
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491452}

ash/shelf/shelf.cc

@@ -120,6 +120,8 @@ void Shelf::CreateShelfWidget(aura::Window* root) {
   aura::Window* status_container =
       root->GetChildById(kShellWindowId_StatusContainer);
   shelf_widget_->CreateStatusAreaWidget(status_container);
+
+  Shell::Get()->window_tree_host_manager()->AddObserver(this);
 }
 
 void Shelf::ShutdownShelfWidget() {
@@ -128,7 +130,11 @@ void Shelf::ShutdownShelfWidget() {
 }
 
 void Shelf::DestroyShelfWidget() {
-  shelf_widget_.reset();
+  // May be called multiple times during shutdown.
+  if (shelf_widget_) {
+    shelf_widget_.reset();
+    Shell::Get()->window_tree_host_manager()->RemoveObserver(this);
+  }
 }
 
 void Shelf::NotifyShelfInitialized() {
@@ -365,4 +371,19 @@ void Shelf::OnBackgroundUpdated(ShelfBackgroundType background_type,
     observer.OnBackgroundTypeChanged(background_type, change_type);
 }
 
+void Shelf::OnWindowTreeHostReusedForDisplay(
+    AshWindowTreeHost* window_tree_host,
+    const display::Display& display) {
+  // See comment in OnWindowTreeHostsSwappedDisplays().
+  NotifyShelfInitialized();
+}
+
+void Shelf::OnWindowTreeHostsSwappedDisplays(AshWindowTreeHost* host1,
+                                             AshWindowTreeHost* host2) {
+  // The display id for this shelf instance may have changed, so request
+  // re-initialization to fetch the alignment and auto-hide state from prefs.
+  // See http://crbug.com/748291
+  NotifyShelfInitialized();
+}
+
 }  // namespace ash

ash/shelf/shelf.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "ash/ash_export.h"
+#include "ash/display/window_tree_host_manager.h"
 #include "ash/public/cpp/shelf_types.h"
 #include "ash/shelf/shelf_layout_manager_observer.h"
 #include "ash/shelf/shelf_locking_manager.h"
@@ -40,7 +41,8 @@ class ShelfObserver;
 // Controller for the shelf state. One per display, because each display might
 // have different shelf alignment, autohide, etc. Exists for the lifetime of the
 // root window controller.
-class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
+class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver,
+                         public WindowTreeHostManager::Observer {
  public:
   Shelf();
   ~Shelf() override;
@@ -146,6 +148,13 @@ class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
   void OnBackgroundUpdated(ShelfBackgroundType background_type,
                            AnimationChangeType change_type) override;
 
+  // WindowTreeHostManager::Observer:
+  void OnWindowTreeHostReusedForDisplay(
+      AshWindowTreeHost* window_tree_host,
+      const display::Display& display) override;
+  void OnWindowTreeHostsSwappedDisplays(AshWindowTreeHost* host1,
+                                        AshWindowTreeHost* host2) override;
+
  private:
   class AutoHideEventHandler;
   friend class ShelfLayoutManagerTest;

ash/shelf/shelf_unittest.cc

@@ -5,15 +5,55 @@
 #include <utility>
 
 #include "ash/public/cpp/shelf_model.h"
+#include "ash/root_window_controller.h"
+#include "ash/session/test_session_controller_client.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shelf/shelf_button.h"
+#include "ash/shelf/shelf_controller.h"
 #include "ash/shelf/shelf_view.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 "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
+#include "mojo/public/cpp/bindings/associated_binding.h"
 
 namespace ash {
+namespace {
+
+Shelf* GetShelfForDisplay(int64_t display_id) {
+  return Shell::GetRootWindowControllerWithDisplayId(display_id)->shelf();
+}
+
+// Tracks shelf initialization display ids.
+class ShelfInitializationObserver : public mojom::ShelfObserver {
+ public:
+  ShelfInitializationObserver() = default;
+  ~ShelfInitializationObserver() override = default;
+
+  // mojom::ShelfObserver:
+  void OnShelfInitialized(int64_t display_id) override {
+    shelf_initialized_display_ids_.push_back(display_id);
+  }
+  void OnAlignmentChanged(ShelfAlignment alignment,
+                          int64_t display_id) override {}
+  void OnAutoHideBehaviorChanged(ShelfAutoHideBehavior auto_hide,
+                                 int64_t display_id) override {}
+  void OnShelfItemAdded(int32_t, const ShelfItem&) override {}
+  void OnShelfItemRemoved(const ShelfID&) override {}
+  void OnShelfItemMoved(const ShelfID&, int32_t) override {}
+  void OnShelfItemUpdated(const ShelfItem&) override {}
+  void OnShelfItemDelegateChanged(const ShelfID&,
+                                  mojom::ShelfItemDelegatePtr) override {}
+
+  std::vector<int64_t> shelf_initialized_display_ids_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ShelfInitializationObserver);
+};
+
+}  // namespace
 
 class ShelfTest : public AshTestBase {
  public:
@@ -110,4 +150,65 @@ TEST_F(ShelfTest, ShowOverflowBubble) {
   EXPECT_FALSE(shelf_widget->IsShowingOverflowBubble());
 }
 
+// Verifies that shelves are re-initialized after display swap, which will
+// reload their alignment prefs. http://crbug.com/748291
+TEST_F(ShelfTest, ShelfInitializedOnDisplaySwap) {
+  ShelfController* controller = Shell::Get()->shelf_controller();
+  ShelfInitializationObserver observer;
+  mojom::ShelfObserverAssociatedPtr observer_ptr;
+  mojo::AssociatedBinding<mojom::ShelfObserver> binding(
+      &observer, mojo::MakeIsolatedRequest(&observer_ptr));
+  controller->AddObserver(observer_ptr.PassInterface());
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(0u, observer.shelf_initialized_display_ids_.size());
+
+  // Simulate adding an external display at the lock screen. The shelf on the
+  // new display is initialized.
+  GetSessionControllerClient()->RequestLockScreen();
+  UpdateDisplay("1024x768,800x600");
+  base::RunLoop().RunUntilIdle();
+  const int64_t internal_display_id = GetPrimaryDisplay().id();
+  const int64_t external_display_id = GetSecondaryDisplay().id();
+  ASSERT_EQ(1u, observer.shelf_initialized_display_ids_.size());
+  EXPECT_EQ(external_display_id, observer.shelf_initialized_display_ids_[0]);
+  observer.shelf_initialized_display_ids_.clear();
+
+  // Simulate the external display becoming the primary display. Each shelf is
+  // re-initialized.
+  SwapPrimaryDisplay();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_EQ(2u, observer.shelf_initialized_display_ids_.size());
+  EXPECT_TRUE(base::ContainsValue(observer.shelf_initialized_display_ids_,
+                                  internal_display_id));
+  EXPECT_TRUE(base::ContainsValue(observer.shelf_initialized_display_ids_,
+                                  external_display_id));
+
+  // Simulate shelf state being set from prefs, which is how Chrome responds to
+  // the initialization request.
+  controller->SetAlignment(SHELF_ALIGNMENT_LEFT, internal_display_id);
+  controller->SetAlignment(SHELF_ALIGNMENT_RIGHT, external_display_id);
+  controller->SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS,
+                                  internal_display_id);
+  controller->SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS,
+                                  external_display_id);
+
+  // Shelf is still locked to bottom because screen is locked.
+  EXPECT_EQ(SHELF_ALIGNMENT_BOTTOM_LOCKED,
+            GetShelfForDisplay(internal_display_id)->alignment());
+  EXPECT_EQ(SHELF_ALIGNMENT_BOTTOM_LOCKED,
+            GetShelfForDisplay(external_display_id)->alignment());
+
+  // After screen unlock all shelves should have an alignment.
+  GetSessionControllerClient()->UnlockScreen();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(SHELF_ALIGNMENT_LEFT,
+            GetShelfForDisplay(internal_display_id)->alignment());
+  EXPECT_EQ(SHELF_ALIGNMENT_RIGHT,
+            GetShelfForDisplay(external_display_id)->alignment());
+  EXPECT_EQ(SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS,
+            GetShelfForDisplay(internal_display_id)->auto_hide_behavior());
+  EXPECT_EQ(SHELF_AUTO_HIDE_BEHAVIOR_ALWAYS,
+            GetShelfForDisplay(external_display_id)->auto_hide_behavior());
+}
+
 }  // namespace ash

ash/shelf/shelf_view_unittest.cc

@@ -79,11 +79,6 @@ int64_t GetPrimaryDisplayId() {
   return display::Screen::GetScreen()->GetPrimaryDisplay().id();
 }
 
-}  // namespace
-
-////////////////////////////////////////////////////////////////////////////////
-// ShelfObserver::OnShelfIconPositionsChanged tests.
-
 class TestShelfObserver : public ShelfObserver {
  public:
   explicit TestShelfObserver(Shelf* shelf) : shelf_(shelf) {
@@ -118,6 +113,11 @@ class TestShelfObserver : public ShelfObserver {
   DISALLOW_COPY_AND_ASSIGN(TestShelfObserver);
 };
 
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// ShelfObserver::OnShelfIconPositionsChanged tests.
+
 class ShelfObserverIconTest : public AshTestBase {
  public:
   ShelfObserverIconTest() {}