[Lacros] Change AccountManagerFacadeLacros creation flow

Improves testability of AccountManagerFacadeLacros - mojo::Remote is now
passed explicitly to the ctor.

Bug: 1117472
Change-Id: I81475ea7bf369a51ff0b0bdf8e43117737d09706
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2550809
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830998}

chrome/browser/lacros/account_manager_facade_factory_lacros.cc

@@ -6,11 +6,25 @@
 
 #include "base/no_destructor.h"
 #include "chrome/browser/lacros/account_manager_facade_lacros.h"
+#include "chromeos/lacros/lacros_chrome_service_impl.h"
 #include "components/account_manager_core/account_manager_facade.h"
+#include "mojo/public/cpp/bindings/remote.h"
 
 account_manager::AccountManagerFacade* GetAccountManagerFacade(
     const std::string& profile_path) {
   // Multi-Login is disabled with Lacros. Always return the same instance.
-  static base::NoDestructor<AccountManagerFacadeLacros> facade;
+  static base::NoDestructor<AccountManagerFacadeLacros> facade([] {
+    auto* lacros_chrome_service_impl = chromeos::LacrosChromeServiceImpl::Get();
+    DCHECK(lacros_chrome_service_impl);
+    if (!lacros_chrome_service_impl->IsAccountManagerAvailable()) {
+      LOG(WARNING) << "Connected to an older version of ash. Account "
+                      "consistency will not be available";
+      return mojo::Remote<crosapi::mojom::AccountManager>();
+    }
+    mojo::Remote<crosapi::mojom::AccountManager> remote;
+    lacros_chrome_service_impl->BindAccountManagerReceiver(
+        remote.BindNewPipeAndPassReceiver());
+    return remote;
+  }());
   return facade.get();
 }

chrome/browser/lacros/account_manager_facade_lacros.cc

@@ -8,7 +8,6 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "chromeos/lacros/lacros_chrome_service_impl.h"
 
 namespace {
 
@@ -18,14 +17,14 @@ constexpr uint32_t kMinVersionWithObserver = 1;
 
 }  // namespace
 
