[Nav Experiment] Cache visible item in nav manager during session restore

TabGridMediator relies on VisibleURL to determine if the title of a
tab should be hidden. During session restore, the restore_session.html
item is briefly the only visible item in the web view until the other
entries are restored, and it has chrome://newtab as the virtual URL.
This incorrectly causes the CreateItem() in TabGridMediator to set
|hidesTitle| to true for the grid item.

Previously visible item title was cached in WebStateImpl for a similar
reason (http://crbug.com/819606). This change combines the two use cases.


Bug: 869351
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I5391eda466ad9de26087d2d8a1e2a97a7a83f27a
Reviewed-on: https://chromium-review.googlesource.com/1157866
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581946}

ios/chrome/browser/voice/voice_search_navigations_tab_helper.mm

@@ -55,7 +55,6 @@ bool VoiceSearchNavigationTabHelper::IsNavigationFromVoiceSearch(
   if (item && (item == pending_item || item == transient_item))
     return will_navigate_to_voice_search_result_;
   // Check if the marker exists if it's a committed navigation.
-  DCHECK_NE(manager->GetIndexOfItem(item), -1);
   return item->GetUserData(kNavigationMarkerKey) != nullptr;
 }
 

ios/web/navigation/wk_based_navigation_manager_impl.h

@@ -238,6 +238,17 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
 
   WKWebViewCache web_view_cache_;
 
+  // Whether this navigation manager is in the process of restoring session
+  // history into WKWebView. It is set in Restore() and unset in the first
+  // OnNavigationItemCommitted() callback.
+  bool is_restore_session_in_progress_ = false;
+
+  // The active navigation entry in the restored session. GetVisibleItem()
+  // returns this item when |is_restore_session_in_progress_| is true so that
+  // clients of this navigation manager gets sane values for visible title and
+  // URL.
+  std::unique_ptr<NavigationItem> restored_visible_item_;
+
   DISALLOW_COPY_AND_ASSIGN(WKBasedNavigationManagerImpl);
 };
 

ios/web/navigation/wk_based_navigation_manager_impl.mm

