Refactor TabModel initialisation from a saved session.

ios/chrome/browser/tabs/tab_model.mm

 // Copyright 2012 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/tabs/tab_model.h"
 
 #include <list>
 #include <utility>
 #include <vector>
 
@@ skipping 139 matching lines @@
 // The |index| parameter can be set to
 // TabModelConstants::kTabPositionAutomatically if the caller doesn't have a
 // preference for the position of the tab.
 - (Tab*)insertTabWithLoadParams:
             (const web::NavigationManager::WebLoadParams&)params
                      windowName:(NSString*)windowName
                          opener:(Tab*)parentTab
                     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
 // "tab selected" notification is sent. |persist| indicates whether or not
 // the tab's state should be persisted in history upon switching.
 - (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
 
 @synthesize browserState = _browserState;
 @synthesize sessionID = _sessionID;
 @synthesize webUsageEnabled = webUsageEnabled_;
 
 #pragma mark - Overriden
 
@@ skipping 66 matching lines @@
     // Normal browser states are the only ones to get tab restore. Tab sync
     // handles incognito browser states by filtering on profile, so it's
     // important to the backend code to always have a sync window delegate.
     if (!_browserState->IsOffTheRecord()) {
       // Set up the usage recorder before tabs are created.
       _tabUsageRecorder.reset(new TabUsageRecorder(self));
     }
     _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);
   }
   return self;
 }
 
 - (instancetype)init {
   NOTREACHED();
   return nil;
 }
 
 - (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 {
   // Do nothing if there are tabs in the model but no selected tab. This is
   // a transitional state.
   if ((!_currentTab && [_tabs count]) || !_browserState)
     return;
   [_sessionService saveWindow:self.windowForSavingSession
               forBrowserState:_browserState
                   immediately:immediately];
@@ skipping 193 matching lines @@
   [self insertTab:tab atIndex:index];
   return tab;
 }
 
 - (void)insertTab:(Tab*)tab atIndex:(NSUInteger)index {
   DCHECK(tab);
   DCHECK(index <= [_tabs count]);
   [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];
 
   base::RecordAction(base::UserMetricsAction("MobileNewTabOpened"));
   // Persist the session due to a new tab being inserted. If this is a
   // background tab (will not become active), saving now will capture the
   // state properly. If it does eventually become active, another save will
   // be triggered to properly capture the end result.
   [self saveSessionImmediately:NO];
   ++_newTabCount;
@@ skipping 399 matching lines @@
   // initializer dictionaryWithObject, because that approach adds the dictionary
   // to the autorelease pool, which in turn holds Tab alive longer than
   // necessary.
   base::scoped_nsobject<NSDictionary> userInfo(
       [[NSDictionary alloc] initWithObjectsAndKeys:tab, kTabModelTabKey, nil]);
   [[NSNotificationCenter defaultCenter] postNotificationName:notificationName
                                                       object:self
                                                     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.
 - (void)willResignActive:(NSNotification*)notify {
   if (webUsageEnabled_ && _currentTab) {
     [[SnapshotCache sharedInstance]
         willBeSavedGreyWhenBackgrounding:_currentTab.get().tabId];
   }
 }
 
@@ skipping 60 matching lines @@
   web::NavigationManager::WebLoadParams params(URL);
   params.referrer = referrer;
   params.transition_type = ui::PAGE_TRANSITION_TYPED;
   [[tab webController] loadWithParams:params];
   [tab webController].webUsageEnabled = webUsageEnabled_;
   [self insertTab:tab atIndex:index];
   return tab;
 }
 
 @end