[Nav Experiment] Use NavigationManager::Restore in SessionStorageBuilder

Added a NavigationManagerImpl::SetPreviousItemIndex() API to support the
SessionStorageBuilder use case, so that CRWSessionController can be
completely hidden behind the NavigationManagerImpl interface.

Fixed TabModelTest's dummy session storage creation code; for empty
session, lastCommittedItemIndex should be -1. Without this, the DCHECKs
in LegacyNavigationManagerImpl::Restore() will fail. This was not a
problem before because SessionStorageBuilder::ExtractSessionState() did
not check input invariants.

Tweaked input DCHECKs in LegacyNavigationManagerImpl::Restore() to allow
empty session as valid input.

Bug: 734150
Change-Id: I3e773347a8e5798f23888f8b757ae026fb85f2f3
Reviewed-on: https://chromium-review.googlesource.com/695830
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506724}

ios/chrome/browser/tabs/tab_model_unittest.mm

@@ -127,6 +127,7 @@ void SetTabModel(TabModel* tab_model) {
     NSMutableArray<CRWSessionStorage*>* sessions = [NSMutableArray array];
     for (int i = 0; i < 3; i++) {
       CRWSessionStorage* session_storage = [[CRWSessionStorage alloc] init];
+      session_storage.lastCommittedItemIndex = -1;
       [sessions addObject:session_storage];
     }
     return base::scoped_nsobject<SessionWindowIOS>(

ios/web/navigation/crw_session_controller.h

@@ -30,7 +30,7 @@ struct Referrer;
 @interface CRWSessionController : NSObject
 
 @property(nonatomic, readonly, assign) NSInteger lastCommittedItemIndex;
-@property(nonatomic, readonly, assign) NSInteger previousItemIndex;
+@property(nonatomic, readwrite, assign) NSInteger previousItemIndex;
 // The index of the pending item if it is in |items|, or -1 if |pendingItem|
 // corresponds with a new navigation (created by addPendingItem:).
 @property(nonatomic, readwrite, assign) NSInteger pendingItemIndex;

ios/web/navigation/crw_session_controller.mm

@@ -60,11 +60,6 @@ @interface CRWSessionController () {
 // Redefine as readwrite.
 @property(nonatomic, readwrite, assign) NSInteger lastCommittedItemIndex;
 
-// Expose setters for serialization properties.  These are exposed in a category
-// in SessionStorageBuilder, and will be removed as ownership of
-// their backing ivars moves to NavigationManagerImpl.
-@property(nonatomic, readwrite, assign) NSInteger previousItemIndex;
-
 // Removes all items after lastCommittedItemIndex.
 - (void)clearForwardItems;
 // Discards the transient item, if any.

ios/web/navigation/legacy_navigation_manager_impl.h

@@ -50,6 +50,7 @@ class LegacyNavigationManagerImpl : public NavigationManagerImpl {
   void CommitPendingItem() override;
   int GetIndexForOffset(int offset) const override;
   int GetPreviousItemIndex() const override;
+  void SetPreviousItemIndex(int previous_item_index) override;
 
   // NavigationManager:
   BrowserState* GetBrowserState() const override;

ios/web/navigation/legacy_navigation_manager_impl.mm

@@ -136,7 +136,7 @@
 }
 
 int LegacyNavigationManagerImpl::GetItemCount() const {
-  return [session_controller_ items].size();
+  return session_controller_ ? [session_controller_ items].size() : 0;
 }
 
 NavigationItem* LegacyNavigationManagerImpl::GetItemAtIndex(
@@ -216,8 +216,8 @@
     int last_committed_item_index,
     std::vector<std::unique_ptr<NavigationItem>> items) {
   DCHECK(GetItemCount() == 0 && !GetPendingItem());
-  DCHECK_GE(last_committed_item_index, 0);
-  DCHECK_LT(static_cast<size_t>(last_committed_item_index), items.size());
+  DCHECK_LT(last_committed_item_index, static_cast<int>(items.size()));
+  DCHECK(items.empty() || last_committed_item_index >= 0);
   SetSessionController([[CRWSessionController alloc]
         initWithBrowserState:browser_state_
              navigationItems:std::move(items)
@@ -332,4 +332,9 @@
   return base::checked_cast<int>([session_controller_ previousItemIndex]);
 }
 
+void LegacyNavigationManagerImpl::SetPreviousItemIndex(
+    int previous_item_index) {
+  [session_controller_ setPreviousItemIndex:previous_item_index];
+}
+
 }  // namespace web

ios/web/navigation/navigation_manager_impl.h

@@ -111,6 +111,9 @@ class NavigationManagerImpl : public NavigationManager {
   // Returns the index of the previous item. Only used by SessionStorageBuilder.
   virtual int GetPreviousItemIndex() const = 0;
 
+  // Sets the index of the previous item. Only used by SessionStorageBuilder.
+  virtual void SetPreviousItemIndex(int previous_item_index) = 0;
+
   // Resets the transient url rewriter list.
   void RemoveTransientURLRewriters();
 

ios/web/navigation/navigation_manager_impl_unittest.mm

@@ -172,6 +172,21 @@ void SimulateGoToIndex(int index) {
   EXPECT_EQ(-1, navigation_manager()->GetPreviousItemIndex());
 }
 
+// Tests that the simpler setter SetPreviousItemIndex() updates the previous
+// item index without sanity check.
+TEST_P(NavigationManagerTest, SetPreviousItemIndex) {
+  EXPECT_EQ(-1, navigation_manager()->GetPreviousItemIndex());
+
+  navigation_manager()->SetPreviousItemIndex(0);
+  EXPECT_EQ(0, navigation_manager()->GetPreviousItemIndex());
+
+  navigation_manager()->SetPreviousItemIndex(1);
+  EXPECT_EQ(1, navigation_manager()->GetPreviousItemIndex());
+
+  navigation_manager()->SetPreviousItemIndex(-1);
+  EXPECT_EQ(-1, navigation_manager()->GetPreviousItemIndex());
+}
+
 // Tests that GetPendingItemIndex() returns -1 if there is no pending entry.
 TEST_P(NavigationManagerTest, GetPendingItemIndexWithoutPendingEntry) {
   navigation_manager()->AddPendingItem(

ios/web/navigation/session_storage_builder.mm

@@ -9,8 +9,6 @@
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #include "base/memory/ptr_util.h"
-#import "ios/web/navigation/crw_session_controller+private_constructors.h"
-#import "ios/web/navigation/crw_session_controller.h"
 #import "ios/web/navigation/legacy_navigation_manager_impl.h"
 #import "ios/web/navigation/navigation_item_impl.h"
 #import "ios/web/navigation/navigation_item_storage_builder.h"
@@ -25,14 +23,6 @@
 #error "This file requires ARC support."
 #endif
 
-// CRWSessionController's readonly properties redefined as readwrite.  These
-// will be removed and NavigationManagerImpl's ivars will be written directly
-// as this functionality moves from CRWSessionController to
-// NavigationManagerImpl;
-@interface CRWSessionController (ExposedForSerialization)
-@property(nonatomic, readwrite, assign) NSInteger previousItemIndex;
-@end
-
 namespace web {
 
 CRWSessionStorage* SessionStorageBuilder::BuildStorage(
@@ -82,15 +72,10 @@ @interface CRWSessionController (ExposedForSerialization)
         item_storage_builder.BuildNavigationItemImpl(item_storages[index]);
     items[index] = std::move(item_impl);
   }
-  NSUInteger last_committed_item_index = storage.lastCommittedItemIndex;
-  base::scoped_nsobject<CRWSessionController> session_controller(
-      [[CRWSessionController alloc]
-            initWithBrowserState:nullptr
-                 navigationItems:std::move(items)
-          lastCommittedItemIndex:last_committed_item_index]);
-  [session_controller setPreviousItemIndex:storage.previousItemIndex];
-
-  web_state->navigation_manager_->SetSessionController(session_controller);
+  web_state->navigation_manager_->Restore(storage.lastCommittedItemIndex,
+                                          std::move(items));
+  web_state->navigation_manager_->SetPreviousItemIndex(
+      storage.previousItemIndex);
 
   SessionCertificatePolicyCacheStorageBuilder cert_builder;
   std::unique_ptr<SessionCertificatePolicyCacheImpl> cert_policy_cache =

ios/web/navigation/wk_based_navigation_manager_impl.h

@@ -75,6 +75,7 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   int GetIndexForOffset(int offset) const override;
   // Returns the previous navigation item in the main frame.
   int GetPreviousItemIndex() const override;
+  void SetPreviousItemIndex(int previous_item_index) override;
 
   // NavigationManager:
   BrowserState* GetBrowserState() const override;

ios/web/navigation/wk_based_navigation_manager_impl.mm

@@ -190,6 +190,11 @@ void SetNavigationItemInWKItem(WKBackForwardListItem* wk_item,
   return previous_item_index_;
 }
 
+void WKBasedNavigationManagerImpl::SetPreviousItemIndex(
+    int previous_item_index) {
+  previous_item_index_ = previous_item_index;
+}
+
 BrowserState* WKBasedNavigationManagerImpl::GetBrowserState() const {
   return browser_state_;
 }