Refactor TabModel initialisation from a saved session.

ios/chrome/browser/tabs/tab_model.mm

Index: ios/chrome/browser/tabs/tab_model.mm
diff --git a/ios/chrome/browser/tabs/tab_model.mm b/ios/chrome/browser/tabs/tab_model.mm
index 0d324f991f1d252ad29533c3c68245fc6b023216..c770533f678a4607d55469c044e7cf6d01f78a62 100644
--- a/ios/chrome/browser/tabs/tab_model.mm
+++ b/ios/chrome/browser/tabs/tab_model.mm
@@ -157,6 +157,7 @@ void CleanCertificatePolicyCache(
                     openedByDOM:(BOOL)openedByDOM
                         atIndex:(NSUInteger)index
                    inBackground:(BOOL)inBackground;
+
 // Call to switch the selected tab. Broadcasts about the change in selection.
 // It's ok for |newTab| to be nil in case the last tab is going away. In that
 // case, the "tab deselected" notification gets sent, but no corresponding
@@ -165,11 +166,20 @@ void CleanCertificatePolicyCache(
 - (void)changeSelectedTabFrom:(Tab*)oldTab
                            to:(Tab*)newTab
                  persistState:(BOOL)persist;
+
 // Tells the snapshot cache the adjacent tab session ids.
 - (void)updateSnapshotCache:(Tab*)tab;
+
 // Helper method that posts a notification with the given name with |tab|
 // in the userInfo dictionary under the kTabModelTabKey.
 - (void)postNotificationName:(NSString*)notificationName withTab:(Tab*)tab;
+
+// Helper method to restore a saved session and control if the state should
+// be persisted or not. Used to implement the public -restoreSessionWindow:
+// method and restoring session in the initialiser.
+- (BOOL)restoreSessionWindow:(SessionWindowIOS*)window
+                persistState:(BOOL)persistState;
+
 @end
 
 @implementation TabModel
@@ -256,56 +266,34 @@ void CleanCertificatePolicyCache(
     _syncedWindowDelegate.reset(new TabModelSyncedWindowDelegate(self));
 
     _tabs.reset([[NSMutableArray alloc] init]);
-    NSNotificationCenter* defaultCenter = [NSNotificationCenter defaultCenter];
     if (window) {
-      web::WebState::CreateParams params(_browserState);
-      for (CRWNavigationManagerStorage* session in window.sessions) {
-        std::unique_ptr<web::WebState> webState =
-            web::WebState::Create(params, session);
-        DCHECK_EQ(webState->GetBrowserState(), _browserState);
-        // Restore the CertificatePolicyCache.
-        UpdateCertificatePolicyCacheFromWebState(webState.get());
-        // Create a new tab for each entry in the window. Don't send delegate
-        // notifications for each restored tab, only when all done.
-        base::scoped_nsobject<Tab> tab(
-            [[Tab alloc] initWithWebState:std::move(webState) model:self]);
-        [tab webController].usePlaceholderOverlay = YES;
-        [tab fetchFavicon];
-        [_tabs addObject:tab];
-
-        TabParentingGlobalObserver::GetInstance()->OnTabParented(
-            tab.get().webState);
-      }
-      if ([_tabs count]) {
-        DCHECK(window.selectedIndex < [_tabs count]);
-        _currentTab.reset([self tabAtIndex:window.selectedIndex]);
-        DCHECK(_currentTab);
-        if (_tabUsageRecorder)
-          _tabUsageRecorder->InitialRestoredTabs(_currentTab, _tabs);
-        // Perform initializations for affiliated objects which update the
-        // session information related to the current tab.
-        [_currentTab updateLastVisitedTimestamp];
-        [self saveSessionImmediately:NO];
-      }
+      // Restore the session and reset the session metrics (as the event have
+      // not been generated by the user but by a cold start cycle).
+      [self restoreSessionWindow:window persistState:NO];

[rohitrao (ping after 24h), 2017/02/06 13:45:40]

Is it worth DCHECKing that _observers is still empty before calling
restoreSessionWindow?  It's possible that we could accidentally add an observer
at some point in the future.

[sdefresne, 2017/02/06 14:10:45]

The only code path that can lead to adding observer is -addObserver:. Since this
is a public method of that class, I can't see how it can be called before the
initialiser returns. I can add a DCHECK here but I think it is not useful
(especially given that _observers is reset 20 lines higher in the same method).

[rohitrao (ping after 24h), 2017/02/06 14:15:57]

As an example, what would happen if we modified _tabUsageRecorder or
_syncedWindowDelegate to add themselves as delegates?  What happens if you call
a public method on an object that's still being initialized? =)

[sdefresne, 2017/02/06 14:34:02]

Called public method on an object that is not initialised can happens but is
discouraged (this is why the style recommend not using properties in the
initialiser as a subclass may do some processing in the property setter). We've
also had issues with some classes we inherited from that called a public method
at the end of their initialiser (something like [self reloadData]) in MDC IIRC.

I could argue that adding _syncedWindowDelegate or _tabUsageRecorder to
_observers would be something that the owner of TabModel would make
intentionally and thus they would be able to decide whether they want to be
invoked for the events (if they don't they can change the order of the object
initialisation) :-)

But since I don't care that much, I've just added the DCHECK.

+      [self resetSessionMetrics];

[rohitrao (ping after 24h), 2017/02/06 13:45:40]

This was another difference between the two functions -- whether or not adding
the tabs logged any metrics.  It may be worth calling this out in the CL
description, and adding a line saying that metrics are logged, but immediately
wiped.

[sdefresne, 2017/02/06 14:10:45]

Done.

     }
 
     _orderController.reset(
         [[TabModelOrderController alloc] initWithTabModel:self]);
+
     // Register for resign active notification.
-    [defaultCenter addObserver:self
-                      selector:@selector(willResignActive:)
-                          name:UIApplicationWillResignActiveNotification
-                        object:nil];
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(willResignActive:)
+               name:UIApplicationWillResignActiveNotification
+             object:nil];
     // Register for background notification.
-    [defaultCenter addObserver:self
-                      selector:@selector(applicationDidEnterBackground:)
-                          name:UIApplicationDidEnterBackgroundNotification
-                        object:nil];
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(applicationDidEnterBackground:)
+               name:UIApplicationDidEnterBackgroundNotification
+             object:nil];
     // Register for foregrounding notification.