-AccountManagerFacadeLacros::AccountManagerFacadeLacros()
-    : lacros_chrome_service_impl_(chromeos::LacrosChromeServiceImpl::Get()) {
-  if (!lacros_chrome_service_impl_->IsAccountManagerAvailable())
+AccountManagerFacadeLacros::AccountManagerFacadeLacros(
+    mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote)
+    : account_manager_remote_(std::move(account_manager_remote)) {
+  if (!account_manager_remote_)
     return;
 
-  lacros_chrome_service_impl_->account_manager_remote().QueryVersion(
-      base::BindOnce(&AccountManagerFacadeLacros::OnVersionCheck,
-                     weak_factory_.GetWeakPtr()));
+  account_manager_remote_.QueryVersion(base::BindOnce(
+      &AccountManagerFacadeLacros::OnVersionCheck, weak_factory_.GetWeakPtr()));
 }
 
 AccountManagerFacadeLacros::~AccountManagerFacadeLacros() = default;
@@ -38,7 +37,7 @@ void AccountManagerFacadeLacros::OnVersionCheck(uint32_t version) {
   if (version < kMinVersionWithObserver)
     return;
 
-  lacros_chrome_service_impl_->account_manager_remote()->AddObserver(
+  account_manager_remote_->AddObserver(
       base::BindOnce(&AccountManagerFacadeLacros::OnReceiverReceived,
                      weak_factory_.GetWeakPtr()));
 }
@@ -50,9 +49,8 @@ void AccountManagerFacadeLacros::OnReceiverReceived(
           this, std::move(receiver));
   // At this point (|receiver_| exists), we are subscribed to Account Manager.
 
-  lacros_chrome_service_impl_->account_manager_remote()->IsInitialized(
-      base::BindOnce(&AccountManagerFacadeLacros::OnInitialized,
-                     weak_factory_.GetWeakPtr()));
+  account_manager_remote_->IsInitialized(base::BindOnce(
+      &AccountManagerFacadeLacros::OnInitialized, weak_factory_.GetWeakPtr()));
 }
 void AccountManagerFacadeLacros::OnInitialized(bool is_initialized) {
   if (is_initialized)

chrome/browser/lacros/account_manager_facade_lacros.h

@@ -11,10 +11,7 @@
 #include "chromeos/crosapi/mojom/account_manager.mojom.h"
 #include "components/account_manager_core/account_manager_facade.h"
 #include "mojo/public/cpp/bindings/receiver.h"
-
-namespace chromeos {
-class LacrosChromeServiceImpl;
-}  // namespace chromeos
+#include "mojo/public/cpp/bindings/remote.h"
 
 // Lacros specific implementation of |AccountManagerFacade| that talks to
 // |chromeos::AccountManager|, residing in ash-chrome, over Mojo.
@@ -22,7 +19,8 @@ class AccountManagerFacadeLacros
     : public account_manager::AccountManagerFacade,
       public crosapi::mojom::AccountManagerObserver {
  public:
-  AccountManagerFacadeLacros();
+  explicit AccountManagerFacadeLacros(
+      mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote);
   AccountManagerFacadeLacros(const AccountManagerFacadeLacros&) = delete;
   AccountManagerFacadeLacros& operator=(const AccountManagerFacadeLacros&) =
       delete;
@@ -42,7 +40,7 @@ class AccountManagerFacadeLacros
   void OnInitialized(bool is_initialized);
 
   bool is_initialized_ = false;
-  chromeos::LacrosChromeServiceImpl* const lacros_chrome_service_impl_;
+  mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote_;
   std::unique_ptr<mojo::Receiver<crosapi::mojom::AccountManagerObserver>>
       receiver_;
 

chromeos/lacros/lacros_chrome_service_impl.cc

@@ -363,20 +363,6 @@ void LacrosChromeServiceImpl::BindReceiver(
                        ToMojo(delegate_->GetChromeVersion())));
   }
 
-  if (IsAccountManagerAvailable()) {
-    mojo::PendingReceiver<crosapi::mojom::AccountManager>
-        account_manager_receiver =
-            account_manager_remote_.BindNewPipeAndPassReceiver();
-    never_blocking_sequence_->PostTask(
-        FROM_HERE,
-        base::BindOnce(
-            &LacrosChromeServiceNeverBlockingState::BindAccountManagerReceiver,
-            weak_sequenced_state_, std::move(account_manager_receiver)));
-  } else {
-    LOG(WARNING) << "Connected to an older version of ash. Account consistency "
-                    "will not be available";
-  }
-
   if (IsFileManagerAvailable()) {
     mojo::PendingReceiver<crosapi::mojom::FileManager> pending_receiver =
         file_manager_remote_.BindNewPipeAndPassReceiver();
@@ -434,6 +420,16 @@ bool LacrosChromeServiceImpl::IsAccountManagerAvailable() {
              AshChromeService::MethodMinVersions::kBindAccountManagerMinVersion;
 }
 
+void LacrosChromeServiceImpl::BindAccountManagerReceiver(
+    mojo::PendingReceiver<crosapi::mojom::AccountManager> pending_receiver) {
+  DCHECK(IsAccountManagerAvailable());
+  never_blocking_sequence_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &LacrosChromeServiceNeverBlockingState::BindAccountManagerReceiver,
+          weak_sequenced_state_, std::move(pending_receiver)));
+}
+
 bool LacrosChromeServiceImpl::IsFileManagerAvailable() {
   base::Optional<uint32_t> version = AshChromeServiceVersion();
   return version &&

chromeos/lacros/lacros_chrome_service_impl.h

@@ -162,17 +162,6 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
       mojo::PendingReceiver<media_session::mojom::MediaControllerManager>
           remote);
 
-  // account_manager_remote() can only be used if this method returns true.
-  bool IsAccountManagerAvailable();
-
-  // This must be called on the affine sequence. It exposes a remote that can
-  // be used to interact with accounts in Chrome OS Account Manager.
-  mojo::Remote<crosapi::mojom::AccountManager>& account_manager_remote() {
-    DCHECK_CALLED_ON_VALID_SEQUENCE(affine_sequence_checker_);
-    DCHECK(IsAccountManagerAvailable());
-    return account_manager_remote_;
-  }
-
   // file_manager_remote() can only be used if this method returns true.
   bool IsFileManagerAvailable();
 
@@ -197,6 +186,13 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   void BindScreenManagerReceiver(
       mojo::PendingReceiver<crosapi::mojom::ScreenManager> pending_receiver);
 
+  // BindAccountManagerReceiver() can only be used if this method returns true.
+  bool IsAccountManagerAvailable();
+
+  // This may be called on any thread.
+  void BindAccountManagerReceiver(
+      mojo::PendingReceiver<crosapi::mojom::AccountManager> pending_receiver);
+
   // OnLacrosStartup method of AshChromeService crosapi can only be called
   // if this method returns true.
   bool IsOnLacrosStartupAvailable();
@@ -251,7 +247,6 @@ class COMPONENT_EXPORT(CHROMEOS_LACROS) LacrosChromeServiceImpl {
   mojo::Remote<device::mojom::HidManager> hid_manager_remote_;
   mojo::Remote<crosapi::mojom::Feedback> feedback_remote_;
   mojo::Remote<crosapi::mojom::KeystoreService> keystore_service_remote_;
-  mojo::Remote<crosapi::mojom::AccountManager> account_manager_remote_;
   mojo::Remote<crosapi::mojom::FileManager> file_manager_remote_;
 
   // This member is instantiated on the affine sequence alongside the