[ios] Use WebStateList to back the tab grid.

ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm

Index: ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
diff --git a/ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm b/ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
index 1b620951c1f70b2fac738686876e148534d96095..6682dd01de5e8a041972c4b26ed5b295b5db396c 100644
--- a/ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
+++ b/ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm
@@ -21,6 +21,7 @@
 #import "ios/clean/chrome/browser/ui/tab/tab_coordinator.h"
 #import "ios/clean/chrome/browser/ui/tab_grid/tab_grid_view_controller.h"
 #import "ios/shared/chrome/browser/coordinator_context/coordinator_context.h"
+#import "ios/shared/chrome/browser/tabs/web_state_list.h"
 #import "ios/web/public/navigation_manager.h"
 #include "ios/web/public/web_state/web_state.h"
 #import "net/base/mac/url_conversions.h"
@@ -33,13 +34,10 @@
 @interface TabGridCoordinator ()<TabGridDataSource,
                                  SettingsCommands,
                                  TabCommands,
-                                 TabGridCommands> {
-  std::vector<std::unique_ptr<web::WebState>> _webStates;
-  size_t _activeWebStateIndex;
-}
-
+                                 TabGridCommands>
 @property(nonatomic, strong) TabGridViewController* viewController;
 @property(nonatomic, weak) SettingsCoordinator* settingsCoordinator;
+@property(readonly) WebStateList& webStateList;

[sdefresne, 2017/03/14 09:04:08]

Need to be "nonatomic" (as "atomic" is the default).

[edchin, 2017/03/14 16:02:42]

Atomicity should only be relevant when you allow the compiler to generate
getters and setters, since the compiler will insert locks for atomic properties.
But since we override the getter, and disable the setter, nonatomic has no
meaning here.

[sdefresne, 2017/03/14 16:34:34]

I know, however the style guide requires the properties to be marked as
nonatomic even if they are generated (and this is how we use the annotation
everywhere in the code):

  > Be aware of the overhead of properties. By default, all synthesized setters
  > and getters are atomic. This gives each set and get calls a substantial
  > amount of synchronization overhead. Declare your properties nonatomic
  > unless you require atomicity.

[edchin, 2017/03/14 16:50:38]

Done.

 @end
 
 @implementation TabGridCoordinator
@@ -55,9 +53,13 @@
     web::WebState::CreateParams webStateCreateParams(browser->browser_state());
     std::unique_ptr<web::WebState> webState =
         web::WebState::Create(webStateCreateParams);
-    _webStates.push_back(std::move(webState));
+    self.webStateList.InsertWebState(0, webState.release(), nullptr);
   }
-  _activeWebStateIndex = 0;
+  self.webStateList.ActivateWebStateAt(0);
+}
+
+- (WebStateList&)webStateList {
+  return self.browser->web_state_list();
 }
 
 #pragma mark - BrowserCoordinator
@@ -80,15 +82,12 @@
 
 #pragma mark - TabGridDataSource
 
