[signin] Remove dependency of IOSWebViewSigninClient on SigninErrorController

SigninClient implementations should not depend on SigninErrorController.
The dependency should be the other way around.
This CL fixes the WebView implementation of SigninClient.

NOTRY=true

Change-Id: I0592028bd988036bc01a2a48d375337f077bfbd7
Bug: 836212
Reviewed-on: https://chromium-review.googlesource.com/c/1348035
Commit-Queue: David Roger <droger@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611090}

ios/web_view/internal/cwv_web_view_configuration.mm

@@ -20,6 +20,7 @@
 #include "ios/web_view/internal/signin/web_view_account_tracker_service_factory.h"
 #include "ios/web_view/internal/signin/web_view_oauth2_token_service_factory.h"
 #include "ios/web_view/internal/signin/web_view_signin_client_factory.h"
+#include "ios/web_view/internal/signin/web_view_signin_error_controller_factory.h"
 #include "ios/web_view/internal/signin/web_view_signin_manager_factory.h"
 #import "ios/web_view/internal/sync/cwv_sync_controller_internal.h"
 #import "ios/web_view/internal/sync/web_view_profile_sync_service_factory.h"
@@ -166,12 +167,16 @@ CWVWebViewConfiguration* gIncognitoConfiguration = nil;
     ProfileOAuth2TokenService* tokenService =
         ios_web_view::WebViewOAuth2TokenServiceFactory::GetForBrowserState(
             self.browserState);
+    SigninErrorController* signinErrorController =
+        ios_web_view::WebViewSigninErrorControllerFactory::GetForBrowserState(
+            self.browserState);
 
     _syncController = [[CWVSyncController alloc]
         initWithProfileSyncService:profileSyncService
              accountTrackerService:accountTrackerService
                      signinManager:signinManager
-                      tokenService:tokenService];
+                      tokenService:tokenService
+             signinErrorController:signinErrorController];
 
     // Set the newly created CWVSyncController on IOSWebViewSigninClient to
     // so access tokens can be fetched.

ios/web_view/internal/signin/ios_web_view_signin_client.h

@@ -11,7 +11,6 @@
 #include "components/content_settings/core/browser/cookie_settings.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/signin/core/browser/signin_client.h"
-#include "components/signin/core/browser/signin_error_controller.h"
 #include "components/signin/ios/browser/wait_for_network_callback_helper.h"
 #include "net/cookies/cookie_change_dispatcher.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -19,14 +18,12 @@
 @class CWVSyncController;
 
 // iOS WebView specific signin client.
