Remove NOTIFICATION_BROWSER_OPENED from LoginDisplayHostCommon.

Use BrowserListObserver::OnBrowserAdded instead.

Bug: 268984
Change-Id: Ib006156034fb7b3b52d8c0ecad7f0189fe91e08e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715097Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680506}

chrome/browser/chromeos/login/ui/login_display_host_common.cc

@@ -17,10 +17,10 @@
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/system/device_disabling_manager.h"
 #include "chrome/browser/ui/ash/wallpaper_controller_client.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/webui/chromeos/internet_detail_dialog.h"
 #include "chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h"
 #include "components/keep_alive_registry/keep_alive_types.h"
-#include "components/keep_alive_registry/scoped_keep_alive.h"
 #include "ui/base/ui_base_features.h"
 
 namespace chromeos {
@@ -43,19 +43,14 @@ void ScheduleCompletionCallbacks(std::vector<base::OnceClosure>&& callbacks) {
 
 }  // namespace
 
-LoginDisplayHostCommon::LoginDisplayHostCommon() : weak_factory_(this) {
-  keep_alive_.reset(
-      new ScopedKeepAlive(KeepAliveOrigin::LOGIN_DISPLAY_HOST_WEBUI,
-                          KeepAliveRestartOption::DISABLED));
-
-  // Close the login screen on NOTIFICATION_APP_TERMINATING.
+LoginDisplayHostCommon::LoginDisplayHostCommon()
+    : keep_alive_(KeepAliveOrigin::LOGIN_DISPLAY_HOST_WEBUI,
+                  KeepAliveRestartOption::DISABLED) {
+  // Close the login screen on NOTIFICATION_APP_TERMINATING (for the case where
+  // shutdown occurs before login completes).
   registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
                  content::NotificationService::AllSources());
-  // NOTIFICATION_BROWSER_OPENED is issued after browser is created, but
-  // not shown yet. Lock window has to be closed at this point so that
-  // a browser window exists and the window can acquire input focus.
-  registrar_.Add(this, chrome::NOTIFICATION_BROWSER_OPENED,
-                 content::NotificationService::AllSources());
+  BrowserList::AddObserver(this);
 }
 
 LoginDisplayHostCommon::~LoginDisplayHostCommon() {
@@ -235,21 +230,25 @@ void LoginDisplayHostCommon::ResyncUserData() {
     GetExistingUserController()->ResyncUserData();
 }
 
+void LoginDisplayHostCommon::OnBrowserAdded(Browser* browser) {
+  // Browsers created before session start (windows opened by extensions, for
+  // example) are ignored.
+  if (session_starting_) {
+    // OnBrowserAdded is called when the browser is created, but not shown yet.
+    // Lock window has to be closed at this point so that a browser window
+    // exists and the window can acquire input focus.
+    OnBrowserCreated();
+    registrar_.RemoveAll();
+    BrowserList::RemoveObserver(this);
+  }
+}
+
 void LoginDisplayHostCommon::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  if (type == chrome::NOTIFICATION_APP_TERMINATING) {
+  if (type == chrome::NOTIFICATION_APP_TERMINATING)
     ShutdownDisplayHost();
-  } else if (type == chrome::NOTIFICATION_BROWSER_OPENED && session_starting_) {
-    // Browsers created before session start (windows opened by extensions, for
-    // example) are ignored.
-    OnBrowserCreated();
-    registrar_.Remove(this, chrome::NOTIFICATION_APP_TERMINATING,
-                      content::NotificationService::AllSources());
-    registrar_.Remove(this, chrome::NOTIFICATION_BROWSER_OPENED,
-                      content::NotificationService::AllSources());
-  }
 }
 
 void LoginDisplayHostCommon::OnCancelPasswordChangedFlow() {}
@@ -265,6 +264,7 @@ void LoginDisplayHostCommon::ShutdownDisplayHost() {
   ProfileHelper::Get()->ClearSigninProfile(base::DoNothing());
   shutting_down_ = true;
   registrar_.RemoveAll();
+  BrowserList::RemoveObserver(this);
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
 }
 

chrome/browser/chromeos/login/ui/login_display_host_common.h

@@ -11,11 +11,12 @@
 
 #include "chrome/browser/chromeos/login/ui/kiosk_app_menu_controller.h"
 #include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/ui/browser_list_observer.h"
+#include "components/keep_alive_registry/scoped_keep_alive.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
 class AccountId;
-class ScopedKeepAlive;
 
 namespace chromeos {
 
@@ -26,6 +27,7 @@ class DemoAppLauncher;
 // implementation - the goal is to reduce code duplication between
 // LoginDisplayHostMojo and LoginDisplayHostWebUI.
 class LoginDisplayHostCommon : public LoginDisplayHost,
+                               public BrowserListObserver,
                                public content::NotificationObserver {
  public:
   LoginDisplayHostCommon();
@@ -55,6 +57,9 @@ class LoginDisplayHostCommon : public LoginDisplayHost,
   void MigrateUserData(const std::string& old_password) final;
   void ResyncUserData() final;
 
+  // BrowserListObserver:
+  void OnBrowserAdded(Browser* browser) override;
+
   // content::NotificationObserver:
   void Observe(int type,
                const content::NotificationSource& source,
@@ -108,14 +113,14 @@ class LoginDisplayHostCommon : public LoginDisplayHost,
   bool is_finalizing_ = false;
 
   // Make sure chrome won't exit while we are at login/oobe screen.
-  std::unique_ptr<ScopedKeepAlive> keep_alive_;
+  ScopedKeepAlive keep_alive_;
 
   // Called after host deletion.
   std::vector<base::OnceClosure> completion_callbacks_;
 
   KioskAppMenuController kiosk_app_menu_controller_;
 
-  base::WeakPtrFactory<LoginDisplayHostCommon> weak_factory_;
+  base::WeakPtrFactory<LoginDisplayHostCommon> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(LoginDisplayHostCommon);
 };

chrome/browser/chromeos/login/webview_login_browsertest.cc

@@ -40,6 +40,7 @@
 #include "chrome/browser/ui/login/login_handler.h"
 #include "chrome/browser/ui/webui/chromeos/login/eula_screen_handler.h"
 #include "chrome/browser/ui/webui/signin/signin_utils.h"
+#include "chrome/test/base/ui_test_utils.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/dbus/session_manager/fake_session_manager_client.h"
 #include "components/content_settings/core/common/pref_names.h"
@@ -253,6 +254,9 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, Basic) {
   WaitForGaiaPageBackButtonUpdate();
   ExpectPasswordPage();
 
+  ASSERT_TRUE(LoginDisplayHost::default_host());
+  EXPECT_TRUE(LoginDisplayHost::default_host()->GetWebUILoginView());
+
   content::WindowedNotificationObserver session_start_waiter(
       chrome::NOTIFICATION_SESSION_STARTED,
       content::NotificationService::AllSources());
@@ -261,7 +265,16 @@ IN_PROC_BROWSER_TEST_F(WebviewLoginTest, Basic) {
   SigninFrameJS().TypeIntoPath(FakeGaiaMixin::kFakeUserPassword, {"password"});
   SigninFrameJS().TapOn("nextButton");
 
+  // The login view should be destroyed after the browser window opens.
+  ui_test_utils::WaitForBrowserToOpen();
+  EXPECT_FALSE(LoginDisplayHost::default_host()->GetWebUILoginView());
+
   session_start_waiter.Wait();
+
+  // Wait for the LoginDisplayHost to delete itself, which is a posted task.
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(LoginDisplayHost::default_host());
 }
 
 IN_PROC_BROWSER_TEST_F(WebviewLoginTest, BackButton) {