Convert some of TabModel logic to WebStateListObservers.

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 2a344082d667b86259ade56e52b5ad9f66d541aa..6647782cca00545df536dfcc06a70bbd0876de8d 100644
--- a/ios/chrome/browser/tabs/tab_model.mm
+++ b/ios/chrome/browser/tabs/tab_model.mm
@@ -31,9 +31,12 @@
 #import "ios/chrome/browser/tabs/legacy_tab_helper.h"
 #import "ios/chrome/browser/tabs/tab.h"
 #import "ios/chrome/browser/tabs/tab_model_list.h"
+#import "ios/chrome/browser/tabs/tab_model_metrics_observer.h"
 #import "ios/chrome/browser/tabs/tab_model_observers.h"
 #import "ios/chrome/browser/tabs/tab_model_order_controller.h"
 #import "ios/chrome/browser/tabs/tab_model_synced_window_delegate.h"
+#import "ios/chrome/browser/tabs/tab_model_tab_model_observers_bridge.h"
+#import "ios/chrome/browser/tabs/tab_model_tab_parenting_observer.h"
 #import "ios/chrome/browser/xcallback_parameters.h"
 #import "ios/shared/chrome/browser/tabs/web_state_list.h"
 #import "ios/shared/chrome/browser/tabs/web_state_list_fast_enumeration_helper.h"
@@ -128,6 +131,10 @@ void CleanCertificatePolicyCache(
   // WebState owns the associated Tab.
   base::scoped_nsobject<NSMutableSet<Tab*>> _tabs;
 
+  // WebStateListObserver bridges to react to modifications of the model (may
+  // send notification, translate and forward events, update metrics, ...).
+  std::vector<std::unique_ptr<WebStateListObserver>> _observerBridges;
+
   // Maintains policy for where new tabs go and the selection when a tab
   // is removed.
   base::scoped_nsobject<TabModelOrderController> _orderController;
@@ -214,6 +221,12 @@ void CleanCertificatePolicyCache(
   DCHECK(!_browserState);
 
   [[NSNotificationCenter defaultCenter] removeObserver:self];
+
+  // Unregister all listeners before closing all the tabs.
+  for (const auto& observerBridge : _observerBridges)
+    _webStateList.RemoveObserver(observerBridge.get());
+  _observerBridges.clear();
+
   // Make sure the tabs do clean after themselves. It is important for
   // removeObserver: to be called first otherwise a lot of unecessary work will
   // happen on -closeAllTabs.
@@ -273,6 +286,17 @@ void CleanCertificatePolicyCache(
         initWithWebStateList:&_webStateList
                 proxyFactory:self]);
 
+    _observerBridges.push_back(base::MakeUnique<WebStateListObserverBridge>(
+        [[TabModelTabParentingObserver alloc] init]));
+    _observerBridges.push_back(base::MakeUnique<WebStateListObserverBridge>(
+        [[TabModelTabModelObserversBridge alloc]
+             initWithTabModel:self
+            tabModelObservers:_observers.get()]));
+    _observerBridges.push_back(base::MakeUnique<WebStateListObserverBridge>(
+        [[TabModelMetricsObserver alloc] init]));
+    for (const auto& observerBridge : _observerBridges)
+      _webStateList.AddObserver(observerBridge.get());
+
     _browserState = browserState;
     DCHECK(_browserState);
 
@@ -544,15 +568,10 @@ void CleanCertificatePolicyCache(
 - (void)insertTab:(Tab*)tab atIndex:(NSUInteger)index {
   DCHECK(tab);
   DCHECK(![_tabs containsObject:tab]);
-  [tab fetchFavicon];

[marq (ping after 24h), 2017/02/13 17:06:54]

Any idea why [tab setParentTabModel:self] isn't called in this method? I was
wondering if we could just move the -fetchFavicon calls into there, where at
least they would be contained in the "right" place, but it looks like the
current implementation is asymmetric and only calls -setParentTabModel in
-replaceTab:withTab:

[sdefresne, 2017/02/13 17:13:39]

Yes, this is because this method is only ever called from one of TabModel method
that pass "self" as TabModel to the Tab initialiser. Tab -setParentTabModel:
DCHECK that parentTabModel_ ivar is nil or that tabModel is nil (to allow
resetting it before deallocation). The is no way to get the parentTabModel from
Tab public API, so it is not possible to see if it is correct and only
conditionally calling -setParentTabModel:.

I was considering changing this, but I think it should be done as a separate CL
(after introducing a protocol to break the dependency cycle between Tab and
TabModel).

 
   [_tabs addObject:tab];
   _webStateList.InsertWebState(index, tab.webState);
-  TabParentingGlobalObserver::GetInstance()->OnTabParented(tab.webState);
-  [_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
@@ -565,34 +584,27 @@ void CleanCertificatePolicyCache(
 - (void)moveTab:(Tab*)tab toIndex:(NSUInteger)toIndex {
   size_t fromIndex = _webStateList.GetIndexOfWebState(tab.webState);
   _webStateList.MoveWebStateAt(fromIndex, toIndex);
-  [_observers tabModel:self didMoveTab:tab fromIndex:fromIndex toIndex:toIndex];
 }
 
 - (void)replaceTab:(Tab*)oldTab withTab:(Tab*)newTab {
   NSUInteger index = [self indexOfTab:oldTab];
   DCHECK_NE(NSNotFound, static_cast<NSInteger>(index));
+
   DCHECK([_tabs containsObject:oldTab]);
   DCHECK(![_tabs containsObject:newTab]);
 
   base::scoped_nsobject<Tab> tabSaver([oldTab retain]);
-  [newTab fetchFavicon];
   [_tabs removeObject:oldTab];
   [_tabs addObject:newTab];
   [newTab setParentTabModel:self];
 
   _webStateList.ReplaceWebStateAt(index, newTab.webState);
-  TabParentingGlobalObserver::GetInstance()->OnTabParented(newTab.webState);
-  [_observers tabModel:self didReplaceTab:oldTab withTab:newTab atIndex:index];
 
   if (self.currentTab == oldTab)
     [self changeSelectedTabFrom:nil to:newTab persistState:NO];
 
   [oldTab setParentTabModel:nil];
   [oldTab close];
-
-  // Record a tab clobber, since swapping tabs bypasses the tab code that would
-  // normally log clobbers.
-  base::RecordAction(base::UserMetricsAction("MobileTabClobbered"));
 }
 
 - (void)closeTabAtIndex:(NSUInteger)index {
@@ -748,8 +760,6 @@ void CleanCertificatePolicyCache(
   [_tabs removeObject:closedTab];
 
   _webStateList.RemoveWebStateAt(closedTabIndex);
-  [_observers tabModel:self didRemoveTab:closedTab atIndex:closedTabIndex];
-  [_observers tabModelDidChangeTabCount:self];
 
   // Current tab has closed, update the selected tab and swap in its
   // contents. There is nothing to do if a non-selected tab is closed as
@@ -761,7 +771,6 @@ void CleanCertificatePolicyCache(
   } else {
     [self saveSessionImmediately:NO];
   }
-  base::RecordAction(base::UserMetricsAction("MobileTabClosed"));
   ++_closedTabCount;
 }