-class IOSWebViewSigninClient : public SigninClient,
-                               public SigninErrorController::Observer {
+class IOSWebViewSigninClient : public SigninClient {
  public:
   IOSWebViewSigninClient(
       PrefService* pref_service,
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       network::mojom::CookieManager* cookie_manager,
-      SigninErrorController* signin_error_controller,
       scoped_refptr<content_settings::CookieSettings> cookie_settings,
       scoped_refptr<HostContentSettingsMap> host_content_settings_map);
 
@@ -59,9 +56,6 @@ class IOSWebViewSigninClient : public SigninClient,
       override;
   void OnSignedOut() override;
 
-  // SigninErrorController::Observer implementation.
-  void OnErrorChanged() override;
-
   // CWVSyncController setter/getter.
   void SetSyncController(CWVSyncController* sync_controller);
   CWVSyncController* GetSyncController() const;
@@ -73,8 +67,6 @@ class IOSWebViewSigninClient : public SigninClient,
   PrefService* pref_service_;
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   network::mojom::CookieManager* cookie_manager_;
-  // Used to check for errors related to signing in.
-  SigninErrorController* signin_error_controller_;
   // Used to check if sign in cookies are allowed.
   scoped_refptr<content_settings::CookieSettings> cookie_settings_;
   // Used to add and remove content settings observers.

ios/web_view/internal/signin/ios_web_view_signin_client.mm

@@ -18,7 +18,6 @@ IOSWebViewSigninClient::IOSWebViewSigninClient(
     PrefService* pref_service,
     scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     network::mojom::CookieManager* cookie_manager,
-    SigninErrorController* signin_error_controller,
     scoped_refptr<content_settings::CookieSettings> cookie_settings,
     scoped_refptr<HostContentSettingsMap> host_content_settings_map)
     : network_callback_helper_(
@@ -26,14 +25,11 @@ IOSWebViewSigninClient::IOSWebViewSigninClient(
       pref_service_(pref_service),
       url_loader_factory_(url_loader_factory),
       cookie_manager_(cookie_manager),
-      signin_error_controller_(signin_error_controller),
       cookie_settings_(cookie_settings),
       host_content_settings_map_(host_content_settings_map) {
-  signin_error_controller_->AddObserver(this);
 }
 
 IOSWebViewSigninClient::~IOSWebViewSigninClient() {
-  signin_error_controller_->RemoveObserver(this);
 }
 
 void IOSWebViewSigninClient::Shutdown() {
@@ -104,10 +100,6 @@ std::unique_ptr<GaiaAuthFetcher> IOSWebViewSigninClient::CreateGaiaAuthFetcher(
                                            url_loader_factory);
 }
 
-void IOSWebViewSigninClient::OnErrorChanged() {
-  [sync_controller_ didUpdateAuthError:signin_error_controller_->auth_error()];
-}
-
 void IOSWebViewSigninClient::SetSyncController(
     CWVSyncController* sync_controller) {
   sync_controller_ = sync_controller;

ios/web_view/internal/signin/web_view_signin_client_factory.mm

@@ -11,7 +11,6 @@
 #include "ios/web_view/internal/content_settings/web_view_cookie_settings_factory.h"
 #include "ios/web_view/internal/content_settings/web_view_host_content_settings_map_factory.h"
 #include "ios/web_view/internal/signin/ios_web_view_signin_client.h"
-#include "ios/web_view/internal/signin/web_view_signin_error_controller_factory.h"
 #include "ios/web_view/internal/web_view_browser_state.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -36,7 +35,6 @@ WebViewSigninClientFactory::WebViewSigninClientFactory()
     : BrowserStateKeyedServiceFactory(
           "SigninClient",
           BrowserStateDependencyManager::GetInstance()) {
-  DependsOn(WebViewSigninErrorControllerFactory::GetInstance());
   DependsOn(WebViewCookieSettingsFactory::GetInstance());
   DependsOn(WebViewHostContentSettingsMapFactory::GetInstance());
 }
@@ -49,7 +47,6 @@ WebViewSigninClientFactory::BuildServiceInstanceFor(
   return std::make_unique<IOSWebViewSigninClient>(
       browser_state->GetPrefs(), browser_state->GetSharedURLLoaderFactory(),
       browser_state->GetCookieManager(),
-      WebViewSigninErrorControllerFactory::GetForBrowserState(browser_state),
       WebViewCookieSettingsFactory::GetForBrowserState(browser_state),
       WebViewHostContentSettingsMapFactory::GetForBrowserState(browser_state));
 }

ios/web_view/internal/sync/cwv_sync_controller.mm

@@ -61,10 +61,13 @@ CWVSyncError CWVConvertGoogleServiceAuthErrorStateToCWVSyncError(
 
 @interface CWVSyncController ()
 
-// Called by WebViewSyncServiceObserverBridge's |OnSyncConfigurationCompleted|.
+// Called by WebViewSyncControllerObserverBridge's
+// |OnSyncConfigurationCompleted|.
 - (void)didCompleteSyncConfiguration;
-// Called by WebViewSyncServiceObserverBridge's |OnSyncShutdown|.
+// Called by WebViewSyncControllerObserverBridge's |OnSyncShutdown|.
 - (void)didShutdownSync;
+// Called by WebViewSyncControllerObserverBridge's |OnErrorChanged|.
+- (void)didUpdateAuthError;
 
 // Call to refresh access tokens for |currentIdentity|.
 - (void)reloadCredentials;
@@ -75,18 +78,15 @@ namespace ios_web_view {
 
 // Bridge that observes browser_sync::ProfileSyncService and calls analagous
 // methods on CWVSyncController.
-class WebViewSyncServiceObserverBridge : public syncer::SyncServiceObserver {
+class WebViewSyncControllerObserverBridge
+    : public syncer::SyncServiceObserver,
+      public SigninErrorController::Observer {
  public:
-  explicit WebViewSyncServiceObserverBridge(CWVSyncController* sync_controller)
+  explicit WebViewSyncControllerObserverBridge(
+      CWVSyncController* sync_controller)
       : sync_controller_(sync_controller) {}
-  void OnStateChanged(syncer::SyncService* sync) override {
-    // No op.
-  }
-
-  void OnSyncCycleCompleted(syncer::SyncService* sync) override {
-    // No op.
-  }
 
+  // syncer::SyncServiceObserver:
   void OnSyncConfigurationCompleted(syncer::SyncService* sync) override {
     [sync_controller_ didCompleteSyncConfiguration];
   }
@@ -95,6 +95,9 @@ class WebViewSyncServiceObserverBridge : public syncer::SyncServiceObserver {
     [sync_controller_ didShutdownSync];
   }
 
+  // SigninErrorController::Observer:
+  void OnErrorChanged() override { [sync_controller_ didUpdateAuthError]; }
+
  private:
   __weak CWVSyncController* sync_controller_;
 };
@@ -105,9 +108,9 @@ class WebViewSyncServiceObserverBridge : public syncer::SyncServiceObserver {
   browser_sync::ProfileSyncService* _profileSyncService;
   AccountTrackerService* _accountTrackerService;
   SigninManager* _signinManager;
-  IOSWebViewSigninClient* _signinClient;
   ProfileOAuth2TokenService* _tokenService;
-  std::unique_ptr<ios_web_view::WebViewSyncServiceObserverBridge> _observer;
+  SigninErrorController* _signinErrorController;
+  std::unique_ptr<ios_web_view::WebViewSyncControllerObserverBridge> _observer;
 
   // Data source that can provide access tokens.
   __weak id<CWVSyncControllerDataSource> _dataSource;
@@ -116,19 +119,24 @@ class WebViewSyncServiceObserverBridge : public syncer::SyncServiceObserver {
 @synthesize delegate = _delegate;
 
 - (instancetype)
-initWithProfileSyncService:(browser_sync::ProfileSyncService*)profileSyncService
-     accountTrackerService:(AccountTrackerService*)accountTrackerService
-             signinManager:(SigninManager*)signinManager
-              tokenService:(ProfileOAuth2TokenService*)tokenService {
+    initWithProfileSyncService:
+        (browser_sync::ProfileSyncService*)profileSyncService
+         accountTrackerService:(AccountTrackerService*)accountTrackerService
+                 signinManager:(SigninManager*)signinManager
+                  tokenService:(ProfileOAuth2TokenService*)tokenService
+         signinErrorController:(SigninErrorController*)signinErrorController {
   self = [super init];
   if (self) {
     _profileSyncService = profileSyncService;
     _accountTrackerService = accountTrackerService;
     _signinManager = signinManager;
     _tokenService = tokenService;
+    _signinErrorController = signinErrorController;
     _observer =
-        std::make_unique<ios_web_view::WebViewSyncServiceObserverBridge>(self);
+        std::make_unique<ios_web_view::WebViewSyncControllerObserverBridge>(
+            self);
     _profileSyncService->AddObserver(_observer.get());
+    _signinErrorController->AddObserver(_observer.get());
 
     // Refresh access tokens on foreground to extend expiration dates.
     [[NSNotificationCenter defaultCenter]
@@ -142,6 +150,7 @@ initWithProfileSyncService:(browser_sync::ProfileSyncService*)profileSyncService
 
 - (void)dealloc {
   _profileSyncService->RemoveObserver(_observer.get());
+  _signinErrorController->RemoveObserver(_observer.get());
 }
 
 #pragma mark - Public Methods
@@ -204,6 +213,7 @@ initWithProfileSyncService:(browser_sync::ProfileSyncService*)profileSyncService
 
 - (void)didShutdownSync {
   _profileSyncService->RemoveObserver(_observer.get());
+  _signinErrorController->RemoveObserver(_observer.get());
 }
 
 - (void)reloadCredentials {
@@ -254,7 +264,8 @@ initWithProfileSyncService:(browser_sync::ProfileSyncService*)profileSyncService
   [_delegate syncController:self didStopSyncWithReason:reason];
 }
 
-- (void)didUpdateAuthError:(const GoogleServiceAuthError&)authError {
+- (void)didUpdateAuthError {
+  GoogleServiceAuthError authError = _signinErrorController->auth_error();
   CWVSyncError code =
       CWVConvertGoogleServiceAuthErrorStateToCWVSyncError(authError.state());
   if (code != CWVSyncErrorNone) {

ios/web_view/internal/sync/cwv_sync_controller_internal.h

@@ -21,15 +21,18 @@ class ProfileSyncService;
 class AccountTrackerService;
 class SigninManager;
 class ProfileOAuth2TokenService;
+class SigninErrorController;
 
 @interface CWVSyncController ()
 
 // All dependencies must out live this class.
 - (instancetype)
-initWithProfileSyncService:(browser_sync::ProfileSyncService*)profileSyncService
-     accountTrackerService:(AccountTrackerService*)accountTrackerService
-             signinManager:(SigninManager*)signinManager
-              tokenService:(ProfileOAuth2TokenService*)tokenService
+    initWithProfileSyncService:
+        (browser_sync::ProfileSyncService*)profileSyncService
+         accountTrackerService:(AccountTrackerService*)accountTrackerService
+                 signinManager:(SigninManager*)signinManager
+                  tokenService:(ProfileOAuth2TokenService*)tokenService
+         signinErrorController:(SigninErrorController*)SigninErrorController
     NS_DESIGNATED_INITIALIZER;
 
 // Called by WebViewProfileOAuth2TokenServiceIOSProviderImpl to obtain
@@ -41,9 +44,6 @@ initWithProfileSyncService:(browser_sync::ProfileSyncService*)profileSyncService
 // Called by IOSWebViewSigninClient when signing out.
 - (void)didSignoutWithSourceMetric:(signin_metrics::ProfileSignout)metric;
 
-// Called by IOSWebViewSigninClient when auth error changes.
-- (void)didUpdateAuthError:(const GoogleServiceAuthError&)authError;
-
 @end
 
 NS_ASSUME_NONNULL_END

ios/web_view/internal/sync/cwv_sync_controller_unittest.mm

@@ -50,17 +50,17 @@ class CWVSyncControllerTest : public PlatformTest {
   CWVSyncControllerTest()
       : browser_state_(/*off_the_record=*/false),
         signin_client_(browser_state_.GetPrefs()),
-        sigin_error_controller_(
+        signin_error_controller_(
             SigninErrorController::AccountMode::ANY_ACCOUNT),
-        token_service_(
-            browser_state_.GetPrefs(),
-            std::make_unique<ProfileOAuth2TokenServiceIOSDelegate>(
-                &signin_client_,
-                std::make_unique<FakeProfileOAuth2TokenServiceIOSProvider>(),
-                &account_tracker_service_,
-                &sigin_error_controller_)),
-        gaia_cookie_manager_service_(&token_service_,
-                                     &signin_client_),
+        token_service_delegate_(new ProfileOAuth2TokenServiceIOSDelegate(
+            &signin_client_,
+            std::make_unique<FakeProfileOAuth2TokenServiceIOSProvider>(),
+            &account_tracker_service_,
+            &signin_error_controller_)),
+        token_service_(browser_state_.GetPrefs(),
+                       std::unique_ptr<ProfileOAuth2TokenServiceIOSDelegate>(
+                           token_service_delegate_)),
+        gaia_cookie_manager_service_(&token_service_, &signin_client_),
         signin_manager_(&signin_client_,
                         &token_service_,
                         &account_tracker_service_,
@@ -87,7 +87,8 @@ class CWVSyncControllerTest : public PlatformTest {
         initWithProfileSyncService:profile_sync_service_.get()
              accountTrackerService:&account_tracker_service_
                      signinManager:&signin_manager_
-                      tokenService:&token_service_];
+                      tokenService:&token_service_
+             signinErrorController:&signin_error_controller_];
   };
 
   ~CWVSyncControllerTest() override {
@@ -109,7 +110,11 @@ class CWVSyncControllerTest : public PlatformTest {
   std::unique_ptr<browser_sync::ProfileSyncServiceMock> profile_sync_service_;
   AccountTrackerService account_tracker_service_;
   TestSigninClient signin_client_;
-  SigninErrorController sigin_error_controller_;
+  SigninErrorController signin_error_controller_;
+
+  // Weak, owned by the token service.
+  ProfileOAuth2TokenServiceIOSDelegate* token_service_delegate_;
+
   FakeProfileOAuth2TokenService token_service_;
   FakeGaiaCookieManagerService gaia_cookie_manager_service_;
   FakeSigninManager signin_manager_;
@@ -164,22 +169,25 @@ TEST_F(CWVSyncControllerTest, DelegateCallbacks) {
             &CWVSyncControllerTest_DelegateCallbacks_Test::OnConfigureDone));
     syncer::DataTypeManager::ConfigureResult result;
     profile_sync_service_->OnConfigureDone(result);
-
     [[delegate expect]
           syncController:sync_controller_
         didFailWithError:[OCMArg checkWithBlock:^BOOL(NSError* error) {
           return error.code == CWVSyncErrorInvalidGAIACredentials;
         }]];
+
+    // Create authentication error.
     GoogleServiceAuthError auth_error(
         GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
-    [sync_controller_ didUpdateAuthError:auth_error];
+    std::string account_id = account_tracker_service_.SeedAccountInfo(
+        "gaia_id", "email@example.com");
+    token_service_delegate_->AddOrUpdateAccount(account_id);
+    token_service_delegate_->UpdateAuthError(account_id, auth_error);
 
     [[delegate expect] syncController:sync_controller_
                 didStopSyncWithReason:CWVStopSyncReasonServer];
     [sync_controller_
         didSignoutWithSourceMetric:signin_metrics::ProfileSignout::
                                        SERVER_FORCED_DISABLE];
-
     [delegate verify];
   }
 }