Reland [Nav Experiment] Associate NavigationItem with WKBackForwardList

This is a reland of 2c0e1ed7
Renamed CRWNavigationItemHolder's |-setNavigationItem| to
|-replaceNavigationItem| to fix xcode_clang error.

Original change's description:
> [Nav Experiment] Associate NavigationItem with WKBackForwardList.
> 
> Added WKBasedNavigationManagerTest for implementation-specific test
> cases. The majority of WKBasedNavigationManagerImpl can be tested
> using NavigationManagerTest. This will be enabled in a subsequent CL.
> 
> Bug: 734150
> Change-Id: Ief59f397a72fd5bb1404e94a58486fa3daa87780
> Reviewed-on: https://chromium-review.googlesource.com/580628
> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
> Reviewed-by: Eugene But <eugenebut@chromium.org>
> Commit-Queue: Danyao Wang <danyao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#490117}

Bug: 734150
Change-Id: I277c048478a9a0fa9aea2962d10b56059d84fc5b
Reviewed-on: https://chromium-review.googlesource.com/592273
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491130}

ios/web/BUILD.gn

@@ -61,6 +61,8 @@ source_set("web_arc") {
     "interstitials/web_interstitial_impl.h",
     "interstitials/web_interstitial_impl.mm",
     "load_committed_details.cc",
+    "navigation/crw_navigation_item_holder.h",
+    "navigation/crw_navigation_item_holder.mm",
     "navigation/crw_session_controller+private_constructors.h",
     "navigation/crw_session_controller.h",
     "navigation/crw_session_controller.mm",
@@ -523,6 +525,7 @@ source_set("ios_web_navigation_unittests") {
   ]
 
   sources = [
+    "navigation/crw_navigation_item_holder_unittest.mm",
     "navigation/crw_navigation_item_storage_unittest.mm",
     "navigation/crw_session_controller_unittest.mm",
     "navigation/crw_session_storage_unittest.mm",
@@ -533,6 +536,7 @@ source_set("ios_web_navigation_unittests") {
     "navigation/navigation_manager_util_unittest.mm",
     "navigation/nscoder_util_unittest.mm",
     "navigation/serializable_user_data_manager_impl_unittest.mm",
+    "navigation/wk_based_navigation_manager_impl_unittest.mm",
   ]
 }
 

ios/web/navigation/crw_navigation_item_holder.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_WEB_NAVIGATION_CRW_NAVIGATION_ITEM_HOLDER_H_
+#define IOS_WEB_NAVIGATION_CRW_NAVIGATION_ITEM_HOLDER_H_
+
+#import <WebKit/WebKit.h>
+
+#include <memory>
+
+NS_ASSUME_NONNULL_BEGIN
+
+namespace web {
+class NavigationItemImpl;
+}  // namespace web
+
+// An NSObject wrapper for an std::unique_ptr<NavigationItemImpl>. This object
+// is used to associate a NavigationItemImpl to a WKBackForwardListItem to store
+// additional navigation states needed by the embedder.
+@interface CRWNavigationItemHolder : NSObject
+
+// Returns the CRWNavigationItemHolder object associated with |item|. Creates an
+// empty holder if none currently exists for |item|.
++ (instancetype)holderForBackForwardListItem:(WKBackForwardListItem*)item;
+
+- (instancetype)init NS_UNAVAILABLE;
+
+// Designated init method that associates the new instance with |item|.
+- (instancetype)initWithBackForwardListItem:(WKBackForwardListItem*)item;
+
+// Returns the NavigationItemImpl stored in this instance.
+- (web::NavigationItemImpl*)navigationItem;
+
+// Stores the NavigationItemImpl object in this instance.
+- (void)setNavigationItem:
+    (std::unique_ptr<web::NavigationItemImpl>)navigationItem;
+
+@end
+
+NS_ASSUME_NONNULL_END
+
+#endif  // IOS_WEB_NAVIGATION_CRW_NAVIGATION_ITEM_HOLDER_H_

ios/web/navigation/crw_navigation_item_holder.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/web/navigation/crw_navigation_item_holder.h"
+
+#import <objc/runtime.h>
+
+#import "ios/web/navigation/navigation_item_impl.h"
+
+// The address of this static variable is used to set and get the associated
+// NavigationItemImpl object from a WKBackForwardListItem.
+const void* kNavigationItemKey = &kNavigationItemKey;
+
+@implementation CRWNavigationItemHolder {
+  std::unique_ptr<web::NavigationItemImpl> _navigationItem;
+}
+
++ (instancetype)holderForBackForwardListItem:(WKBackForwardListItem*)item {
+  DCHECK(item);
+  CRWNavigationItemHolder* holder =
+      objc_getAssociatedObject(item, &kNavigationItemKey);
+  if (!holder) {
+    holder = [[CRWNavigationItemHolder alloc] initWithBackForwardListItem:item];
+  }
+  return holder;
+}
+
+- (instancetype)initWithBackForwardListItem:(WKBackForwardListItem*)item {
+  self = [super init];
+  if (self) {
+    objc_setAssociatedObject(item, &kNavigationItemKey, self,
+                             OBJC_ASSOCIATION_RETAIN);
+  }
+  return self;
+}
+
+- (web::NavigationItemImpl*)navigationItem {
+  return _navigationItem.get();
+}
+
+- (void)setNavigationItem:
+    (std::unique_ptr<web::NavigationItemImpl>)navigationItem {
+  _navigationItem = std::move(navigationItem);
+}
+
+@end

ios/web/navigation/crw_navigation_item_holder_unittest.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/web/navigation/crw_navigation_item_holder.h"
+
+#include "base/memory/ptr_util.h"
+#import "ios/web/navigation/navigation_item_impl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#import "testing/gtest_mac.h"
+#include "testing/platform_test.h"
+#include "third_party/ocmock/OCMock/OCMock.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+// Test fixture for CRWNavigationItemHolder.
+typedef PlatformTest CRWNavigationItemHolderTest;
+
+TEST_F(CRWNavigationItemHolderTest, NewHolderCreatedAutomatically) {
+  WKBackForwardListItem* item = OCMClassMock([WKBackForwardListItem class]);
+  CRWNavigationItemHolder* holder =
+      [CRWNavigationItemHolder holderForBackForwardListItem:item];
+  ASSERT_NSNE(holder, nil);
+  EXPECT_TRUE([holder navigationItem] == nullptr);
+}
+
+// Tests that stored NavigationItemImpl can be retrieved.
+TEST_F(CRWNavigationItemHolderTest, SetNavigationItem) {
+  GURL url("http://www.0.com");
+  auto navigation_item = base::MakeUnique<web::NavigationItemImpl>();
+  navigation_item->SetURL(url);
+
+  WKBackForwardListItem* item = OCMClassMock([WKBackForwardListItem class]);
+  [[CRWNavigationItemHolder holderForBackForwardListItem:item]
+      setNavigationItem:std::move(navigation_item)];
+
+  CRWNavigationItemHolder* holder =
+      [CRWNavigationItemHolder holderForBackForwardListItem:item];
+
+  ASSERT_NSNE(holder, nil);
+  ASSERT_TRUE([holder navigationItem] != nullptr);
+  EXPECT_EQ(url, [holder navigationItem]->GetURL());
+}
+
+// Tests that each WKBackForwardListItem has its unique CRWNavigationItemHolder.
+TEST_F(CRWNavigationItemHolderTest, OneHolderPerWKItem) {
+  GURL url1("http://www.1.com");
+  auto navigation_item1 = base::MakeUnique<web::NavigationItemImpl>();
+  navigation_item1->SetURL(url1);
+  WKBackForwardListItem* item1 = OCMClassMock([WKBackForwardListItem class]);
+  [[CRWNavigationItemHolder holderForBackForwardListItem:item1]
+      setNavigationItem:std::move(navigation_item1)];
+
+  GURL url2("http://www.2.com");
+  auto navigation_item2 = base::MakeUnique<web::NavigationItemImpl>();
+  navigation_item2->SetURL(url2);
+  WKBackForwardListItem* item2 = OCMClassMock([WKBackForwardListItem class]);
+  [[CRWNavigationItemHolder holderForBackForwardListItem:item2]
+      setNavigationItem:std::move(navigation_item2)];
+
+  ASSERT_NSNE(item1, item2);
+
+  CRWNavigationItemHolder* holder1 =
+      [CRWNavigationItemHolder holderForBackForwardListItem:item1];
+  CRWNavigationItemHolder* holder2 =
+      [CRWNavigationItemHolder holderForBackForwardListItem:item2];
+
+  EXPECT_NSNE(holder1, holder2);
+  ASSERT_TRUE(holder1.navigationItem);
+  EXPECT_EQ(url1, holder1.navigationItem->GetURL());
+  ASSERT_TRUE(holder2.navigationItem);
+  EXPECT_EQ(url2, holder2.navigationItem->GetURL());
+}

ios/web/navigation/legacy_navigation_manager_impl.mm

@@ -75,7 +75,7 @@ void LegacyNavigationManagerImpl::OnNavigationItemCommitted() {
   if (details.previous_item_index >= 0) {
     DCHECK([session_controller_ previousItem]);
     details.previous_url = [session_controller_ previousItem]->GetURL();
-    details.is_in_page = AreUrlsFragmentChangeNavigation(
+    details.is_in_page = IsFragmentChangeNavigationBetweenUrls(
         details.previous_url, details.item->GetURL());
   } else {
     details.previous_url = GURL();
@@ -115,46 +115,13 @@ void LegacyNavigationManagerImpl::AddPendingItem(
                        initiationType:initiation_type
               userAgentOverrideOption:user_agent_override_option];
 
-  // Set the user agent type for web URLs.
-  NavigationItem* pending_item = GetPendingItem();
-  if (!pending_item)
+  if (!GetPendingItem()) {
     return;
-
-  // |user_agent_override_option| must be INHERIT if |pending_item|'s
-  // UserAgentType is NONE, as requesting a desktop or mobile user agent should
-  // be disabled for app-specific URLs.
-  DCHECK(pending_item->GetUserAgentType() != UserAgentType::NONE ||
-         user_agent_override_option == UserAgentOverrideOption::INHERIT);
-
-  // Newly created pending items are created with UserAgentType::NONE for native
-  // pages or UserAgentType::MOBILE for non-native pages.  If the pending item's
-  // URL is non-native, check which user agent type it should be created with
-  // based on |user_agent_override_option|.
-  DCHECK_NE(UserAgentType::DESKTOP, pending_item->GetUserAgentType());
-  if (pending_item->GetUserAgentType() == UserAgentType::NONE)
-    return;
-
-  switch (user_agent_override_option) {
-    case UserAgentOverrideOption::DESKTOP:
-      pending_item->SetUserAgentType(UserAgentType::DESKTOP);
-      break;
-    case UserAgentOverrideOption::MOBILE:
-      pending_item->SetUserAgentType(UserAgentType::MOBILE);
-      break;
-    case UserAgentOverrideOption::INHERIT: {
-      // Propagate the last committed non-native item's UserAgentType if there
-      // is one, otherwise keep the default value, which is mobile.
-      NavigationItem* last_non_native_item =
-          GetLastCommittedNonAppSpecificItem();
-      DCHECK(!last_non_native_item ||
-             last_non_native_item->GetUserAgentType() != UserAgentType::NONE);
-      if (last_non_native_item) {
-        pending_item->SetUserAgentType(
-            last_non_native_item->GetUserAgentType());
-      }
-      break;
-    }
   }
+
+  UpdatePendingItemUserAgentType(user_agent_override_option,
+                                 GetLastCommittedNonAppSpecificItem(),
+                                 GetPendingItem());
 }
 
 void LegacyNavigationManagerImpl::CommitPendingItem() {

ios/web/navigation/navigation_manager_impl.h

@@ -143,17 +143,23 @@ class NavigationManagerImpl : public NavigationManager {
   // TODO(crbug.com/738020): Remove legacy code and merge
   // WKBasedNavigationManager into this class after the navigation experiment.
 
+  // Checks whether or not two URLs differ only in the fragment.
+  static bool IsFragmentChangeNavigationBetweenUrls(const GURL& existing_url,
+                                                    const GURL& new_url);
+
+  // Applies the user agent override to |pending_item|, or inherits the user
+  // agent of |inherit_from| if |user_agent_override_option| is INHERIT.
+  static void UpdatePendingItemUserAgentType(
+      UserAgentOverrideOption override_option,
+      const NavigationItem* inherit_from,
+      NavigationItem* pending_item);
+
   // Identical to GetItemAtIndex() but returns the underlying NavigationItemImpl
   // instead of the public NavigationItem interface. This is used by
   // SessionStorageBuilder to persist session state.
   virtual NavigationItemImpl* GetNavigationItemImplAtIndex(
       size_t index) const = 0;
 
-  // Checks whether or not two URL are an in-page navigation (differing only
-  // in the fragment).
-  static bool AreUrlsFragmentChangeNavigation(const GURL& existing_url,
-                                              const GURL& new_url);
-
   // The primary delegate for this manager.
   NavigationManagerDelegate* delegate_;
 

ios/web/navigation/navigation_manager_impl.mm

@@ -47,15 +47,58 @@ NavigationManager::WebLoadParams& NavigationManager::WebLoadParams::operator=(
 }
 
 /* static */
-bool NavigationManagerImpl::AreUrlsFragmentChangeNavigation(
+bool NavigationManagerImpl::IsFragmentChangeNavigationBetweenUrls(
     const GURL& existing_url,
     const GURL& new_url) {
+  // TODO(crbug.com/749542): Current implementation incorrectly returns false
+  // if URL changes from http://google.com#foo to http://google.com.
   if (existing_url == new_url || !new_url.has_ref())
     return false;
 
   return existing_url.EqualsIgnoringRef(new_url);
 }
 
+/* static */
+void NavigationManagerImpl::UpdatePendingItemUserAgentType(
+    UserAgentOverrideOption user_agent_override_option,
+    const NavigationItem* inherit_from_item,
+    NavigationItem* pending_item) {
+  DCHECK(pending_item);
+
+  // |user_agent_override_option| must be INHERIT if |pending_item|'s
+  // UserAgentType is NONE, as requesting a desktop or mobile user agent should
+  // be disabled for app-specific URLs.
+  DCHECK(pending_item->GetUserAgentType() != UserAgentType::NONE ||
+         user_agent_override_option == UserAgentOverrideOption::INHERIT);
+
+  // Newly created pending items are created with UserAgentType::NONE for native
+  // pages or UserAgentType::MOBILE for non-native pages.  If the pending item's
+  // URL is non-native, check which user agent type it should be created with
+  // based on |user_agent_override_option|.
+  DCHECK_NE(UserAgentType::DESKTOP, pending_item->GetUserAgentType());
+  if (pending_item->GetUserAgentType() == UserAgentType::NONE)
+    return;
+
+  switch (user_agent_override_option) {
+    case UserAgentOverrideOption::DESKTOP:
+      pending_item->SetUserAgentType(UserAgentType::DESKTOP);
+      break;
+    case UserAgentOverrideOption::MOBILE:
+      pending_item->SetUserAgentType(UserAgentType::MOBILE);
+      break;
+    case UserAgentOverrideOption::INHERIT: {
+      // Propagate the last committed non-native item's UserAgentType if there
+      // is one, otherwise keep the default value, which is mobile.
+      DCHECK(!inherit_from_item ||
+             inherit_from_item->GetUserAgentType() != UserAgentType::NONE);
+      if (inherit_from_item) {
+        pending_item->SetUserAgentType(inherit_from_item->GetUserAgentType());
+      }
+      break;
+    }
+  }
+}
+
 NavigationManagerImpl::NavigationManagerImpl()
     : delegate_(nullptr), browser_state_(nullptr) {}
 

ios/web/navigation/wk_based_navigation_manager_impl.h

@@ -14,10 +14,13 @@
 #include "base/macros.h"
 #import "ios/web/navigation/navigation_item_impl.h"
 #import "ios/web/navigation/navigation_manager_impl.h"
+#include "ios/web/navigation/time_smoother.h"
 #include "ios/web/public/reload_type.h"
 #include "ui/base/page_transition_types.h"
 #include "url/gurl.h"
 
+@class WKBackForwardListItem;
+
 namespace web {
 class BrowserState;
 class NavigationItem;
@@ -27,12 +30,28 @@ class SessionStorageBuilder;
 // WKBackForwardList based implementation of NavigationManagerImpl.
 // This class relies on the following WKWebView APIs, defined by the
 // CRWWebViewNavigationProxy protocol:
+//   @property URL
 //   @property backForwardList
 //   @property canGoBack
 //   @property canGoForward
 //   - goBack
 //   - goForward
 //   - goToBackForwardListItem:
+//
+// This navigation manager uses WKBackForwardList as the ground truth for back-
+// forward navigation history. It uses the Associated Objects runtime feature
+// to link a NavigationItemImpl object to each WKBackForwardListItem to store
+// additional states needed by the embedder.
+//
+// For all main frame navigations (both UI-initiated and renderer-initiated),
+// the NavigationItemImpl objects are created proactively via AddPendingItem and
+// CommitPendingItem.
+//
+// Non-main-frame navigations can only be initiated from the renderer. The
+// NavigationItemImpl objects in this case are created lazily in GetItemAtIndex
+// because the provisional load and commit events for iframe navigation are not
+// visible via the WKNavigationDelegate interface. Consequently, pending item
+// and previous item are only tracked for the main frame.
 class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
  public:
   WKBasedNavigationManagerImpl();
@@ -56,6 +75,7 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
       UserAgentOverrideOption user_agent_override_option) override;
   void CommitPendingItem() override;
   int GetIndexForOffset(int offset) const override;
+  // Returns the previous navigation item in the main frame.
   int GetPreviousItemIndex() const override;
 
   // NavigationManager:
@@ -63,6 +83,7 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   WebState* GetWebState() const override;
   NavigationItem* GetVisibleItem() const override;
   NavigationItem* GetLastCommittedItem() const override;
+  // Returns the pending navigation item in the main frame.
   NavigationItem* GetPendingItem() const override;
   NavigationItem* GetTransientItem() const override;
   void DiscardNonCommittedItems() override;
@@ -92,9 +113,40 @@ class WKBasedNavigationManagerImpl : public NavigationManagerImpl {
   // NavigationManagerImpl methods used by SessionStorageBuilder.
   NavigationItemImpl* GetNavigationItemImplAtIndex(size_t index) 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.
+  std::unique_ptr<NavigationItemImpl> pending_item_;
+
+  // -1 if pending_item_ represents a new navigation or there is no pending
+  // navigation. Otherwise, this is the index of the pending_item in the
+  // back-forward list.
+  int pending_item_index_;
+
+  // Index of the previous navigation item in the main frame. If there is none,
+  // this field will have value -1.
+  int previous_item_index_;
+
+  // Index of the last committed item in the main frame. If there is none, this
+  // field will equal to -1.
+  int last_committed_item_index_;
 
+  // The transient item in main frame.
+  std::unique_ptr<NavigationItemImpl> transient_item_;
 
-  // List of transient url rewriters added by |AddTransientURLRewriter()|.
+  // Time smoother for navigation item timestamps. See comment in
+  // navigation_controller_impl.h.
+  TimeSmoother time_smoother_;
 
   DISALLOW_COPY_AND_ASSIGN(WKBasedNavigationManagerImpl);
 };

ios/web/web_state/ui/crw_web_view_navigation_proxy.h

@@ -12,6 +12,7 @@ NS_ASSUME_NONNULL_BEGIN
 // A protocol to expose a subset of the WKWebView API to NavigationManager.
 @protocol CRWWebViewNavigationProxy
 
+@property(nullable, nonatomic, readonly, copy) NSURL* URL;
 @property(nonatomic, readonly, strong) WKBackForwardList* backForwardList;
 @property(nonatomic, readonly) BOOL canGoBack;
 @property(nonatomic, readonly) BOOL canGoForward;