[Nav Experiment] Add DetachFromWebView to WKBasedNavigationManagerImpl.

CRWWebController sometimes needs to remove WKWebView (e.g. crbug/770914)
and this obliterates the session history when WKBasedNavigationManager
is used. This CL adds the ability for WKBasedNavigationManager to
temporarily be detached from the web view.

Major parts to this CL:
1. NavigationManagerImpl::DetachFromWebView() is a new API that
   CRWWebController uses to notify the navigation manager before
   removing the web view.
2. WKBasedNavigationManager implements this API with the help of a new
   nested class WKWebViewShim. It knows how to cache the items when
   detaching from a web view and service getter calls in detached mode.
3. Added overrides for FinishReload(), FinishLoadURLWithParams() and
   LoadIfNecessary() in WKBasedNavigationManager so it can switch back
   to attached mode when a new navigation starts by restoring the cached
   navigation items.

Also cleaned up unneeded APIs from CRWWebViewNavigationProxy.

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

ios/web/navigation/navigation_manager_impl.h

@@ -83,6 +83,9 @@ class NavigationManagerImpl : public NavigationManager {
   virtual void OnNavigationItemChanged() = 0;
   virtual void OnNavigationItemCommitted() = 0;
 
+  // Prepares for the deletion of WKWebView such as caching necessary data.
+  virtual void DetachFromWebView();
+
   // Temporary accessors and content/ class pass-throughs.
   // TODO(stuartmorgan): Re-evaluate this list once the refactorings have
   // settled down.
@@ -171,7 +174,7 @@ class NavigationManagerImpl : public NavigationManager {
   void GoToIndex(int index) final;
   void Reload(ReloadType reload_type, bool check_for_reposts) final;
   void ReloadWithUserAgentType(UserAgentType user_agent_type) final;
-  void LoadIfNecessary() final;
+  void LoadIfNecessary() override;
 
   // Implementation for corresponding NavigationManager getters.
   virtual NavigationItemImpl* GetPendingItemImpl() const = 0;
@@ -222,6 +225,8 @@ class NavigationManagerImpl : public NavigationManager {
 
   // Subclass specific implementation to update session state.
   virtual void FinishGoToIndex(int index, NavigationInitiationType type) = 0;
+  virtual void FinishReload();
+  virtual void FinishLoadURLWithParams();
 
   // Returns true if the subclass uses placeholder URLs and this is such a URL.
   virtual bool IsPlaceholderUrl(const GURL& url) const;

ios/web/navigation/navigation_manager_impl.mm

@@ -130,6 +130,8 @@ void NavigationManagerImpl::SetBrowserState(BrowserState* browser_state) {
   browser_state_ = browser_state;
 }
 
+void NavigationManagerImpl::DetachFromWebView() {}
+
 void NavigationManagerImpl::RemoveTransientURLRewriters() {
   transient_url_rewriters_.clear();
 }
@@ -286,7 +288,7 @@ void NavigationManagerImpl::LoadURLWithParams(
     added_item->SetShouldSkipRepostFormConfirmation(true);
   }
 
-  delegate_->LoadCurrentItem();
+  FinishLoadURLWithParams();
 }
 
 void NavigationManagerImpl::AddTransientURLRewriter(
@@ -321,7 +323,7 @@ void NavigationManagerImpl::Reload(ReloadType reload_type,
     reload_item->SetURL(reload_item->GetOriginalRequestURL());
   }
 
-  delegate_->Reload();
+  FinishReload();
 }
 
 void NavigationManagerImpl::ReloadWithUserAgentType(
@@ -438,6 +440,14 @@ NavigationItem* NavigationManagerImpl::GetLastCommittedNonAppSpecificItem()
   return nullptr;
 }
 
+void NavigationManagerImpl::FinishReload() {
+  delegate_->Reload();
+}
+
+void NavigationManagerImpl::FinishLoadURLWithParams() {
+  delegate_->LoadCurrentItem();
+}
+
 bool NavigationManagerImpl::IsPlaceholderUrl(const GURL& url) const {
   return false;  // Default implementation doesn't use placeholder URLs
 }

ios/web/navigation/wk_based_navigation_manager_impl.h

@@ -31,10 +31,6 @@ class SessionStorageBuilder;
 // CRWWebViewNavigationProxy protocol:
 //   @property URL
 //   @property backForwardList
-//   @property canGoBack
-//   @property canGoForward
-//   - goBack
-//   - goForward
 //   - goToBackForwardListItem:
 //
 // This navigation manager uses WKBackForwardList as the ground truth for back-
@@ -67,6 +63,18 @@ class SessionStorageBuilder;
 //   single normal entry. The only difference is that a subsequent call to
 //   CommitPendingItem() will *replace* the empty window open item. From this
 //   point onward, it is as if the empty window open item never occurred.
+//
+// Detach from web view edge case:
+//
+//   As long as this navigation manager is alive, the navigation manager
+//   delegate should not delete its WKWebView. However, legacy use cases exist
+//   (e.g. https://crbug/770914). As a workaround, before deleting the
+//   WKWebView, the delegate must call
+//   NavigationManagerImpl::DetachFromWebView() to cache the current session
+//   history. This puts the navigation manager in a detached state. While in
+//   this state, all getters are serviced using the cached session history.
+//   Mutation methods are not allowed. The navigation manager returns to the
+//   attached state when a new navigation starts.
 class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
  public:
   WKBasedNavigationManagerImpl();
@@ -78,6 +86,7 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   void OnNavigationItemsPruned(size_t pruned_item_count) override;
   void OnNavigationItemChanged() override;
   void OnNavigationItemCommitted() override;
+  void DetachFromWebView() override;
   CRWSessionController* GetSessionController() const override;
   void AddTransientItem(const GURL& url) override;
   void AddPendingItem(
@@ -117,12 +126,68 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   bool CanPruneAllButLastCommittedItem() const override;
   void Restore(int last_committed_item_index,
                std::vector<std::unique_ptr<NavigationItem>> items) override;
+  void LoadIfNecessary() override;
 
  private:
   // The SessionStorageBuilder functions require access to private variables of
   // NavigationManagerImpl.
   friend SessionStorageBuilder;
 
+  // Access shim for NavigationItems associated with the WKBackForwardList. It
+  // is responsible for caching NavigationItems when the navigation manager
+  // detaches from its web view.
+  class WKWebViewCache {
+   public:
+    explicit WKWebViewCache(WKBasedNavigationManagerImpl* navigation_manager);
+    ~WKWebViewCache();
+
+    // Returns true if the navigation manager is attached to a WKWebView.
+    bool IsAttachedToWebView() const;
+
+    // Caches NavigationItems from the WKWebView in |this| and changes state to
+    // detached.
+    void DetachFromWebView();
+
+    // Clears the cached NavigationItems and resets state to attached. Callers
+    // that wish to restore the cached navigation items into the new web view
+    // must call ReleaseCachedItems() first.
+    void ResetToAttached();
+
+    // Returns ownership of the cached NavigationItems. This is convenient for
+    // restoring session history when reattaching to a new web view.
+    std::vector<std::unique_ptr<NavigationItem>> ReleaseCachedItems();
+
+    // Returns the number of items in the back-forward history.
+    size_t GetBackForwardListItemCount() const;
+
+    // Returns the absolute index of WKBackForwardList's |currentItem| or -1 if
+    // |currentItem| is nil. If navigation manager is in detached mode, returns
+    // the cached value of this property captured at the last call of
+    // DetachFromWebView().
+    int GetCurrentItemIndex() const;
+
+    // Returns the NavigationItem associated with the WKBackForwardListItem at
+    // |index|. If |create_if_missing| is true and the WKBackForwardListItem
+    // does not have an associated NavigationItem, creates a new one and returns
+    // it to the caller.
+    NavigationItemImpl* GetNavigationItemImplAtIndex(
+        size_t index,
+        bool create_if_missing) const;
+
+    // Returns the WKBackForwardListItem at |index|. Must only be called when
+    // IsAttachedToWebView() is true.
+    WKBackForwardListItem* GetWKItemAtIndex(size_t index) const;
+
+   private:
+    WKBasedNavigationManagerImpl* navigation_manager_;
+    bool attached_to_web_view_;
+
+    std::vector<std::unique_ptr<NavigationItemImpl>> cached_items_;
+    int cached_current_item_index_;
+
+    DISALLOW_COPY_AND_ASSIGN(WKWebViewCache);
+  };
+
   // NavigationManagerImpl:
   NavigationItemImpl* GetNavigationItemImplAtIndex(size_t index) const override;
   NavigationItemImpl* GetLastCommittedItemImpl() const override;
@@ -131,18 +196,10 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   NavigationItemImpl* GetTransientItemImpl() const override;
   void FinishGoToIndex(int index,
                        NavigationInitiationType initiation_type) override;
+  void FinishReload() override;
+  void FinishLoadURLWithParams() override;
   bool IsPlaceholderUrl(const GURL& url) const override;
 
-  // Returns the absolute index of WKBackForwardList's |currentItem|. Returns -1
-  // if |currentItem| is nil.
-  int GetWKCurrentItemIndex() const;
-
-  // Returns the WKNavigationItem object at the absolute index, where index = 0
-  // corresponds to the oldest entry in the back-forward history, and
-  // index = GetItemCount() - 1 corresponds to the newest entry in the back-
-  // forward history. Returns nil if |index| is outside [0, GetItemCount()).
-  WKBackForwardListItem* GetWKItemAtIndex(int index) const;
-
   // The pending main frame navigation item. This is nullptr if there is no
   // pending item or if the pending item is a back-forward navigation, in which
   // case the NavigationItemImpl is stored on the WKBackForwardListItem.
@@ -178,6 +235,8 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   // have to be lazily created on read, this is the only workaround.
   mutable TimeSmoother time_smoother_;
 
+  WKWebViewCache web_view_cache_;
+
   DISALLOW_COPY_AND_ASSIGN(WKBasedNavigationManagerImpl);
 };
 

ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm

@@ -15,6 +15,7 @@
 #import "ios/web/navigation/wk_navigation_util.h"
 #include "ios/web/public/load_committed_details.h"
 #include "ios/web/public/navigation_item.h"
+#include "ios/web/public/reload_type.h"
 #include "ios/web/public/test/fakes/test_browser_state.h"
 #import "ios/web/public/web_client.h"
 #import "ios/web/test/fakes/crw_fake_back_forward_list.h"
@@ -788,4 +789,148 @@ TEST_F(WKBasedNavigationManagerTest, EmptyWindowOpenNavigation) {
   manager_->GoToIndex(0);
 }
 
+// Test fixture for detach from web view mode for WKBasedNavigationManagerImpl.
+class WKBasedNavigationManagerDetachedModeTest
+    : public WKBasedNavigationManagerTest {
+ protected:
+  void SetUp() override {
+    // Sets up each test case with a session history of 3 items. The middle item
+    // is the current item.
+    url0_ = GURL("http://www.0.com");
+    url1_ = GURL("http://www.1.com");
+    url2_ = GURL("http://www.2.com");
+
+    [mock_wk_list_ setCurrentURL:@"http://www.1.com"
+                    backListURLs:@[ @"http://www.0.com" ]
+                 forwardListURLs:@[ @"http://www.2.com" ]];
+
+    ASSERT_EQ(url0_, manager_->GetItemAtIndex(0)->GetURL());
+    ASSERT_EQ(url1_, manager_->GetItemAtIndex(1)->GetURL());
+    ASSERT_EQ(url2_, manager_->GetItemAtIndex(2)->GetURL());
+  }
+
+  // Returns the value of the "?session" URL param from |url|.
+  static std::string ExtractRestoredSession(const GURL& url) {
+    std::string session_json;
+    net::GetValueForKeyInQuery(
+        url, wk_navigation_util::kRestoreSessionSessionQueryKey, &session_json);
+    return session_json;
+  }
+
+  GURL url0_;
+  GURL url1_;
+  GURL url2_;
+};
+
+// Tests that all getters return the expected value in detached mode.
+TEST_F(WKBasedNavigationManagerDetachedModeTest, CachedSessionHistory) {
+  manager_->DetachFromWebView();
+  delegate_.RemoveWebView();
+
+  EXPECT_EQ(url1_, manager_->GetVisibleItem()->GetURL());
+  EXPECT_EQ(3, manager_->GetItemCount());
+
+  EXPECT_EQ(url0_, manager_->GetItemAtIndex(0)->GetURL());
+  EXPECT_EQ(url1_, manager_->GetItemAtIndex(1)->GetURL());
+  EXPECT_EQ(url2_, manager_->GetItemAtIndex(2)->GetURL());
+
+  EXPECT_EQ(0, manager_->GetIndexOfItem(manager_->GetItemAtIndex(0)));
+  EXPECT_EQ(1, manager_->GetIndexOfItem(manager_->GetItemAtIndex(1)));
+  EXPECT_EQ(2, manager_->GetIndexOfItem(manager_->GetItemAtIndex(2)));
+
+  EXPECT_EQ(-1, manager_->GetPendingItemIndex());
+  EXPECT_EQ(nullptr, manager_->GetPendingItem());
+
+  EXPECT_EQ(1, manager_->GetLastCommittedItemIndex());
+  EXPECT_EQ(url1_, manager_->GetLastCommittedItem()->GetURL());
+
+  EXPECT_TRUE(manager_->CanGoBack());
+  EXPECT_TRUE(manager_->CanGoForward());
+  EXPECT_TRUE(manager_->CanGoToOffset(0));
+  EXPECT_TRUE(manager_->CanGoToOffset(-1));
+  EXPECT_TRUE(manager_->CanGoToOffset(1));
+
+  EXPECT_EQ(0, manager_->GetIndexForOffset(-1));
+  EXPECT_EQ(1, manager_->GetIndexForOffset(0));
+  EXPECT_EQ(2, manager_->GetIndexForOffset(1));
+
+  NavigationItemList backward_items = manager_->GetBackwardItems();
+  EXPECT_EQ(1UL, backward_items.size());
+  EXPECT_EQ(url0_, backward_items[0]->GetURL());
+
+  NavigationItemList forward_items = manager_->GetForwardItems();
+  EXPECT_EQ(1UL, forward_items.size());
+  EXPECT_EQ(url2_, forward_items[0]->GetURL());
+}
+
+// Tests that detaching from an empty WKWebView works.
+TEST_F(WKBasedNavigationManagerDetachedModeTest, NothingToCache) {
+  delegate_.RemoveWebView();
+  manager_->DetachFromWebView();
+
+  EXPECT_EQ(0, manager_->GetItemCount());
+  EXPECT_EQ(nullptr, manager_->GetVisibleItem());
+  EXPECT_EQ(nullptr, manager_->GetItemAtIndex(0));
+  EXPECT_EQ(nullptr, manager_->GetPendingItem());
+  EXPECT_EQ(-1, manager_->GetLastCommittedItemIndex());
+
+  manager_->Reload(web::ReloadType::NORMAL, false /* check_for_repost */);
+  EXPECT_EQ(nullptr, manager_->GetPendingItem());
+}
+
+// Tests that Reload from detached mode restores cached history.
+TEST_F(WKBasedNavigationManagerDetachedModeTest, Reload) {
+  manager_->DetachFromWebView();
+  delegate_.RemoveWebView();
+
+  manager_->Reload(web::ReloadType::NORMAL, false /* check_for_repost */);
+  EXPECT_EQ(
+      "{\"offset\":-1,\"titles\":[\"\",\"\",\"\"],\"urls\":[\"http://www.0.com/"
+      "\",\"http://www.1.com/\",\"http://www.2.com/\"]}",
+      ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
+  EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+}
+
+// Tests that GoToIndex from detached mode restores cached history with updated
+// current item offset.
+TEST_F(WKBasedNavigationManagerDetachedModeTest, GoToIndex) {
+  manager_->DetachFromWebView();
+  delegate_.RemoveWebView();
+
+  manager_->GoToIndex(0);
+  EXPECT_EQ(
+      "{\"offset\":-2,\"titles\":[\"\",\"\",\"\"],\"urls\":[\"http://www.0.com/"
+      "\",\"http://www.1.com/\",\"http://www.2.com/\"]}",
+      ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
+  EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+}
+
+// Tests that LoadIfNecessary from detached mode restores cached history.
+TEST_F(WKBasedNavigationManagerDetachedModeTest, LoadIfNecessary) {
+  manager_->DetachFromWebView();
+  delegate_.RemoveWebView();
+
+  manager_->LoadIfNecessary();
+  EXPECT_EQ(
+      "{\"offset\":-1,\"titles\":[\"\",\"\",\"\"],\"urls\":[\"http://www.0.com/"
+      "\",\"http://www.1.com/\",\"http://www.2.com/\"]}",
+      ExtractRestoredSession(manager_->GetPendingItem()->GetURL()));
+  EXPECT_EQ(url0_, manager_->GetPendingItem()->GetVirtualURL());
+}
+
+// Tests that LoadURLWithParams from detached mode restores backward history and
+// adds the new item at the end.
+TEST_F(WKBasedNavigationManagerDetachedModeTest, LoadURLWithParams) {
+  manager_->DetachFromWebView();
+  delegate_.RemoveWebView();
+
+  NavigationManager::WebLoadParams params(GURL("http://www.3.com"));
+  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());
+}
+
 }  // namespace web

ios/web/web_state/ui/crw_web_controller.mm

@@ -3909,6 +3909,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
     return;
 
   _webStateImpl->CancelDialogs();
+  self.navigationManagerImpl->DetachFromWebView();
 
   [self abortLoad];
   [_webView removeFromSuperview];

ios/web/web_state/ui/crw_web_view_navigation_proxy.h

@@ -14,11 +14,7 @@ NS_ASSUME_NONNULL_BEGIN
 
 @property(nullable, nonatomic, readonly, copy) NSURL* URL;
 @property(nonatomic, readonly, strong) WKBackForwardList* backForwardList;
-@property(nonatomic, readonly) BOOL canGoBack;
-@property(nonatomic, readonly) BOOL canGoForward;
 
-- (nullable WKNavigation*)goBack;
-- (nullable WKNavigation*)goForward;
 - (nullable WKNavigation*)goToBackForwardListItem:(WKBackForwardListItem*)item;
 
 @end