-    [defaultCenter addObserver:self
-                      selector:@selector(applicationWillEnterForeground:)
-                          name:UIApplicationWillEnterForegroundNotification
-                        object:nil];
+    [[NSNotificationCenter defaultCenter]
+        addObserver:self
+           selector:@selector(applicationWillEnterForeground:)
+               name:UIApplicationWillEnterForegroundNotification
+             object:nil];
 
     // Associate with ios::ChromeBrowserState.
     RegisterTabModelWithChromeBrowserState(_browserState, self);
@@ -319,55 +307,7 @@ void CleanCertificatePolicyCache(
 }
 
 - (BOOL)restoreSessionWindow:(SessionWindowIOS*)window {
-  DCHECK(_browserState);
-  DCHECK(window);
-  NSArray* sessions = window.sessions;
-  if (!sessions.count)
-    return NO;
-  size_t oldCount = [_tabs count];
-  size_t index = oldCount;
-  web::WebState::CreateParams params(_browserState);
-  for (CRWNavigationManagerStorage* session in sessions) {
-    std::unique_ptr<web::WebState> webState =
-        web::WebState::Create(params, session);
-    DCHECK_EQ(webState->GetBrowserState(), _browserState);
-    Tab* tab = [self insertTabWithWebState:std::move(webState) atIndex:index++];
-    tab.webController.usePlaceholderOverlay = YES;
-    // Restore the CertificatePolicyCache. Note that after calling Pass()
-    // |webState| is invalid, so we need to get the webstate from |tab|.
-    UpdateCertificatePolicyCacheFromWebState(tab.webState);
-  }
-  DCHECK([_tabs count] > oldCount);
-  // If any tab was restored, the saved selected tab must be selected.
-  if ([_tabs count] > oldCount) {
-    NSUInteger selectedIndex = window.selectedIndex;
-    if (selectedIndex == NSNotFound)
-      selectedIndex = oldCount;
-    else
-      selectedIndex += oldCount;
-    DCHECK(selectedIndex < [_tabs count]);
-    Tab* newTab = [self tabAtIndex:selectedIndex];
-    DCHECK(newTab);
-    [self changeSelectedTabFrom:_currentTab to:newTab persistState:YES];
-
-    // If there was only one tab and it was the new tab page, clobber it.
-    if (oldCount == 1) {
-      Tab* tab = [_tabs objectAtIndex:0];
-      if (tab.url == GURL(kChromeUINewTabURL)) {
-        [self closeTab:tab];
-        if (_tabUsageRecorder)
-          _tabUsageRecorder->InitialRestoredTabs(_currentTab, _tabs);
-        return YES;
-      }
-    }
-    if (_tabUsageRecorder) {
-      _tabUsageRecorder->InitialRestoredTabs(
-          _currentTab,
-          [_tabs subarrayWithRange:NSMakeRange(oldCount,
-                                               [_tabs count] - oldCount)]);
-    }
-  }
-  return NO;
+  return [self restoreSessionWindow:window persistState:YES];
 }
 
 - (void)saveSessionImmediately:(BOOL)immediately {
@@ -581,6 +521,7 @@ void CleanCertificatePolicyCache(
   [tab fetchFavicon];
   [_tabs insertObject:tab atIndex:index];
 
+  TabParentingGlobalObserver::GetInstance()->OnTabParented(tab.webState);

[rohitrao (ping after 24h), 2017/02/06 13:45:40]

This is called a lot more often now, isn't it?  It previously was only called
when tabs were added during init.

[sdefresne, 2017/02/06 14:10:45]

The method should have been called here too. The method documentation says that
TabParentingGlobalObserver "[a]llows clients to observe every tab (i.e.,
WebState) that is parented". Previously it was only called for Tab created in
the initialiser when restoring a session during a cold start. So I think it was
a bug that this method was not invoked here too.

There are two client registering with TabParentingGlobalObserver: 
- IOSChromeLocalSessionEventRouter
- IOSChromeMetricsServiceClient

The second, IOSChromeMetricsServiceClient, informs the metrics code that the
client is not idle, which I think is coreect (i.e. if the user opens a new tab,
I would expect that would mean the client is not idle). The same method is
called every time the user interact with the omnibox, so no ill-effect I think.

The first, IOSChromeLocalSessionEventRouter, does inform the sync service that
there were some activity on the WebState. Since it is a
web::GlobalWebStateObserver, it will also be notified when any navigation events
happens for the WebState, so I don't think it would have any ill-effect.

BTW, I think that this method
TabParentingGlobalObserver::GetInstance()->OnTabParented() may also have to be
called from -replaceTab:withTab: since this cause the Tab* to be parented with
TabModel. WDYT?

[rohitrao (ping after 24h), 2017/02/06 14:15:57]

I looked at the callsites as well and decided that this change would be fine. 
Should we move it (and the additional call in replaceTab) to a separate CL,
since these are larger behavioral changes than anything else in this CL?

[sdefresne, 2017/02/06 14:34:02]

Sure, done => https://codereview.chromium.org/2678823002.

   [_observers tabModel:self didInsertTab:tab atIndex:index inForeground:NO];
   [_observers tabModelDidChangeTabCount:self];
 
@@ -1000,6 +941,61 @@ void CleanCertificatePolicyCache(
                                                     userInfo:userInfo];
 }
 