@@ -86,6 +86,12 @@ void WKBasedNavigationManagerImpl::OnNavigationItemCommitted() {
   LoadCommittedDetails details;
   details.item = GetLastCommittedItem();
   DCHECK(details.item);
+
+  if (!wk_navigation_util::IsRestoreSessionUrl(details.item->GetURL())) {
+    is_restore_session_in_progress_ = false;
+    restored_visible_item_.reset();
+  }
+
   details.previous_item_index = GetPreviousItemIndex();
   NavigationItem* previous_item = GetItemAtIndex(details.previous_item_index);
   details.is_in_page =
@@ -279,6 +285,9 @@ WebState* WKBasedNavigationManagerImpl::GetWebState() const {
 }
 
 NavigationItem* WKBasedNavigationManagerImpl::GetVisibleItem() const {
+  if (is_restore_session_in_progress_)
+    return restored_visible_item_.get();
+
   NavigationItem* transient_item = GetTransientItem();
   if (transient_item) {
     return transient_item;
@@ -319,9 +328,8 @@ NavigationItem* WKBasedNavigationManagerImpl::GetItemAtIndex(
 
 int WKBasedNavigationManagerImpl::GetIndexOfItem(
     const NavigationItem* item) const {
-  if (item == empty_window_open_item_.get()) {
+  if (item == empty_window_open_item_.get())
     return 0;
-  }
 
   for (size_t index = 0; index < web_view_cache_.GetBackForwardListItemCount();
        index++) {
@@ -467,6 +475,15 @@ void WKBasedNavigationManagerImpl::Restore(
   // This pending item will become the first item in the restored history.
   params.virtual_url = items[0]->GetVirtualURL();
 
+  // Ordering is important. Cache the visible item of the restored session
+  // before starting the new navigation, which may trigger client lookup of
+  // visible item. The visible item of the restored session is the last
+  // committed item, because a restored session has no pending or transient
+  // item.
+  is_restore_session_in_progress_ = true;
+  if (last_committed_item_index > -1)
+    restored_visible_item_ = std::move(items[last_committed_item_index]);
+
   LoadURLWithParams(params);
 }
 

ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm

@@ -620,6 +620,9 @@ TEST_F(WKBasedNavigationManagerTest, RestoreSessionWithHistory) {
       "{\"offset\":0,\"titles\":[\"Test Website 0\",\"\"],"
       "\"urls\":[\"http://www.0.com/\",\"http://www.1.com/\"]}",
       ExtractRestoredSession(pending_url));
+
+  // Check that cached visible item is returned.
+  EXPECT_EQ("http://www.1.com/", manager_->GetVisibleItem()->GetURL());
 }
 
 // Tests that restoring session replaces existing history in navigation manager.
@@ -661,8 +664,10 @@ TEST_F(WKBasedNavigationManagerTest, RestoreSessionResetsHistory) {
   EXPECT_TRUE(manager_->GetPendingItem() != nullptr);
 
   // Restores a fake session.
+  auto restored_item = std::make_unique<NavigationItemImpl>();
+  restored_item->SetURL(GURL("http://restored.com"));
   std::vector<std::unique_ptr<NavigationItem>> items;
-  items.push_back(std::make_unique<NavigationItemImpl>());
+  items.push_back(std::move(restored_item));
   manager_->Restore(0 /* last_committed_item_index */, std::move(items));
 
   // Check that last_committed_index, previous_item_index and pending_item_index
@@ -680,6 +685,9 @@ TEST_F(WKBasedNavigationManagerTest, RestoreSessionResetsHistory) {
   GURL pending_url = pending_item->GetURL();
   EXPECT_TRUE(pending_url.SchemeIsFile());
   EXPECT_EQ("restore_session.html", pending_url.ExtractFileName());
+
+  // Check that cached visible item is returned.
+  EXPECT_EQ("http://restored.com/", manager_->GetVisibleItem()->GetURL());
 }
 
 // Tests that Restore() accepts empty session history and performs no-op.
@@ -884,6 +892,7 @@ TEST_F(WKBasedNavigationManagerDetachedModeTest, Reload) {
       "\",\"http://www.1.com/\",\"http://www.2.com/\"]}",
       ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
   EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+  EXPECT_EQ(url1_, manager_->GetVisibleItem()->GetURL());
 }
 
 // Tests that GoToIndex from detached mode restores cached history with updated
@@ -898,6 +907,7 @@ TEST_F(WKBasedNavigationManagerDetachedModeTest, GoToIndex) {
       "\",\"http://www.1.com/\",\"http://www.2.com/\"]}",
       ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
   EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+  EXPECT_EQ(url0_, manager_->GetVisibleItem()->GetURL());
 }
 
 // Tests that LoadIfNecessary from detached mode restores cached history.
@@ -911,6 +921,7 @@ TEST_F(WKBasedNavigationManagerDetachedModeTest, LoadIfNecessary) {
       "\",\"http://www.1.com/\",\"http://www.2.com/\"]}",
       ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
   EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+  EXPECT_EQ(url1_, manager_->GetVisibleItem()->GetURL());
 }
 
 // Tests that LoadURLWithParams from detached mode restores backward history and
@@ -919,13 +930,15 @@ TEST_F(WKBasedNavigationManagerDetachedModeTest, LoadURLWithParams) {
   manager_->DetachFromWebView();
   delegate_.RemoveWebView();
 
-  NavigationManager::WebLoadParams params(GURL("http://www.3.com"));
+  GURL url("http://www.3.com");
+  NavigationManager::WebLoadParams params(url);
   manager_->LoadURLWithParams(params);
   EXPECT_EQ(
       "{\"offset\":0,\"titles\":[\"\",\"\",\"\"],\"urls\":[\"http://www.0.com/"
       "\",\"http://www.1.com/\",\"http://www.3.com/\"]}",
       ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
   EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+  EXPECT_EQ(url, manager_->GetVisibleItem()->GetURL());
 }
 
 }  // namespace web

ios/web/web_state/web_state_impl.h

@@ -377,9 +377,6 @@ class WebStateImpl : public WebState, public NavigationManagerDelegate {
   // The most recently restored session history that has not yet committed in
   // the WKWebView. This is reset in OnNavigationItemCommitted().
   CRWSessionStorage* restored_session_storage_;
-  // The title of the active navigation entry in |restored_session_storage_|.
-  // It is only valid when |restore_session_storage_| is not nil.
-  base::string16 restored_title_;
 
   // Favicons URLs received in OnFaviconUrlUpdated.
   // WebStateObserver:FaviconUrlUpdated must be called for same-document

ios/web/web_state/web_state_impl.mm

@@ -323,11 +323,6 @@ const base::string16& WebStateImpl::GetTitle() const {
   DCHECK(Configured());
   web::NavigationItem* item = navigation_manager_->GetLastCommittedItem();
   if (web::GetWebClient()->IsSlimNavigationManagerEnabled()) {
-    if (!restored_title_.empty()) {
-      DCHECK(!item);
-      return restored_title_;
-    }
-
     // Display title for the visible item makes more sense. Only do this in
     // WKBasedNavigationManager for now to limit impact.
     item = navigation_manager_->GetVisibleItem();
@@ -860,10 +855,8 @@ void WebStateImpl::OnNavigationItemCommitted(
   // A committed navigation item indicates that NavigationManager has a new
   // valid session history so should invalidate the cached restored session
   // history.
-  if (web::GetWebClient()->IsSlimNavigationManagerEnabled()) {
+  if (web::GetWebClient()->IsSlimNavigationManagerEnabled())
     restored_session_storage_ = nil;
-    restored_title_.clear();
-  }
   for (auto& observer : observers_)
     observer.NavigationItemCommitted(this, load_details);
 }
@@ -888,15 +881,8 @@ void WebStateImpl::RestoreSessionStorage(CRWSessionStorage* session_storage) {
   // happen to inactive tabs when a navigation in the current tab triggers the
   // serialization of all tabs and when user clicks on tab switcher without
   // switching to a tab.
-  if (web::GetWebClient()->IsSlimNavigationManagerEnabled()) {
+  if (web::GetWebClient()->IsSlimNavigationManagerEnabled())
     restored_session_storage_ = session_storage;
-    NSInteger index = session_storage.lastCommittedItemIndex;
-    if (index > -1) {
-      CRWNavigationItemStorage* item_storage =
-          session_storage.itemStorages[index];
-      restored_title_ = item_storage.title;
-    }
-  }
   SessionStorageBuilder session_storage_builder;
   session_storage_builder.ExtractSessionState(this, session_storage);
 }

ios/web/web_state/web_state_impl_unittest.mm

@@ -963,11 +963,13 @@ TEST_P(WebStateImplTest, UncommittedRestoreSession) {
   scoped_feature_list.InitAndEnableFeature(
       web::features::kSlimNavigationManager);
 
+  GURL url("http://test.com");
   CRWSessionStorage* session_storage = [[CRWSessionStorage alloc] init];
   session_storage.lastCommittedItemIndex = 0;
   CRWNavigationItemStorage* item_storage =
       [[CRWNavigationItemStorage alloc] init];
   item_storage.title = base::SysNSStringToUTF16(@"Title");
+  item_storage.virtualURL = url;
   session_storage.itemStorages = @[ item_storage ];
 
   web::WebState::CreateParams params(GetBrowserState());
@@ -978,6 +980,7 @@ TEST_P(WebStateImplTest, UncommittedRestoreSession) {
   EXPECT_EQ(0, extracted_session_storage.lastCommittedItemIndex);
   EXPECT_EQ(1U, extracted_session_storage.itemStorages.count);
   EXPECT_NSEQ(@"Title", base::SysUTF16ToNSString(web_state.GetTitle()));
+  EXPECT_EQ(url, web_state.GetVisibleURL());
 }
 
 TEST_P(WebStateImplTest, NoUncommittedRestoreSession) {
@@ -989,6 +992,7 @@ TEST_P(WebStateImplTest, NoUncommittedRestoreSession) {
   EXPECT_EQ(-1, session_storage.lastCommittedItemIndex);
   EXPECT_NSEQ(@[], session_storage.itemStorages);
   EXPECT_TRUE(web_state_->GetTitle().empty());
+  EXPECT_EQ(GURL::EmptyGURL(), web_state_->GetVisibleURL());
 }
 
 // Tests showing and clearing interstitial when NavigationManager is