Remove ash/system references from WebUILoginView

Bug: 854346
Test: the new test case
Change-Id: I5a78d4037ffcf2daf995c2fc04ef44459abbd21c
Reviewed-on: https://chromium-review.googlesource.com/c/1338187
Commit-Queue: Jun Mukai <mukai@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612451}

ash/BUILD.gn

@@ -66,7 +66,6 @@ component("ash") {
     "sticky_keys/sticky_keys_controller.h",
     "system/status_area_widget.h",
     "system/status_area_widget_delegate.h",
-    "system/system_tray_focus_observer.h",
     "system/tray/system_tray_notifier.h",
     "system/unified/unified_system_tray.h",
     "touch/touch_observer_hud.h",

ash/login/login_screen_controller.cc

@@ -21,6 +21,7 @@
 #include "ash/system/status_area_widget_delegate.h"
 #include "ash/system/toast/toast_data.h"
 #include "ash/system/toast/toast_manager.h"
+#include "ash/system/tray/system_tray_notifier.h"
 #include "base/bind.h"
 #include "base/debug/alias.h"
 #include "base/strings/string_number_conversions.h"
@@ -59,9 +60,15 @@ void SetSystemTrayVisibility(SystemTrayVisibility visibility) {
 
 }  // namespace
 
-LoginScreenController::LoginScreenController() : weak_factory_(this) {}
+LoginScreenController::LoginScreenController(
+    SystemTrayNotifier* system_tray_notifier)
+    : system_tray_notifier_(system_tray_notifier), weak_factory_(this) {
+  system_tray_notifier_->AddSystemTrayFocusObserver(this);
+}
 
-LoginScreenController::~LoginScreenController() = default;
+LoginScreenController::~LoginScreenController() {
+  system_tray_notifier_->RemoveSystemTrayFocusObserver(this);
+}
 
 // static
 void LoginScreenController::RegisterProfilePrefs(PrefRegistrySimple* registry,
@@ -559,4 +566,10 @@ void LoginScreenController::OnShow() {
   }
 }
 
+void LoginScreenController::OnFocusLeavingSystemTray(bool reverse) {
+  if (!login_screen_client_)
+    return;
+  login_screen_client_->OnFocusLeavingSystemTray(reverse);
+}
+
 }  // namespace ash

ash/login/login_screen_controller.h

@@ -7,6 +7,7 @@
 
 #include "ash/ash_export.h"
 #include "ash/login/login_screen_controller_observer.h"
+#include "ash/public/cpp/system_tray_focus_observer.h"
 #include "ash/public/interfaces/kiosk_app_info.mojom.h"
 #include "ash/public/interfaces/login_screen.mojom.h"
 #include "base/macros.h"
