[ios] Refactor BrowserViewInformation

This CL inverts the relationship between |currentBVC| and |currentBrowserCoordinator|.
This CL makes |currentBrowserCoordinator| the source of truth, and |currentBVC| simply
returns the corresponding BVC.

Change-Id: Ibdcb79ca3f6fb2f3c2d8cecae1be036c58962dae
Reviewed-on: https://chromium-review.googlesource.com/c/1355995Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613140}

ios/chrome/app/main_controller.mm

@@ -399,6 +399,10 @@ enum class ShowTabSwitcherSnapshotResult {
   StartupTasks* _startupTasks;
 }
 
+// Redefined from BrowserViewInformation.
+@property(nonatomic, weak, readwrite)
+    BrowserCoordinator* currentBrowserCoordinator;
+
 // The main coordinator, lazily created the first time it is accessed. Manages
 // the main view controller. This property should not be accessed before the
 // browser has started up to the FOREGROUND stage.
@@ -1293,10 +1297,10 @@ enum class ShowTabSwitcherSnapshotResult {
   TabModel* tabModel;
   if (launchMode == ApplicationMode::INCOGNITO) {
     tabModel = otrTabModel;
-    self.currentBVC = self.otrBVC;
+    self.currentBrowserCoordinator = self.incognitoBrowserCoordinator;
   } else {
     tabModel = mainTabModel;
-    self.currentBVC = self.mainBVC;
+    self.currentBrowserCoordinator = self.mainBrowserCoordinator;
   }
   if (_tabSwitcherIsActive) {
     DCHECK(!_dismissingTabSwitcher);
@@ -1498,13 +1502,16 @@ enum class ShowTabSwitcherSnapshotResult {
                                        transition:ui::PAGE_TRANSITION_TYPED
                                        completion:nil];
     } else {
-      [self dismissModalDialogsWithCompletion:^{
-        self.currentBVC = [command inIncognito] ? self.otrBVC : self.mainBVC;
-        DCHECK(self.currentBVC.browserState->IsOffTheRecord() ==
-               command.inIncognito);
-        [self.currentBVC webPageOrderedOpen:command];
-      }
-                               dismissOmnibox:YES];
+      [self
+          dismissModalDialogsWithCompletion:^{
+            self.currentBrowserCoordinator =
+                [command inIncognito] ? self.incognitoBrowserCoordinator
+                                      : self.mainBrowserCoordinator;
+            DCHECK(self.currentBVC.browserState->IsOffTheRecord() ==
+                   command.inIncognito);
+            [self.currentBVC webPageOrderedOpen:command];
+          }
+                             dismissOmnibox:YES];
     }
 
   } else {
@@ -1799,17 +1806,28 @@ enum class ShowTabSwitcherSnapshotResult {
   return [_browserViewWrangler currentBVC];
 }
 
-// Note that the current tab of |bvc| will normally be reloaded by this method.
-// If a new tab is about to be added, call expectNewForegroundTab on the BVC
-// first to avoid extra work and possible page load side-effects for the tab
-// being replaced.
-- (void)setCurrentBVC:(BrowserViewController*)bvc {
-  DCHECK(bvc != nil);
-  if (self.currentBVC == bvc)
+- (BrowserCoordinator*)mainBrowserCoordinator {
+  DCHECK(_browserViewWrangler);
+  return _browserViewWrangler.mainBrowserCoordinator;
+}
+
+- (BrowserCoordinator*)incognitoBrowserCoordinator {
+  DCHECK(_browserViewWrangler);
+  return _browserViewWrangler.incognitoBrowserCoordinator;
+}
+
+// Note that the current tab of |browserCoordinator|'s BVC will normally be
+// reloaded by this method. If a new tab is about to be added, call
+// expectNewForegroundTab on the BVC first to avoid extra work and possible page
+// load side-effects for the tab being replaced.
+- (void)setCurrentBrowserCoordinator:(BrowserCoordinator*)browserCoordinator {
+  DCHECK(browserCoordinator);
+  if (self.currentBrowserCoordinator == browserCoordinator)
     return;
 
   DCHECK(_browserViewWrangler);
-  [_browserViewWrangler setCurrentBVC:bvc storageSwitcher:self];
+  [_browserViewWrangler setCurrentBrowserCoordinator:browserCoordinator
+                                     storageSwitcher:self];
 
   if (!_dismissingTabSwitcher)
     [self displayCurrentBVCAndFocusOmnibox:NO];
@@ -1856,7 +1874,7 @@ enum class ShowTabSwitcherSnapshotResult {
   if ([self.currentTabModel count] == 0U) {
     [self showTabSwitcher];
   } else {
-    self.currentBVC = self.mainBVC;
+    self.currentBrowserCoordinator = self.mainBrowserCoordinator;
   }
 }
 
@@ -1877,10 +1895,12 @@ enum class ShowTabSwitcherSnapshotResult {
 #pragma mark - Mode Switching
 
 - (void)switchModesAndOpenNewTab:(OpenNewTabCommand*)command {
-  BrowserViewController* bvc = command.inIncognito ? self.otrBVC : self.mainBVC;
-  DCHECK(bvc);
-  [bvc expectNewForegroundTab];
-  self.currentBVC = bvc;
+  BrowserCoordinator* browserCoordinator =
+      command.inIncognito ? self.incognitoBrowserCoordinator
+                          : self.mainBrowserCoordinator;
+  DCHECK(browserCoordinator);
+  [browserCoordinator.viewController expectNewForegroundTab];
+  self.currentBrowserCoordinator = browserCoordinator;
   [self openURLInNewTab:command];
 }
 
@@ -2103,9 +2123,10 @@ enum class ShowTabSwitcherSnapshotResult {
   DCHECK(tabModel == self.mainTabModel || tabModel == self.otrTabModel);
 
   _dismissingTabSwitcher = YES;
-  BrowserViewController* targetBVC =
-      (tabModel == self.mainTabModel) ? self.mainBVC : self.otrBVC;
-  self.currentBVC = targetBVC;
+  BrowserCoordinator* targetBrowserCoordinator =
+      (tabModel == self.mainTabModel) ? self.mainBrowserCoordinator
+                                      : self.incognitoBrowserCoordinator;
+  self.currentBrowserCoordinator = targetBrowserCoordinator;
 
   // The call to set currentBVC above does not actually display the BVC, because
   // _dismissingTabSwitcher is YES.  So: Force the BVC transition to start.
@@ -2120,10 +2141,10 @@ enum class ShowTabSwitcherSnapshotResult {
 
   if (_modeToDisplayOnTabSwitcherDismissal ==
       TabSwitcherDismissalMode::NORMAL) {
-    self.currentBVC = self.mainBVC;
+    self.currentBrowserCoordinator = self.mainBrowserCoordinator;
   } else if (_modeToDisplayOnTabSwitcherDismissal ==
              TabSwitcherDismissalMode::INCOGNITO) {
-    self.currentBVC = self.otrBVC;
+    self.currentBrowserCoordinator = self.incognitoBrowserCoordinator;
   }
 
   _modeToDisplayOnTabSwitcherDismissal = TabSwitcherDismissalMode::NONE;
@@ -2315,8 +2336,9 @@ enum class ShowTabSwitcherSnapshotResult {
                       withURL:(const GURL&)url
                    transition:(ui::PageTransition)transition
                    completion:(ProceduralBlock)completion {
-  BrowserViewController* targetBVC =
-      targetMode == ApplicationMode::NORMAL ? self.mainBVC : self.otrBVC;
+  BrowserCoordinator* targetBrowserCoordinator =
+      targetMode == ApplicationMode::NORMAL ? self.mainBrowserCoordinator
+                                            : self.incognitoBrowserCoordinator;
   NSUInteger tabIndex = NSNotFound;
 
   ProceduralBlock startupCompletion =
@@ -2349,26 +2371,29 @@ enum class ShowTabSwitcherSnapshotResult {
           targetMode == ApplicationMode::NORMAL
               ? TabSwitcherDismissalMode::NORMAL
               : TabSwitcherDismissalMode::INCOGNITO;
-      [targetBVC appendTabAddedCompletion:tabOpenedCompletion];
-      tab = [targetBVC addSelectedTabWithURL:url
-                                     atIndex:tabIndex
-                                  transition:transition];
+      [targetBrowserCoordinator.viewController
+          appendTabAddedCompletion:tabOpenedCompletion];
+      tab = [targetBrowserCoordinator.viewController
+          addSelectedTabWithURL:url
+                        atIndex:tabIndex
+                     transition:transition];
     } else {
       // Voice search, QRScanner and the omnibox are presented by the BVC.
       // They must be started after the BVC view is added in the hierarchy.
       self.NTPActionAfterTabSwitcherDismissal =
           [_startupParameters postOpeningAction];
       [self setStartupParameters:nil];
-      tab = [_tabSwitcher dismissWithNewTabAnimationToModel:targetBVC.tabModel
-                                                    withURL:url
-                                                    atIndex:tabIndex
-                                                 transition:transition];
+      tab = [_tabSwitcher
+          dismissWithNewTabAnimationToModel:targetBrowserCoordinator.tabModel
+                                    withURL:url
+                                    atIndex:tabIndex
+                                 transition:transition];
     }
   } else {
     if (!self.currentBVC.presentedViewController) {
-      [targetBVC expectNewForegroundTab];
+      [targetBrowserCoordinator.viewController expectNewForegroundTab];
     }
-    self.currentBVC = targetBVC;
+    self.currentBrowserCoordinator = targetBrowserCoordinator;
     tab = [self openOrReuseTabInMode:targetMode
                              withURL:url
                           transition:transition

ios/chrome/browser/ui/main/browser_view_information.h

@@ -18,19 +18,27 @@ class ChromeBrowserState;
 // Information about the Browser View, controllers and tab model.
 @protocol BrowserViewInformation<NSObject>
 
-// The normal (non-OTR) BrowserViewController
+// The normal (non-incognito, non-OTR) BrowserCoordinator.
+@property(nonatomic, strong, readonly)
+    BrowserCoordinator* mainBrowserCoordinator;
+// The incognito (a.k.a OTR) BrowserCoordinator.
+@property(nonatomic, strong, readonly)
+    BrowserCoordinator* incognitoBrowserCoordinator;
+// The BrowserCoordinator that is currently being used (one of mainBVC or
+// otrBVC). The other, if present, is in suspended mode..
+@property(nonatomic, weak, readonly)
+    BrowserCoordinator* currentBrowserCoordinator;
+
+// The BrowserViewController corresponding to |mainBrowserCoordinator|.
 @property(nonatomic, readonly) BrowserViewController* mainBVC;
-// The normal (non-OTR) TabModel corresponding to mainBVC.
+// The normal (non-OTR) TabModel corresponding to |mainBrowserCoordinator|.
 @property(nonatomic, retain) TabModel* mainTabModel;
-// The OTR BrowserViewController.
+// The BrowserViewController corresponding to |incognitoBrowserCoordinator|.
 @property(nonatomic, readonly) BrowserViewController* otrBVC;
-// The OTR TabModel corresponding to otrBVC.
+// The OTR TabModel corresponding to |incognitoBrowserCoordinator|.
 @property(nonatomic, retain) TabModel* otrTabModel;
-// The BrowserViewController that is currently being used (one of mainBVC or
-// otrBVC). The other, if present, is in suspended mode.
-@property(nonatomic, weak) BrowserViewController* currentBVC;
-// The BrowserCoordinator corresponding to |currentBVC|.
-@property(nonatomic, readonly) BrowserCoordinator* currentBrowserCoordinator;
+// The BrowserViewController corresponding to |currentBrowserCoordinator|.
+@property(nonatomic, readonly) BrowserViewController* currentBVC;
 
 // Halts all tabs from all TabModels.
 - (void)haltAllTabs;

ios/chrome/browser/ui/main/browser_view_wrangler.h

@@ -10,6 +10,7 @@
 #import "ios/chrome/browser/ui/main/browser_view_information.h"
 
 @protocol ApplicationCommands;
+@class BrowserCoordinator;
 @class DeviceSharingManager;
 @protocol TabModelObserver;
 
@@ -42,16 +43,14 @@ class ChromeBrowserState;
 
 - (instancetype)init NS_UNAVAILABLE;
 
-// Set the current BVC to be |bvc|, and use |storageSwitcher| to handle the
-// storage switch. |bvc| should be one of the view controller instances already
-// owned by the receiver (either |mainBVC| or |otrBVBC|), and this method does
-// not retain or take ownership of |bvc|.
-// Note that the BrowserViewInformation protocol defines
-// |currentBVC| as a readwrite property, so users of this class can directly
-// call -setCurrentBVC: and bypass the logic in this method; that should only be
-// done on BVC instances who do not yet have a browser state.
-- (void)setCurrentBVC:(BrowserViewController*)bvc
-      storageSwitcher:(id<BrowserStateStorageSwitching>)storageSwitcher;
+// Set the current BrowserCoordinator to be |browserCoordinator|, and use
+// |storageSwitcher| to handle the storage switch. |browserCoordinator| should
+// be one of the BrowserCoordinator instances already owned by the receiver
+// (either |mainBrowserCoordinator| or |incognitoBrowserCoordinator|), and this
+// method does not retain or take ownership of |browserCoordinator|.
+- (void)setCurrentBrowserCoordinator:(BrowserCoordinator*)browserCoordinator
+                     storageSwitcher:
+                         (id<BrowserStateStorageSwitching>)storageSwitcher;
 
 // Update the device sharing manager. This should be done after updates to the
 // tab model. This class creates and manages the state of the sharing manager.

ios/chrome/browser/ui/main/browser_view_wrangler.mm

@@ -33,12 +33,6 @@
   BOOL _isShutdown;
 }
 
-// Coordinator for non-incognito BVC.
-@property(nonatomic, strong) BrowserCoordinator* mainCoordinator;
-
-// Coordinator for incognito BVC.
-@property(nonatomic, strong) BrowserCoordinator* incognitoCoordinator;
-
 // Responsible for maintaining all state related to sharing to other devices.
 // Redeclared readwrite from the readonly declaration in the Testing interface.
 @property(nonatomic, strong, readwrite)
@@ -65,12 +59,12 @@
 @implementation BrowserViewWrangler
 
 // Properties defined in the BrowserViewInformation protocol.
+@synthesize mainBrowserCoordinator = _mainBrowserCoordinator;
+@synthesize incognitoBrowserCoordinator = _incognitoBrowserCoordinator;
+@synthesize currentBrowserCoordinator = _currentBrowserCoordinator;
 @synthesize mainTabModel = _mainTabModel;
 @synthesize otrTabModel = _otrTabModel;
-@synthesize currentBVC = _currentBVC;
 // Private properies.
-@synthesize mainCoordinator = _mainCoordinator;
-@synthesize incognitoCoordinator = _incognitoCoordinator;
 @synthesize deviceSharingManager = _deviceSharingManager;
 
 - (instancetype)initWithBrowserState:(ios::ChromeBrowserState*)browserState
@@ -91,17 +85,20 @@
 
 #pragma mark - BrowserViewInformation property implementations
 
-- (BrowserViewController*)mainBVC {
-  if (!self.mainCoordinator.viewController) {
-    // |_browserState| should always be set before trying to create
-    // |mainBVC|.
-    DCHECK(_browserState);
-    self.mainCoordinator = [self coordinatorForBrowserState:_browserState
-                                                   tabModel:self.mainTabModel];
-    [self.mainCoordinator start];
-    DCHECK(self.mainCoordinator.viewController);
+- (BrowserCoordinator*)mainBrowserCoordinator {
+  if (!_mainBrowserCoordinator) {
+    _mainBrowserCoordinator =
+        [self coordinatorForBrowserState:_browserState
+                                tabModel:self.mainTabModel];
+    [_mainBrowserCoordinator start];
+    DCHECK(_mainBrowserCoordinator.viewController);
   }
-  return self.mainCoordinator.viewController;
+  return _mainBrowserCoordinator;
+}
+
+- (BrowserViewController*)mainBVC {
+  DCHECK(self.mainBrowserCoordinator.viewController);
+  return self.mainBrowserCoordinator.viewController;
 }
 
 - (TabModel*)mainTabModel {
@@ -132,21 +129,23 @@
   _mainTabModel = mainTabModel;
 }
 
-- (BrowserViewController*)otrBVC {
-  if (!self.incognitoCoordinator.viewController) {
-    // |_browserState| should always be set before trying to create
-    // |otrBVC|.
-    DCHECK(_browserState);
+- (BrowserCoordinator*)incognitoBrowserCoordinator {
+  if (!_incognitoBrowserCoordinator) {
     ios::ChromeBrowserState* otrBrowserState =
         _browserState->GetOffTheRecordChromeBrowserState();
     DCHECK(otrBrowserState);
-    self.incognitoCoordinator =
+    _incognitoBrowserCoordinator =
         [self coordinatorForBrowserState:otrBrowserState
                                 tabModel:self.otrTabModel];
-    [self.incognitoCoordinator start];
-    DCHECK(self.incognitoCoordinator.viewController);
+    [_incognitoBrowserCoordinator start];
+    DCHECK(_incognitoBrowserCoordinator.viewController);
   }
-  return self.incognitoCoordinator.viewController;
+  return _incognitoBrowserCoordinator;
+}
+
+- (BrowserViewController*)otrBVC {
+  DCHECK(self.incognitoBrowserCoordinator.viewController);
+  return self.incognitoBrowserCoordinator.viewController;
 }
 
 - (TabModel*)otrTabModel {
@@ -172,46 +171,48 @@
   _otrTabModel = otrTabModel;
 }
 
-- (void)setCurrentBVC:(BrowserViewController*)bvc
-      storageSwitcher:(id<BrowserStateStorageSwitching>)storageSwitcher {
-  DCHECK(bvc != nil);
-  // |bvc| should be one of the BrowserViewControllers this class already owns.
-  DCHECK(self.mainBVC == bvc || self.otrBVC == bvc);
-  if (self.currentBVC == bvc) {
+- (void)setCurrentBrowserCoordinator:(BrowserCoordinator*)browserCoordinator
+                     storageSwitcher:
+                         (id<BrowserStateStorageSwitching>)storageSwitcher {
+  DCHECK(browserCoordinator);
+  // |browserCoordinator| should be one of the BrowserCoordinators this class
+  // already owns.
+  DCHECK(self.mainBrowserCoordinator == browserCoordinator ||
+         self.incognitoBrowserCoordinator == browserCoordinator);
+  if (self.currentBrowserCoordinator == browserCoordinator) {
     return;
   }
 
-  if (self.currentBVC) {
+  if (self.currentBrowserCoordinator) {
     // Tell the current BVC it moved to the background.
-    [self.currentBVC setPrimary:NO];
+    [self.currentBrowserCoordinator.viewController setPrimary:NO];
 
     // Data storage for the browser is always owned by the current BVC, so it
     // must be updated when switching between BVCs.
-    [storageSwitcher changeStorageFromBrowserState:self.currentBVC.browserState
-                                    toBrowserState:bvc.browserState];
+    [storageSwitcher
+        changeStorageFromBrowserState:self.currentBrowserCoordinator
+                                          .browserState
+                       toBrowserState:browserCoordinator.browserState];
   }
 
-  self.currentBVC = bvc;
+  _currentBrowserCoordinator = browserCoordinator;
 
   // The internal state of the Handoff Manager depends on the current BVC.
   [self updateDeviceSharingManager];
 }
 
-- (BrowserCoordinator*)currentBrowserCoordinator {
-  if (self.currentBVC == self.otrBVC) {
-    return self.incognitoCoordinator;
-  }
-  return self.mainCoordinator;
+- (BrowserViewController*)currentBVC {
+  return self.currentBrowserCoordinator.viewController;
 }
 
 #pragma mark - BrowserViewInformation methods
 
 - (TabModel*)currentTabModel {
-  return self.currentBVC.tabModel;
+  return self.currentBrowserCoordinator.tabModel;
 }
 
 - (ios::ChromeBrowserState*)currentBrowserState {
-  return self.currentBVC.browserState;
+  return self.currentBrowserCoordinator.browserState;
 }
 
 - (void)haltAllTabs {
@@ -245,9 +246,10 @@
   [self.deviceSharingManager updateBrowserState:_browserState];
 
   GURL activeURL;
-  Tab* currentTab = [self.currentBVC tabModel].currentTab;
+  Tab* currentTab = self.currentBrowserCoordinator.tabModel.currentTab;
   // Set the active URL if there's a current tab and the current BVC is not OTR.
-  if (currentTab.webState && self.currentBVC != self.otrBVC) {
+  if (currentTab.webState &&
+      self.currentBrowserCoordinator != self.incognitoBrowserCoordinator) {
     activeURL = currentTab.webState->GetVisibleURL();
   }
   [self.deviceSharingManager updateActiveURL:activeURL];
@@ -263,19 +265,21 @@
   // Stop watching the OTR tab model's state for crashes.
   breakpad::StopMonitoringTabStateForTabModel(self.otrTabModel);
 
-  // At this stage, a new OTR BVC shouldn't be lazily constructed by calling the
-  // .otrBVC property getter.
+  // At this stage, a new incognitoBrowserCoordinator shouldn't be lazily
+  // constructed by calling the property getter.
   BOOL otrBVCIsCurrent =
-      self.currentBVC == self.incognitoCoordinator.viewController;
+      self.currentBrowserCoordinator == _incognitoBrowserCoordinator;
   @autoreleasepool {
-    [self.incognitoCoordinator stop];
-    self.incognitoCoordinator = nil;
+    // At this stage, a new incognitoBrowserCoordinator shouldn't be lazily
+    // constructed by calling the property getter.
+    [_incognitoBrowserCoordinator stop];
+    _incognitoBrowserCoordinator = nil;
 
     // There's no guarantee the tab model was ever added to the BVC (or even
     // that the BVC was created), so ensure the tab model gets notified.
     self.otrTabModel = nil;
     if (otrBVCIsCurrent) {
-      _currentBVC = nil;
+      _currentBrowserCoordinator = nil;
     }
   }
 
@@ -290,7 +294,7 @@
   DCHECK(_browserState->HasOffTheRecordChromeBrowserState());
 
   if (otrBVCIsCurrent) {
-    _currentBVC = self.otrBVC;
+    _currentBrowserCoordinator = self.incognitoBrowserCoordinator;
   }
 }
 
@@ -322,10 +326,12 @@
   [_mainTabModel browserStateDestroyed];
   [_otrTabModel browserStateDestroyed];
 
-  [self.mainCoordinator stop];
-  self.mainCoordinator = nil;
-  [self.incognitoCoordinator stop];
-  self.incognitoCoordinator = nil;
+  // At this stage, new BrowserCoordinators shouldn't be lazily constructed by
+  // calling their property getters.
+  [_mainBrowserCoordinator stop];
+  _mainBrowserCoordinator = nil;
+  [_incognitoBrowserCoordinator stop];
+  _incognitoBrowserCoordinator = nil;
 
   _browserState = nullptr;
 }