Reland cros: Unify cash and mash ChromeLauncherController management.

The first CL was reverted for msan failures:
https://chromium-review.googlesource.com/c/chromium/src/+/670684
This CL refines destruction timing for shelf item delegates.
(destroy with ChromeLauncherController and ShelfController)

ORIGINAL DESCRIPTION:

Refine ChromeLauncherController lifetime management.
(always use the new ChromeLauncherControllerInitializer)

Remove ShellDelegate::ShelfInit and ShelfShutdown.
Remove ShelfInitializer (tests/shell init via default prefs).
Cleanup (std::make_unique, TestShellDelegate usage, etc)
Reorder display update test expectations (unclear what changed...)

Bug: 557406, 681072
Change-Id: I64eb5be117ed8d78e4fb685e51f18fd40ec358cd
Reviewed-on: https://chromium-review.googlesource.com/667838
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#502376}
Reviewed-on: https://chromium-review.googlesource.com/671488
Cr-Commit-Position: refs/heads/master@{#502671}

ash/display/display_manager_unittest.cc

@@ -158,11 +158,11 @@ TEST_F(DisplayManagerTest, UpdateDisplayTest) {
             display_manager()->GetDisplayAt(0).bounds().ToString());
 
   EXPECT_EQ("2 1 0 1 1", GetCountSummary());
-  EXPECT_EQ(display_manager()->GetDisplayAt(1).id(), changed()[0].id());
-  EXPECT_EQ(display_manager()->GetDisplayAt(0).id(), changed()[1].id());
+  EXPECT_EQ(display_manager()->GetDisplayAt(0).id(), changed()[0].id());
+  EXPECT_EQ(display_manager()->GetDisplayAt(1).id(), changed()[1].id());
   EXPECT_EQ(display_manager()->GetDisplayAt(1).id(), added()[0].id());
-  EXPECT_EQ("500,0 400x400", changed()[0].bounds().ToString());
-  EXPECT_EQ("0,0 500x500", changed()[1].bounds().ToString());
+  EXPECT_EQ("0,0 500x500", changed()[0].bounds().ToString());
+  EXPECT_EQ("500,0 400x400", changed()[1].bounds().ToString());
   // Secondary display is on right.
   EXPECT_EQ("500,0 400x400", added()[0].bounds().ToString());
   EXPECT_EQ("0,501 400x400",
@@ -297,14 +297,14 @@ TEST_F(DisplayManagerTest, UpdateThreeDisplaysWithDefaultLayout) {
             display_manager()->GetDisplayAt(2).bounds().ToString());
 
   EXPECT_EQ("3 2 0 1 1", GetCountSummary());
-  EXPECT_EQ(display_manager()->GetDisplayAt(1).id(), changed()[0].id());
-  EXPECT_EQ(display_manager()->GetDisplayAt(2).id(), changed()[1].id());
-  EXPECT_EQ(display_manager()->GetDisplayAt(0).id(), changed()[2].id());
+  EXPECT_EQ(display_manager()->GetDisplayAt(0).id(), changed()[0].id());
+  EXPECT_EQ(display_manager()->GetDisplayAt(1).id(), changed()[1].id());
+  EXPECT_EQ(display_manager()->GetDisplayAt(2).id(), changed()[2].id());
   EXPECT_EQ(display_manager()->GetDisplayAt(1).id(), added()[0].id());
   EXPECT_EQ(display_manager()->GetDisplayAt(2).id(), added()[1].id());
-  EXPECT_EQ("640,0 320x200", changed()[0].bounds().ToString());
-  EXPECT_EQ("960,0 400x300", changed()[1].bounds().ToString());
-  EXPECT_EQ("0,0 640x480", changed()[2].bounds().ToString());
+  EXPECT_EQ("0,0 640x480", changed()[0].bounds().ToString());
+  EXPECT_EQ("640,0 320x200", changed()[1].bounds().ToString());
+  EXPECT_EQ("960,0 400x300", changed()[2].bounds().ToString());
   // Secondary and terniary displays are on right.
   EXPECT_EQ("640,0 320x200", added()[0].bounds().ToString());
   EXPECT_EQ("1000,0 320x200",

ash/mus/shell_delegate_mus.cc

@@ -75,14 +75,6 @@ void ShellDelegateMus::OpenUrlFromArc(const GURL& url) {
   NOTIMPLEMENTED();
 }
 
-void ShellDelegateMus::ShelfInit() {
-  NOTIMPLEMENTED();
-}
-
-void ShellDelegateMus::ShelfShutdown() {
-  NOTIMPLEMENTED();
-}
-
 NetworkingConfigDelegate* ShellDelegateMus::GetNetworkingConfigDelegate() {
   // TODO(mash): Provide a real implementation, perhaps by folding its behavior
   // into an ash-side network information cache. http://crbug.com/651157

ash/mus/shell_delegate_mus.h

@@ -33,8 +33,6 @@ class ShellDelegateMus : public ShellDelegate {
   void Exit() override;
   std::unique_ptr<keyboard::KeyboardUI> CreateKeyboardUI() override;
   void OpenUrlFromArc(const GURL& url) override;
-  void ShelfInit() override;
-  void ShelfShutdown() override;
   NetworkingConfigDelegate* GetNetworkingConfigDelegate() override;
   std::unique_ptr<WallpaperDelegate> CreateWallpaperDelegate() override;
   AccessibilityDelegate* CreateAccessibilityDelegate() override;

ash/shelf/shelf_controller.cc

@@ -106,6 +106,7 @@ ShelfController::~ShelfController() {
     Shell::Get()->tablet_mode_controller()->RemoveObserver(this);
   Shell::Get()->session_controller()->RemoveObserver(this);
   model_.RemoveObserver(this);
+  model_.DestroyItemDelegates();
 }
 
 // static

ash/shell.cc

@@ -782,14 +782,6 @@ Shell::~Shell() {
   // ShelfWindowWatcher has window observers and a pointer to the shelf model.
   shelf_window_watcher_.reset();
 
-  // ShelfItemDelegate subclasses it owns have complex cleanup to run (e.g. ARC
-  // shelf items in Chrome) so explicitly shutdown early.
-  shelf_model()->DestroyItemDelegates();
-
-  // Notify the ShellDelegate that the shelf is shutting down.
-  // TODO(msw): Refine ChromeLauncherController lifetime management.
-  shell_delegate_->ShelfShutdown();
-
   // Removes itself as an observer of |pref_service_|.
   shelf_controller_.reset();
 
@@ -1275,10 +1267,6 @@ void Shell::InitializeShelf() {
   DCHECK(session_controller());
   DCHECK_GT(session_controller()->NumberOfLoggedInUsers(), 0);
 
-  // Notify the ShellDelegate that the shelf is being initialized.
-  // TODO(msw): Refine ChromeLauncherController lifetime management.
-  shell_delegate_->ShelfInit();
-
   if (!shelf_window_watcher_)
     shelf_window_watcher_ = base::MakeUnique<ShelfWindowWatcher>(shelf_model());
 

ash/shell/shell_delegate_impl.cc

@@ -101,14 +101,6 @@ std::unique_ptr<keyboard::KeyboardUI> ShellDelegateImpl::CreateKeyboardUI() {
 
 void ShellDelegateImpl::OpenUrlFromArc(const GURL& url) {}
 
-void ShellDelegateImpl::ShelfInit() {
-  Shelf* shelf = Shell::GetPrimaryRootWindowController()->shelf();
-  shelf->SetAlignment(SHELF_ALIGNMENT_BOTTOM);
-  shelf->SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_NEVER);
-}
-
-void ShellDelegateImpl::ShelfShutdown() {}
-
 NetworkingConfigDelegate* ShellDelegateImpl::GetNetworkingConfigDelegate() {
   return nullptr;
 }

ash/shell/shell_delegate_impl.h

@@ -35,8 +35,6 @@ class ShellDelegateImpl : public ShellDelegate {
   void Exit() override;
   std::unique_ptr<keyboard::KeyboardUI> CreateKeyboardUI() override;
   void OpenUrlFromArc(const GURL& url) override;
-  void ShelfInit() override;
-  void ShelfShutdown() override;
   NetworkingConfigDelegate* GetNetworkingConfigDelegate() override;
   std::unique_ptr<WallpaperDelegate> CreateWallpaperDelegate() override;
   AccessibilityDelegate* CreateAccessibilityDelegate() override;

ash/shell_delegate.h

@@ -87,11 +87,6 @@ class ASH_EXPORT ShellDelegate {
   // Opens the |url| in a new browser tab.
   virtual void OpenUrlFromArc(const GURL& url) = 0;
 
-  // Functions called when the shelf is initialized and shut down.
-  // TODO(msw): Refine ChromeLauncherController lifetime management.
-  virtual void ShelfInit() = 0;
-  virtual void ShelfShutdown() = 0;
-
   // Returns the delegate. May be null in tests.
   virtual NetworkingConfigDelegate* GetNetworkingConfigDelegate() = 0;
 

ash/test_shell_delegate.cc

@@ -4,51 +4,17 @@
 
 #include "ash/test_shell_delegate.h"
 
-#include <limits>
-
 #include "ash/gpu_support_stub.h"
 #include "ash/keyboard/test_keyboard_ui.h"
 #include "ash/palette_delegate.h"
-#include "ash/public/cpp/shell_window_ids.h"
-#include "ash/root_window_controller.h"
-#include "ash/shelf/shelf.h"
-#include "ash/shell.h"
-#include "ash/shell_observer.h"
 #include "ash/system/tray/system_tray_notifier.h"
 #include "ash/test/test_accessibility_delegate.h"
 #include "ash/wallpaper/test_wallpaper_delegate.h"
-#include "ash/wm/window_state.h"
-#include "ash/wm/window_util.h"
 #include "base/logging.h"
-#include "base/memory/ptr_util.h"
-#include "ui/aura/window.h"
 #include "ui/gfx/image/image.h"
 
 namespace ash {
 
-// A ShellObserver that sets the shelf alignment and auto hide behavior when the
-// shelf is created, to simulate ChromeLauncherController's behavior.
-class ShelfInitializer : public ShellObserver {
- public:
-  ShelfInitializer() { Shell::Get()->AddShellObserver(this); }
-  ~ShelfInitializer() override { Shell::Get()->RemoveShellObserver(this); }
-
-  // ShellObserver:
-  void OnShelfCreatedForRootWindow(aura::Window* root_window) override {
-    Shelf* shelf = RootWindowController::ForWindow(root_window)->shelf();
-    // Do not override the custom initialization performed by some unit tests.
-    if (shelf->alignment() == SHELF_ALIGNMENT_BOTTOM_LOCKED &&
-        shelf->auto_hide_behavior() == SHELF_AUTO_HIDE_ALWAYS_HIDDEN) {
-      shelf->SetAlignment(SHELF_ALIGNMENT_BOTTOM);
-      shelf->SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_NEVER);
-      shelf->UpdateVisibilityState();
-    }
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ShelfInitializer);
-};
-
 TestShellDelegate::TestShellDelegate() = default;
 
 TestShellDelegate::~TestShellDelegate() = default;
@@ -86,26 +52,18 @@ void TestShellDelegate::Exit() {
 }
 
 std::unique_ptr<keyboard::KeyboardUI> TestShellDelegate::CreateKeyboardUI() {
-  return base::MakeUnique<TestKeyboardUI>();
+  return std::make_unique<TestKeyboardUI>();
 }
 
 void TestShellDelegate::OpenUrlFromArc(const GURL& url) {}
 
-void TestShellDelegate::ShelfInit() {
-  // Create a separate shelf initializer that mimics ChromeLauncherController.
-  if (!shelf_initializer_)
-    shelf_initializer_ = base::MakeUnique<ShelfInitializer>();
-}
-
-void TestShellDelegate::ShelfShutdown() {}
-
 NetworkingConfigDelegate* TestShellDelegate::GetNetworkingConfigDelegate() {
   return nullptr;
 }
 
 std::unique_ptr<WallpaperDelegate>
 TestShellDelegate::CreateWallpaperDelegate() {
-  return base::MakeUnique<TestWallpaperDelegate>();
+  return std::make_unique<TestWallpaperDelegate>();
 }
 
 AccessibilityDelegate* TestShellDelegate::CreateAccessibilityDelegate() {

ash/test_shell_delegate.h

@@ -6,19 +6,12 @@
 #define ASH_TEST_SHELL_DELEGATE_H_
 
 #include <memory>
-#include <string>
 
 #include "ash/shell_delegate.h"
 #include "base/macros.h"
 
-namespace keyboard {
-class KeyboardUI;
-}
-
 namespace ash {
 
-class ShelfInitializer;
-
 class TestShellDelegate : public ShellDelegate {
  public:
   TestShellDelegate();
@@ -39,8 +32,6 @@ class TestShellDelegate : public ShellDelegate {
   void PreShutdown() override;
   void Exit() override;
   std::unique_ptr<keyboard::KeyboardUI> CreateKeyboardUI() override;
-  void ShelfInit() override;
-  void ShelfShutdown() override;
   void OpenUrlFromArc(const GURL& url) override;
   NetworkingConfigDelegate* GetNetworkingConfigDelegate() override;
   std::unique_ptr<WallpaperDelegate> CreateWallpaperDelegate() override;
@@ -61,7 +52,6 @@ class TestShellDelegate : public ShellDelegate {
   int num_exit_requests_ = 0;
   bool multi_profiles_enabled_ = false;
   bool force_maximize_on_first_run_ = false;
-  std::unique_ptr<ShelfInitializer> shelf_initializer_;
 
   DISALLOW_COPY_AND_ASSIGN(TestShellDelegate);
 };

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -255,7 +255,6 @@ class ChromeLauncherControllerInitializer
     : public session_manager::SessionManagerObserver {
  public:
   ChromeLauncherControllerInitializer() {
-    DCHECK(ash_util::IsRunningInMash());
     session_manager::SessionManager::Get()->AddObserver(this);
   }
 
@@ -272,16 +271,19 @@ class ChromeLauncherControllerInitializer
     if (session_manager::SessionManager::Get()->session_state() ==
         session_manager::SessionState::ACTIVE) {
       chrome_shelf_model_ = std::make_unique<ash::ShelfModel>();
-      chrome_launcher_controller_ = std::make_unique<ChromeLauncherController>(
-          nullptr, chrome_shelf_model_.get());
+      ash::ShelfModel* model = ash_util::IsRunningInMash()
+                                   ? chrome_shelf_model_.get()
+                                   : ash::Shell::Get()->shelf_model();
+      chrome_launcher_controller_ =
+          std::make_unique<ChromeLauncherController>(nullptr, model);
       chrome_launcher_controller_->Init();
       session_manager::SessionManager::Get()->RemoveObserver(this);
     }
   }
 
  private:
-  // These are only used in Mash; corresponding instances are owned by Ash's
-  // ShelfController and ChromeShellDelegate in classic Ash.
+  // This shelf model is synced with Ash's ShelfController instance in Mash.
+  // ChromeLauncherController uses Ash's ShelfModel instance directly in Cash.
   std::unique_ptr<ash::ShelfModel> chrome_shelf_model_;
   std::unique_ptr<ChromeLauncherController> chrome_launcher_controller_;
 
@@ -701,10 +703,8 @@ void ChromeBrowserMainPartsChromeos::PreProfileInit() {
   g_browser_process->platform_part()->InitializeChromeUserManager();
   g_browser_process->platform_part()->InitializeSessionManager();
 
-  if (ash_util::IsRunningInMash()) {
-    chrome_launcher_controller_initializer_ =
-        std::make_unique<internal::ChromeLauncherControllerInitializer>();
-  }
+  chrome_launcher_controller_initializer_ =
+      std::make_unique<internal::ChromeLauncherControllerInitializer>();
 
   ScreenLocker::InitClass();
 

chrome/browser/ui/ash/chrome_shell_delegate.cc

@@ -455,18 +455,6 @@ void ChromeShellDelegate::OpenUrlFromArc(const GURL& url) {
       displayer.browser()->window()->GetNativeWindow());
 }
 
-void ChromeShellDelegate::ShelfInit() {
-  if (!launcher_controller_) {
-    launcher_controller_ = base::MakeUnique<ChromeLauncherController>(
-        nullptr, ash::Shell::Get()->shelf_model());
-    launcher_controller_->Init();
-  }
-}
-
-void ChromeShellDelegate::ShelfShutdown() {
-  launcher_controller_.reset();
-}
-
 ash::GPUSupport* ChromeShellDelegate::CreateGPUSupport() {
   // Chrome uses real GPU support.
   return new ash::GPUSupportImpl;
@@ -543,8 +531,10 @@ void ChromeShellDelegate::Observe(int type,
       // Do not use chrome::NOTIFICATION_PROFILE_ADDED because the
       // profile is not fully initialized by user_manager.  Use
       // chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED instead.
-      if (launcher_controller_)
-        launcher_controller_->OnUserProfileReadyToSwitch(profile);
+      if (ChromeLauncherController::instance()) {
+        ChromeLauncherController::instance()->OnUserProfileReadyToSwitch(
+            profile);
+      }
       break;
     }
     case chrome::NOTIFICATION_SESSION_STARTED:

chrome/browser/ui/ash/chrome_shell_delegate.h

@@ -22,8 +22,6 @@ namespace keyboard {
 class KeyboardUI;
 }
 
-class ChromeLauncherController;
-
 class ChromeShellDelegate : public ash::ShellDelegate,
                             public content::NotificationObserver {
  public:
@@ -42,8 +40,6 @@ class ChromeShellDelegate : public ash::ShellDelegate,
   void Exit() override;
   std::unique_ptr<keyboard::KeyboardUI> CreateKeyboardUI() override;
   void OpenUrlFromArc(const GURL& url) override;
-  void ShelfInit() override;
-  void ShelfShutdown() override;
   ash::NetworkingConfigDelegate* GetNetworkingConfigDelegate() override;
   std::unique_ptr<ash::WallpaperDelegate> CreateWallpaperDelegate() override;
   ash::AccessibilityDelegate* CreateAccessibilityDelegate() override;
@@ -64,8 +60,6 @@ class ChromeShellDelegate : public ash::ShellDelegate,
 
   content::NotificationRegistrar registrar_;
 
-  std::unique_ptr<ChromeLauncherController> launcher_controller_;
-
   std::unique_ptr<chromeos::DisplayConfigurationObserver>
       display_configuration_observer_;
 

chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc

@@ -256,6 +256,9 @@ ChromeLauncherController::~ChromeLauncherController() {
   // Reset the app window controllers here since it has a weak pointer to this.
   app_window_controllers_.clear();
 
+  // Destroy local shelf item delegates; some subclasses have complex cleanup.
+  model_->DestroyItemDelegates();
+
   model_->RemoveObserver(this);
 
   // Release all profile dependent resources.