@@ -19,13 +20,15 @@ class PrefRegistrySimple;
 namespace ash {
 
 class LoginDataDispatcher;
+class SystemTrayNotifier;
 
 // LoginScreenController implements mojom::LoginScreen and wraps the
 // mojom::LoginScreenClient interface. This lets a consumer of ash provide a
 // LoginScreenClient, which we will dispatch to if one has been provided to us.
 // This could send requests to LoginScreenClient and also handle requests from
 // LoginScreenClient through mojo.
-class ASH_EXPORT LoginScreenController : public mojom::LoginScreen {
+class ASH_EXPORT LoginScreenController : public mojom::LoginScreen,
+                                         public SystemTrayFocusObserver {
  public:
   // The current authentication stage. Used to get more verbose logging.
   enum class AuthenticationStage {
@@ -41,7 +44,7 @@ class ASH_EXPORT LoginScreenController : public mojom::LoginScreen {
   using OnAuthenticateCallback =
       base::OnceCallback<void(base::Optional<bool> success)>;
 
-  LoginScreenController();
+  explicit LoginScreenController(SystemTrayNotifier* system_tray_notifier);
   ~LoginScreenController() override;
 
   static void RegisterProfilePrefs(PrefRegistrySimple* registry, bool for_test);
@@ -169,6 +172,9 @@ class ASH_EXPORT LoginScreenController : public mojom::LoginScreen {
   // Common code that is called when the login/lock screen is shown.
   void OnShow();
 
+  // SystemTrayFocusObserver:
+  void OnFocusLeavingSystemTray(bool reverse) override;
+
   // Client interface in chrome browser. May be null in tests.
   mojom::LoginScreenClientPtr login_screen_client_;
 
@@ -177,6 +183,8 @@ class ASH_EXPORT LoginScreenController : public mojom::LoginScreen {
 
   AuthenticationStage authentication_stage_ = AuthenticationStage::kIdle;
 
+  SystemTrayNotifier* system_tray_notifier_;
+
   base::ObserverList<LoginScreenControllerObserver>::Unchecked observers_;
 
   // If set to false, all auth requests will forcibly fail.

ash/login/login_screen_controller_unittest.cc

@@ -12,6 +12,7 @@
 #include "ash/session/test_session_controller_client.h"
 #include "ash/shell.h"
 #include "ash/system/status_area_widget.h"
+#include "ash/system/tray/system_tray_notifier.h"
 #include "ash/system/unified/unified_system_tray.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wallpaper/wallpaper_controller.h"
@@ -243,5 +244,17 @@ TEST_F(LoginScreenControllerTest, ShowLoginScreenRequiresWallpaper) {
   ash::LockScreen::Get()->Destroy();
 }
 
+TEST_F(LoginScreenControllerTest, SystemTrayFocus) {
+  std::unique_ptr<MockLoginScreenClient> client = BindMockLoginScreenClient();
+
+  EXPECT_CALL(*client, OnFocusLeavingSystemTray(true)).Times(1);
+  Shell::Get()->system_tray_notifier()->NotifyFocusOut(true);
+  Shell::Get()->login_screen_controller()->FlushForTesting();
+
+  EXPECT_CALL(*client, OnFocusLeavingSystemTray(false)).Times(1);
+  Shell::Get()->system_tray_notifier()->NotifyFocusOut(false);
+  Shell::Get()->login_screen_controller()->FlushForTesting();
+}
+
 }  // namespace
 }  // namespace ash

ash/login/mock_login_screen_client.h

@@ -93,6 +93,7 @@ class MockLoginScreenClient : public mojom::LoginScreenClient {
   MOCK_METHOD0(ShowResetScreen, void());
   MOCK_METHOD0(ShowAccountAccessHelpApp, void());
   MOCK_METHOD0(FocusOobeDialog, void());
+  MOCK_METHOD1(OnFocusLeavingSystemTray, void(bool reverse));
 
  private:
   bool authenticate_user_callback_result_ = true;

ash/login/ui/lock_contents_view.h

@@ -17,8 +17,8 @@
 #include "ash/login/ui/login_data_dispatcher.h"
 #include "ash/login/ui/login_display_style.h"
 #include "ash/login/ui/non_accessible_view.h"
+#include "ash/public/cpp/system_tray_focus_observer.h"
 #include "ash/session/session_observer.h"
-#include "ash/system/system_tray_focus_observer.h"
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/scoped_observer.h"

ash/public/cpp/BUILD.gn

@@ -112,6 +112,7 @@ component("cpp") {
     "shell_window_ids.h",
     "stylus_utils.cc",
     "stylus_utils.h",
+    "system_tray_focus_observer.h",
     "tablet_mode.cc",
     "tablet_mode.h",
     "wallpaper_types.h",

ash/system/system_tray_focus_observer.h → ash/public/cpp/system_tray_focus_observer.h

@@ -2,24 +2,24 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef ASH_SYSTEM_SYSTEM_TRAY_FOCUS_OBSERVER_H_
-#define ASH_SYSTEM_SYSTEM_TRAY_FOCUS_OBSERVER_H_
+#ifndef ASH_PUBLIC_CPP_SYSTEM_TRAY_FOCUS_OBSERVER_H_
+#define ASH_PUBLIC_CPP_SYSTEM_TRAY_FOCUS_OBSERVER_H_
 
-#include "ash/ash_export.h"
+#include "ash/public/cpp/ash_public_export.h"
 #include "base/macros.h"
 
 namespace ash {
 
 // A class that observes system tray related focus events.
-class ASH_EXPORT SystemTrayFocusObserver {
+class ASH_PUBLIC_EXPORT SystemTrayFocusObserver {
  public:
   // Called when focus is about to leave system tray.
   virtual void OnFocusLeavingSystemTray(bool reverse) = 0;
 
  protected:
-  virtual ~SystemTrayFocusObserver() {}
+  virtual ~SystemTrayFocusObserver() = default;
 };
 
 }  // namespace ash
 
-#endif  // ASH_SYSTEM_SYSTEM_TRAY_FOCUS_OBSERVER_H_
+#endif  // ASH_PUBLIC_CPP_SYSTEM_TRAY_FOCUS_OBSERVER_H_

ash/public/interfaces/login_screen.mojom

@@ -326,4 +326,9 @@ interface LoginScreenClient {
 
   // Show the help app for when users have trouble signing in to their account.
   ShowAccountAccessHelpApp();
+
+  // Called when the keyboard focus is about to leave from the sytem tray in
+  // the login screen / OOBE. |reverse| is true when the focus moves in the
+  // reversed direction.
+  OnFocusLeavingSystemTray(bool reverse);
 };

ash/shell.cc

@@ -666,7 +666,6 @@ Shell::Shell(std::unique_ptr<ShellDelegate> shell_delegate,
           std::make_unique<KeyboardBrightnessController>()),
       locale_notification_controller_(
           std::make_unique<LocaleNotificationController>()),
-      login_screen_controller_(std::make_unique<LoginScreenController>()),
       media_controller_(std::make_unique<MediaController>(connector)),
       new_window_controller_(std::make_unique<NewWindowController>()),
       session_controller_(std::make_unique<SessionController>(connector)),
@@ -685,6 +684,8 @@ Shell::Shell(std::unique_ptr<ShellDelegate> shell_delegate,
   // Ash doesn't properly remove pre-target-handlers.
   ui::EventHandler::DisableCheckTargets();
 
+  login_screen_controller_ =
+      std::make_unique<LoginScreenController>(system_tray_notifier_.get());
   display_manager_.reset(ScreenAsh::CreateDisplayManager());
   window_tree_host_manager_ = std::make_unique<WindowTreeHostManager>();
   user_metrics_recorder_ = std::make_unique<UserMetricsRecorder>();
@@ -816,6 +817,7 @@ Shell::~Shell() {
   // Close all widgets (including the shelf) and destroy all window containers.
   CloseAllRootWindowChildWindows();
 
+  login_screen_controller_.reset();
   system_notification_controller_.reset();
   // Should be destroyed after Shelf and |system_notification_controller_|.
   system_tray_model_.reset();

ash/system/status_area_widget_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "ash/focus_cycler.h"
 #include "ash/public/cpp/ash_switches.h"
+#include "ash/public/cpp/system_tray_focus_observer.h"
 #include "ash/session/session_controller.h"
 #include "ash/session/test_session_controller_client.h"
 #include "ash/shell.h"
@@ -14,7 +15,6 @@
 #include "ash/system/palette/palette_tray.h"
 #include "ash/system/session/logout_button_tray.h"
 #include "ash/system/status_area_widget_test_helper.h"
-#include "ash/system/system_tray_focus_observer.h"
 #include "ash/system/tray/system_tray_notifier.h"
 #include "ash/system/unified/unified_system_tray.h"
 #include "ash/system/virtual_keyboard/virtual_keyboard_tray.h"

ash/system/tray/system_tray_notifier.cc

@@ -4,11 +4,11 @@
 
 #include "ash/system/tray/system_tray_notifier.h"
 
+#include "ash/public/cpp/system_tray_focus_observer.h"
 #include "ash/system/ime/ime_observer.h"
 #include "ash/system/network/network_observer.h"
 #include "ash/system/screen_security/screen_capture_observer.h"
 #include "ash/system/screen_security/screen_share_observer.h"
-#include "ash/system/system_tray_focus_observer.h"
 #include "ash/system/virtual_keyboard/virtual_keyboard_observer.h"
 
 namespace ash {

chrome/browser/chromeos/login/ui/DEPS

@@ -14,9 +14,7 @@ specific_include_rules = {
   # TODO(mash): Fix. https://crbug.com/854346
   "webui_login_view.*": [
      "+ash/accelerators/accelerator_controller.h",
-     "+ash/public/cpp/ash_features.h",
      "+ash/shell.h",
-     "+ash/system",
   ],
   # TODO(jdufault): Deprecate. https://crbug.com/792654
   "login_display_host_webui\.cc": [

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

@@ -9,7 +9,6 @@
 #include "ash/accelerators/accelerator_controller.h"
 #include "ash/public/cpp/ash_features.h"
 #include "ash/shell.h"
-#include "ash/system/tray/system_tray_notifier.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/i18n/rtl.h"
@@ -125,6 +124,8 @@ WebUILoginView::WebUILoginView(const WebViewSettings& settings)
                  content::NotificationService::AllSources());
   registrar_.Add(this, chrome::NOTIFICATION_LOGIN_NETWORK_ERROR_SHOWN,
                  content::NotificationService::AllSources());
+  registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
 
   accel_map_[ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE)] = kAccelNameCancel;
   accel_map_[ui::Accelerator(ui::VKEY_E,
@@ -186,17 +187,19 @@ WebUILoginView::WebUILoginView(const WebViewSettings& settings)
     }
   }
 
-  if (!features::IsMultiProcessMash())
-    ash::Shell::Get()->system_tray_notifier()->AddSystemTrayFocusObserver(this);
+  if (LoginScreenClient::HasInstance()) {
+    LoginScreenClient::Get()->AddSystemTrayFocusObserver(this);
+    observing_system_tray_focus_ = true;
+  }
 }
 
 WebUILoginView::~WebUILoginView() {
   for (auto& observer : observer_list_)
     observer.OnHostDestroying();
 
+  if (observing_system_tray_focus_)
+    LoginScreenClient::Get()->RemoveSystemTrayFocusObserver(this);
   if (!features::IsMultiProcessMash()) {
-    ash::Shell::Get()->system_tray_notifier()->RemoveSystemTrayFocusObserver(
-        this);
     ash::Shell::Get()->accelerator_controller()->UnregisterAll(this);
     ChromeKeyboardControllerClient::Get()->RemoveObserver(this);
   }
@@ -399,7 +402,20 @@ void WebUILoginView::Observe(int type,
     case chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE:
     case chrome::NOTIFICATION_LOGIN_NETWORK_ERROR_SHOWN: {
       OnLoginPromptVisible();
-      registrar_.RemoveAll();
+      registrar_.Remove(this, chrome::NOTIFICATION_LOGIN_OR_LOCK_WEBUI_VISIBLE,
+                        content::NotificationService::AllSources());
+      registrar_.Remove(this, chrome::NOTIFICATION_LOGIN_NETWORK_ERROR_SHOWN,
+                        content::NotificationService::AllSources());
+      break;
+    }
+    case chrome::NOTIFICATION_APP_TERMINATING: {
+      // In some tests, WebUILoginView remains after LoginScreenClient gets
+      // deleted on shutdown. It should unregister itself before the deletion
+      // happens.
+      if (observing_system_tray_focus_) {
+        LoginScreenClient::Get()->RemoveSystemTrayFocusObserver(this);
+        observing_system_tray_focus_ = false;
+      }
       break;
     }
     default:
@@ -565,6 +581,10 @@ bool WebUILoginView::MoveFocusToSystemTray(bool reverse) {
 }
 
 void WebUILoginView::OnLoginPromptVisible() {
+  if (!observing_system_tray_focus_ && LoginScreenClient::HasInstance()) {
+    LoginScreenClient::Get()->AddSystemTrayFocusObserver(this);
+    observing_system_tray_focus_ = true;
+  }
   // If we're hidden than will generate this signal once we're shown.
   if (is_hidden_ || webui_visible_) {
     VLOG(1) << "Login WebUI >> not emitting signal, hidden: " << is_hidden_;

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

@@ -8,7 +8,7 @@
 #include <map>
 #include <string>
 
-#include "ash/system/system_tray_focus_observer.h"
+#include "ash/public/cpp/system_tray_focus_observer.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/observer_list.h"
@@ -222,6 +222,8 @@ class WebUILoginView : public views::View,
   // FocusCyclerDelegate.
   bool delegates_lock_screen_app_focus_cycle_ = false;
 
+  bool observing_system_tray_focus_ = false;
+
   base::ObserverList<web_modal::ModalDialogHostObserver>::Unchecked
       observer_list_;
 

chrome/browser/ui/ash/login_screen_client.cc

@@ -63,6 +63,16 @@ void LoginScreenClient::SetDelegate(Delegate* delegate) {
   delegate_ = delegate;
 }
 
+void LoginScreenClient::AddSystemTrayFocusObserver(
+    ash::SystemTrayFocusObserver* observer) {
+  system_tray_focus_observers_.AddObserver(observer);
+}
+
+void LoginScreenClient::RemoveSystemTrayFocusObserver(
+    ash::SystemTrayFocusObserver* observer) {
+  system_tray_focus_observers_.RemoveObserver(observer);
+}
+
 ash::mojom::LoginScreenPtr& LoginScreenClient::login_screen() {
   return login_screen_;
 }
@@ -211,6 +221,11 @@ void LoginScreenClient::ShowAccountAccessHelpApp() {
       ->ShowHelpTopic(chromeos::HelpAppLauncher::HELP_CANT_ACCESS_ACCOUNT);
 }
 
+void LoginScreenClient::OnFocusLeavingSystemTray(bool reverse) {
+  for (ash::SystemTrayFocusObserver& observer : system_tray_focus_observers_)
+    observer.OnFocusLeavingSystemTray(reverse);
+}
+
 void LoginScreenClient::LoadWallpaper(const AccountId& account_id) {
   WallpaperControllerClient::Get()->ShowUserWallpaper(account_id);
 }

chrome/browser/ui/ash/login_screen_client.h

@@ -5,8 +5,10 @@
 #ifndef CHROME_BROWSER_UI_ASH_LOGIN_SCREEN_CLIENT_H_
 #define CHROME_BROWSER_UI_ASH_LOGIN_SCREEN_CLIENT_H_
 
+#include "ash/public/cpp/system_tray_focus_observer.h"
 #include "ash/public/interfaces/login_screen.mojom.h"
 #include "base/macros.h"
+#include "base/observer_list.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "ui/base/ime/chromeos/input_method_manager.h"
 
@@ -72,6 +74,9 @@ class LoginScreenClient : public ash::mojom::LoginScreenClient {
 
   chromeos::LoginAuthRecorder* auth_recorder();
 
+  void AddSystemTrayFocusObserver(ash::SystemTrayFocusObserver* observer);
+  void RemoveSystemTrayFocusObserver(ash::SystemTrayFocusObserver* observer);
+
   // ash::mojom::LoginScreenClient:
   void AuthenticateUserWithPasswordOrPin(
       const AccountId& account_id,
@@ -109,6 +114,7 @@ class LoginScreenClient : public ash::mojom::LoginScreenClient {
   void LaunchArcKioskApp(const AccountId& account_id) override;
   void ShowResetScreen() override;
   void ShowAccountAccessHelpApp() override;
+  void OnFocusLeavingSystemTray(bool reverse) override;
 
  private:
   void SetPublicSessionKeyboardLayout(
@@ -126,6 +132,9 @@ class LoginScreenClient : public ash::mojom::LoginScreenClient {
   // Captures authentication related user metrics for login screen.
   std::unique_ptr<chromeos::LoginAuthRecorder> auth_recorder_;
 
+  base::ObserverList<ash::SystemTrayFocusObserver>::Unchecked
+      system_tray_focus_observers_;
+
   base::WeakPtrFactory<LoginScreenClient> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(LoginScreenClient);