+- (BOOL)restoreSessionWindow:(SessionWindowIOS*)window
+                persistState:(BOOL)persistState {
+  DCHECK(_browserState);
+  DCHECK(window);
+
+  NSArray* sessions = window.sessions;
+  if (!sessions.count)
+    return NO;
+
+  size_t oldCount = [_tabs count];
+  web::WebState::CreateParams params(_browserState);
+
+  for (CRWNavigationManagerStorage* session in sessions) {
+    std::unique_ptr<web::WebState> webState =
+        web::WebState::Create(params, session);
+    DCHECK_EQ(webState->GetBrowserState(), _browserState);
+    Tab* tab =
+        [self insertTabWithWebState:std::move(webState) atIndex:[_tabs count]];
+    tab.webController.usePlaceholderOverlay = YES;
+
+    // Restore the CertificatePolicyCache (note that webState is invalid after
+    // passing it via move semantic to -insertTabWithWebState:atIndex:).
+    UpdateCertificatePolicyCacheFromWebState(tab.webState);
+  }
+  DCHECK_GT([_tabs count], oldCount);
+
+  // Update the selected tab if there was a selected Tab in the saved session.
+  if (window.selectedIndex != NSNotFound) {
+    NSUInteger selectedIndex = window.selectedIndex + oldCount;
+    DCHECK_LT(selectedIndex, [_tabs count]);
+    DCHECK([self tabAtIndex:selectedIndex]);
+    [self changeSelectedTabFrom:_currentTab
+                             to:[self tabAtIndex:selectedIndex]
+                   persistState:persistState];
+  }
+
+  // If there was only one tab and it was the new tab page, clobber it.
+  BOOL closedNTPTab = NO;
+  if (oldCount == 1) {
+    Tab* tab = [_tabs objectAtIndex:0];
+    if (tab.url == GURL(kChromeUINewTabURL)) {
+      [self closeTab:tab];
+      closedNTPTab = YES;
+      oldCount = 0;
+    }
+  }
+  if (_tabUsageRecorder) {
+    _tabUsageRecorder->InitialRestoredTabs(
+        _currentTab,
+        [_tabs
+            subarrayWithRange:NSMakeRange(oldCount, [_tabs count] - oldCount)]);
+  }
+  return closedNTPTab;
+}
+
 #pragma mark - Notification Handlers
 
 // Called when UIApplicationWillResignActiveNotification is received.