-- (NSUInteger)numberOfTabsInTabGrid {
-  return static_cast<NSUInteger>(_webStates.size());
+- (int)numberOfTabsInTabGrid {
+  return self.webStateList.count();
 }
 
-- (NSString*)titleAtIndex:(NSInteger)index {
-  size_t i = static_cast<size_t>(index);
-  DCHECK(i < _webStates.size());
-  web::WebState* webState = _webStates[i].get();
-  GURL url = webState->GetVisibleURL();
+- (NSString*)titleAtIndex:(int)index {
+  GURL url = self.webStateList.GetWebStateAt(index)->GetVisibleURL();
   NSString* urlText = @"<New Tab>";
   if (url.is_valid()) {
     urlText = base::SysUTF8ToNSString(url.spec());
@@ -96,27 +95,28 @@
   return urlText;
 }
 
-- (NSInteger)indexOfActiveTab {
-  return static_cast<NSInteger>(_activeWebStateIndex);
+- (int)indexOfActiveTab {
+  return self.webStateList.active_index();
 }
 
 #pragma mark - TabCommands
 
 - (void)showTabAtIndexPath:(NSIndexPath*)indexPath {
   TabCoordinator* tabCoordinator = [[TabCoordinator alloc] init];
-  size_t index = static_cast<size_t>(indexPath.item);
-  DCHECK(index < _webStates.size());
-  tabCoordinator.webState = _webStates[index].get();
+  DCHECK_LE(indexPath.item, INT_MAX);
+  int index = static_cast<int>(indexPath.item);
+  tabCoordinator.webState = self.webStateList.GetWebStateAt(index);

[rohitrao (ping after 24h), 2017/03/14 11:41:23]

This can be in a followup CL, but we should remove the webState property from
TabCoordinator, and instead have it configure itself based on
self.browser->web_state_list().

[edchin, 2017/03/14 16:02:42]

Done. Added a PLACEHOLDER note.

   tabCoordinator.presentationKey = indexPath;
   [self addChildCoordinator:tabCoordinator];
   [tabCoordinator start];
-  _activeWebStateIndex = index;
+  self.webStateList.ActivateWebStateAt(index);

[rohitrao (ping after 24h), 2017/03/14 11:41:23]

Can we activate the new webstate before calling addChildCoordinator or start, so
that the TabCoordinator sees the new state of the world?  Does that cause any
other issues?

[edchin, 2017/03/14 16:02:42]

Done. No other issues caused as far as I can tell.

 }
 
 - (void)closeTabAtIndexPath:(NSIndexPath*)indexPath {
-  size_t index = static_cast<size_t>(indexPath.item);
-  DCHECK(index < _webStates.size());
-  _webStates.erase(_webStates.begin() + index);
+  DCHECK_LE(indexPath.item, INT_MAX);
+  int index = static_cast<int>(indexPath.item);
+  std::unique_ptr<web::WebState> closedWebState(
+      self.webStateList.DetachWebStateAt(index));
 }
 
 - (void)createNewTabAtIndexPath:(NSIndexPath*)indexPath {
@@ -124,7 +124,8 @@
       self.browser->browser_state());
   std::unique_ptr<web::WebState> webState =
       web::WebState::Create(webStateCreateParams);
-  _webStates.push_back(std::move(webState));
+  self.webStateList.InsertWebState(self.webStateList.count(),
+                                   webState.release(), nullptr);
 }
 
 #pragma mark - TabGridCommands
@@ -159,15 +160,15 @@
 - (void)openURL:(NSURL*)URL {
   [self.overlayCoordinator stop];
   [self removeOverlayCoordinator];
-  web::WebState* activeWebState = _webStates[_activeWebStateIndex].get();
+  web::WebState* activeWebState = self.webStateList.GetActiveWebState();

[sdefresne, 2017/03/14 09:04:08]

WebStateList.GetActiveWebState() may return null if there are no active
WebStates (i.e. if the WebStateList is empty or not WebState is selected). I
think you should add a DCHECK that there is an active WebState (or support the
case when there are none).

  DCHECK_NE(WebStateList::kInvalidIndex, self.webStateList.active_index()); 

[edchin, 2017/03/14 16:02:42]

Done.

[edchin, 2017/03/14 16:50:38]

I just read the URLOpening protocol comments which say that this method should
fail silently. Instead of this DCHECK, I put in an early return.

   web::NavigationManager::WebLoadParams params(net::GURLWithNSURL(URL));
   params.transition_type = ui::PAGE_TRANSITION_LINK;
   activeWebState->GetNavigationManager()->LoadURLWithParams(params);
   if (!self.children.count) {
-    [self showTabAtIndexPath:[NSIndexPath
-                                 indexPathForItem:static_cast<NSUInteger>(
-                                                      _activeWebStateIndex)
-                                        inSection:0]];
+    int index = [self indexOfActiveTab];
+    DCHECK_LE(index, NSIntegerMax);
+    NSInteger item = static_cast<NSInteger>(index);
+    [self showTabAtIndexPath:[NSIndexPath indexPathForItem:item inSection:0]];
   }
 }