Use the _settingsNavigationController to present the Google services settings.

This CL updates MainController to use a regular _settingsNavigationController
when presenting the Google services settings. This ensures that when the
user attempts to open an URL from the Google services settings, the regular
close settings and open URL command is executed.

Bug: 963436

Change-Id: Ibec138d6cd3baf34c43f713f43c1e9cb908a8382
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1621132
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662190}

ios/chrome/app/main_controller.mm

@@ -124,7 +124,6 @@
 #include "ios/chrome/browser/ui/history/history_coordinator.h"
 #import "ios/chrome/browser/ui/main/browser_view_wrangler.h"
 #import "ios/chrome/browser/ui/promos/signin_promo_view_controller.h"
-#import "ios/chrome/browser/ui/settings/google_services/google_services_navigation_coordinator.h"
 #import "ios/chrome/browser/ui/settings/settings_navigation_controller.h"
 #import "ios/chrome/browser/ui/signin_interaction/signin_interaction_coordinator.h"
 #include "ios/chrome/browser/ui/tab_grid/tab_grid_coordinator.h"
@@ -311,7 +310,6 @@ enum class EnterTabSwitcherSnapshotResult {
 
 @interface MainController () <AppURLLoadingServiceDelegate,
                               BrowserStateStorageSwitching,
-                              GoogleServicesNavigationCoordinatorDelegate,
                               PrefObserverDelegate,
                               SettingsNavigationControllerDelegate,
                               TabSwitcherDelegate,
@@ -432,10 +430,6 @@ enum class EnterTabSwitcherSnapshotResult {
 @property(nonatomic, strong)
     SigninInteractionCoordinator* signinInteractionCoordinator;
 
-// Coordinator to display the Google services settings.
-@property(nonatomic, strong)
-    GoogleServicesNavigationCoordinator* googleServicesNavigationCoordinator;
-
 // Activates |mainBVC| and |otrBVC| and sets |currentBVC| as primary iff
 // |currentBVC| can be made active.
 - (void)activateBVCAndMakeCurrentBVCPrimary;
@@ -1657,9 +1651,10 @@ enum class EnterTabSwitcherSnapshotResult {
                                  completion:nil];
 }
 
-// TODO(crbug.com/779791) : Remove show settings from MainController.
+// TODO(crbug.com/779791) : Remove Google services settings from MainController.
 - (void)showGoogleServicesSettingsFromViewController:
     (UIViewController*)baseViewController {
+  DCHECK(unified_consent::IsUnifiedConsentFeatureEnabled());
   if (!baseViewController) {
     DCHECK_EQ(self.currentBVC, self.mainCoordinator.activeViewController);
     baseViewController = self.currentBVC;
@@ -1676,16 +1671,13 @@ enum class EnterTabSwitcherSnapshotResult {
         showGoogleServicesSettingsFromViewController:baseViewController];
     return;
   }
-  // If the settings dialog is not already opened, start a new
-  // GoogleServicesNavigationCoordinator.
-  self.googleServicesNavigationCoordinator =
-      [[GoogleServicesNavigationCoordinator alloc]
-          initWithBaseViewController:baseViewController
-                        browserState:_mainBrowserState];
-  self.googleServicesNavigationCoordinator.delegate = self;
-  self.googleServicesNavigationCoordinator.dispatcherForSettings =
-      self.dispatcherForSettings;
-  [self.googleServicesNavigationCoordinator start];
+
+  _settingsNavigationController = [SettingsNavigationController
+      newGoogleServicesController:self.currentBrowserState
+                         delegate:self];
+  [baseViewController presentViewController:_settingsNavigationController
+                                   animated:YES
+                                 completion:nil];
 }
 
 // TODO(crbug.com/779791) : Remove show settings commands from MainController.
@@ -2411,10 +2403,6 @@ enum class EnterTabSwitcherSnapshotResult {
   // First, cancel the signin interaction.
   [self.signinInteractionCoordinator cancel];
 
-  [self.googleServicesNavigationCoordinator dismissAnimated:NO];
-  // Need to stop and set |self.googleServicesNavigationCoordinator| to nil.
-  [self stopGoogleServicesNavigationCoordinator];
-
   // Then, depending on what the SSO view controller is presented on, dismiss
   // it.
   ProceduralBlock completionWithBVC = ^{
@@ -2614,24 +2602,6 @@ enum class EnterTabSwitcherSnapshotResult {
   return username.empty() ? nil : base::SysUTF8ToNSString(username);
 }
 
-#pragma mark - GoogleServicesNavigationCoordinatorDelegate
-
-- (void)googleServicesNavigationCoordinatorDidClose:
-    (GoogleServicesNavigationCoordinator*)coordinator {
-  DCHECK_EQ(self.googleServicesNavigationCoordinator, coordinator);
-  // Need to stop and set |self.googleServicesNavigationCoordinator| to nil.
-  DCHECK(self.googleServicesNavigationCoordinator);
-  [self stopGoogleServicesNavigationCoordinator];
-}
-
-#pragma mark - GoogleServicesNavigationCoordinator helpers
-
-- (void)stopGoogleServicesNavigationCoordinator {
-  self.googleServicesNavigationCoordinator.delegate = nil;
-  [self.googleServicesNavigationCoordinator stop];
-  self.googleServicesNavigationCoordinator = nil;
-}
-
 @end
 
 #pragma mark - TestingOnly

ios/chrome/browser/ui/settings/google_services/BUILD.gn

@@ -11,8 +11,6 @@ source_set("google_services") {
     "advanced_signin_settings_coordinator.mm",
     "advanced_signin_settings_navigation_controller.h",
     "advanced_signin_settings_navigation_controller.mm",
-    "google_services_navigation_coordinator.h",
-    "google_services_navigation_coordinator.mm",
     "google_services_settings_command_handler.h",
     "google_services_settings_consumer.h",
     "google_services_settings_coordinator.h",

ios/chrome/browser/ui/settings/google_services/google_services_navigation_coordinator.h

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef IOS_CHROME_BROWSER_UI_SETTINGS_GOOGLE_SERVICES_GOOGLE_SERVICES_NAVIGATION_COORDINATOR_H_
-#define IOS_CHROME_BROWSER_UI_SETTINGS_GOOGLE_SERVICES_GOOGLE_SERVICES_NAVIGATION_COORDINATOR_H_
-
-#import "ios/chrome/browser/ui/coordinators/chrome_coordinator.h"
-
-@protocol ApplicationCommands;
-@protocol BrowserCommands;
-@class GoogleServicesNavigationCoordinator;
-
-// GoogleServicesNavigationCoordinator delegate.
-@protocol GoogleServicesNavigationCoordinatorDelegate <NSObject>
-
-// Called when the user closed GoogleServicesNavigationCoordinator.
-- (void)googleServicesNavigationCoordinatorDidClose:
-    (GoogleServicesNavigationCoordinator*)coordinator;
-
-@end
-
-// Shows the Google services settings in a navigation controller.
-@interface GoogleServicesNavigationCoordinator : ChromeCoordinator
-
-// Delegate.
-@property(nonatomic, weak) id<GoogleServicesNavigationCoordinatorDelegate>
-    delegate;
-// Global dispatcher.
-@property(nonatomic, weak) id<ApplicationCommands, BrowserCommands>
-    dispatcherForSettings;
-
-// Dismisses the Google services navigation view controller.
-- (void)dismissAnimated:(BOOL)animated;
-
-@end
-
-#endif  // IOS_CHROME_BROWSER_UI_SETTINGS_GOOGLE_SERVICES_GOOGLE_SERVICES_NAVIGATION_COORDINATOR_H_

ios/chrome/browser/ui/settings/google_services/google_services_navigation_coordinator.mm

-// Copyright 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#import "ios/chrome/browser/ui/settings/google_services/google_services_navigation_coordinator.h"
-
-#include "base/logging.h"
-#include "components/strings/grit/components_strings.h"
-#import "ios/chrome/browser/ui/icons/chrome_icon.h"
-#import "ios/chrome/browser/ui/settings/google_services/google_services_settings_coordinator.h"
-#import "ios/chrome/browser/ui/settings/google_services/google_services_settings_mode.h"
-#import "ios/chrome/browser/ui/settings/settings_navigation_controller.h"
-#include "ui/base/l10n/l10n_util_mac.h"
-
-#if !defined(__has_feature) || !__has_feature(objc_arc)
-#error "This file requires ARC support."
-#endif
-
-@interface GoogleServicesNavigationCoordinator () <
-    SettingsNavigationControllerDelegate>
-
-// Google services settings coordinator.
-@property(nonatomic, strong)
-    GoogleServicesSettingsCoordinator* googleServicesSettingsCoordinator;
-// Main view controller.
-@property(nonatomic, strong) SettingsNavigationController* navigationController;
-
-@end
-
-@implementation GoogleServicesNavigationCoordinator
-
-@synthesize googleServicesSettingsCoordinator =
-    _googleServicesSettingsCoordinator;
-@synthesize navigationController = _navigationController;
-@synthesize delegate = _delegate;
-
-- (void)start {
-  self.navigationController = [[SettingsNavigationController alloc]
-      initWithRootViewController:nil
-                    browserState:self.browserState
-                        delegate:self];
-  self.navigationController.modalPresentationStyle =
-      UIModalPresentationFormSheet;
-  self.googleServicesSettingsCoordinator =
-      [[GoogleServicesSettingsCoordinator alloc]
-          initWithBaseViewController:self.navigationController
-                        browserState:self.browserState
-                                mode:GoogleServicesSettingsModeSettings];
-  self.googleServicesSettingsCoordinator.dispatcher =
-      self.dispatcherForSettings;
-  self.googleServicesSettingsCoordinator.navigationController =
-      self.navigationController;
-  [self.googleServicesSettingsCoordinator start];
-  [self.baseViewController presentViewController:self.navigationController
-                                        animated:YES
-                                      completion:nil];
-}
-
-- (void)dismissAnimated:(BOOL)animated {
-  DCHECK_EQ(self.navigationController,
-            self.baseViewController.presentedViewController);
-  DCHECK(self.googleServicesSettingsCoordinator);
-  void (^completion)(void) = ^{
-    [self.googleServicesSettingsCoordinator stop];
-    self.googleServicesSettingsCoordinator.delegate = nil;
-    self.googleServicesSettingsCoordinator = nil;
-    [self.delegate googleServicesNavigationCoordinatorDidClose:self];
-  };
-  [self.baseViewController dismissViewControllerAnimated:animated
-                                              completion:completion];
-}
-
-#pragma mark - SettingsNavigationControllerDelegate
-
-- (void)closeSettings {
-  [self dismissAnimated:YES];
-}
-
-@end

ios/chrome/browser/ui/settings/settings_navigation_controller.h

@@ -69,7 +69,15 @@ newSettingsMainControllerWithBrowserState:(ios::ChromeBrowserState*)browserState
 newAccountsController:(ios::ChromeBrowserState*)browserState
              delegate:(id<SettingsNavigationControllerDelegate>)delegate;
 
-// Creates a new SignInSettingsCollectionViewController and the chrome around
+// Creates a new GoogleServicesSettingsCollectionViewController and the chrome
+// around it. |browserState| is used to personalize some settings aspects and
+// should not be nil. |delegate| may be nil.
++ (SettingsNavigationController*)
+    newGoogleServicesController:(ios::ChromeBrowserState*)browserState
+                       delegate:
+                           (id<SettingsNavigationControllerDelegate>)delegate;
+
+// Creates a new SyncSettingsCollectionViewController and the chrome around
 // it. |browserState| is used to personalize some settings aspects and should
 // not be nil. |delegate| may be nil.
 + (SettingsNavigationController*)

ios/chrome/browser/ui/settings/settings_navigation_controller.mm

@@ -92,6 +92,23 @@ newAccountsController:(ios::ChromeBrowserState*)browserState
   return nc;
 }
 
++ (SettingsNavigationController*)
+    newGoogleServicesController:(ios::ChromeBrowserState*)browserState
+                       delegate:
+                           (id<SettingsNavigationControllerDelegate>)delegate {
+  // GoogleServicesSettings uses a coordinator to be presented, therefore the
+  // view controller is not accessible. Prefer creating a
+  // |SettingsNavigationController| with a nil root view controller and then
+  // use the coordinator to push the GoogleServicesSettings as the first
+  // root view controller.
+  SettingsNavigationController* nc = [[SettingsNavigationController alloc]
+      initWithRootViewController:nil
+                    browserState:browserState
+                        delegate:delegate];
+  [nc showGoogleServices];
+  return nc;
+}
+
 + (SettingsNavigationController*)
      newSyncController:(ios::ChromeBrowserState*)browserState
               delegate:(id<SettingsNavigationControllerDelegate>)delegate {
@@ -285,6 +302,10 @@ initWithRootViewController:(UIViewController*)rootViewController
         ->PreUnityCommitChanges();
   }
 
+  // GoogleServicesSettingsCoordinator must be stopped before dismissing the
+  // sync settings view.
+  [self stopGoogleServicesSettingsCoordinator];
+
   // Reset the delegate to prevent any queued transitions from attempting to
   // close the settings.
   delegate_ = nil;
@@ -327,13 +348,40 @@ initWithRootViewController:(UIViewController*)rootViewController
   return closeButton;
 }
 
+// Pushes a GoogleServicesSettingsViewController on this settings navigation
+// controller. Does nothing id the top view controller is already of type
+// |GoogleServicesSettingsViewController|.
+- (void)showGoogleServices {
+  if ([self.topViewController
+          isKindOfClass:[GoogleServicesSettingsViewController class]]) {
+    // The top view controller is already the Google services settings panel.
+    // No need to open it.
+    return;
+  }
+  self.googleServicesSettingsCoordinator =
+      [[GoogleServicesSettingsCoordinator alloc]
+          initWithBaseViewController:self
+                        browserState:mainBrowserState_
+                                mode:GoogleServicesSettingsModeSettings];
+  self.googleServicesSettingsCoordinator.dispatcher =
+      [delegate_ dispatcherForSettings];
+  self.googleServicesSettingsCoordinator.navigationController = self;
+  self.googleServicesSettingsCoordinator.delegate = self;
+  [self.googleServicesSettingsCoordinator start];
+}
+
+// Stops the underlying Google services settings coordinator if it exists.
+- (void)stopGoogleServicesSettingsCoordinator {
+  [self.googleServicesSettingsCoordinator stop];
+  self.googleServicesSettingsCoordinator = nil;
+}
+
 #pragma mark - GoogleServicesSettingsCoordinatorDelegate
 
 - (void)googleServicesSettingsCoordinatorDidRemove:
     (GoogleServicesSettingsCoordinator*)coordinator {
   DCHECK_EQ(self.googleServicesSettingsCoordinator, coordinator);
-  [self.googleServicesSettingsCoordinator stop];
-  self.googleServicesSettingsCoordinator = nil;
+  [self stopGoogleServicesSettingsCoordinator];
 }
 
 #pragma mark - Accessibility
@@ -384,22 +432,7 @@ initWithRootViewController:(UIViewController*)rootViewController
 // TODO(crbug.com/779791) : Do not pass |baseViewController| through dispatcher.
 - (void)showGoogleServicesSettingsFromViewController:
     (UIViewController*)baseViewController {
-  if ([self.topViewController
-          isKindOfClass:[GoogleServicesSettingsViewController class]]) {
-    // The top view controller is already the Google services settings panel.
-    // No need to open it.
-    return;
-  }
-  self.googleServicesSettingsCoordinator =
-      [[GoogleServicesSettingsCoordinator alloc]
-          initWithBaseViewController:self
-                        browserState:mainBrowserState_
-                                mode:GoogleServicesSettingsModeSettings];
-  self.googleServicesSettingsCoordinator.dispatcher =
-      [delegate_ dispatcherForSettings];
-  self.googleServicesSettingsCoordinator.navigationController = self;
-  self.googleServicesSettingsCoordinator.delegate = self;
-  [self.googleServicesSettingsCoordinator start];
+  [self showGoogleServices];
 }
 
 // TODO(crbug.com/779791) : Do not pass |baseViewController| through dispatcher.