[Nav Experiment] Only apply transient URL rewriters for pending item.

Transient URL rewriters do not make sense for lazily synced navigation
items. Refactoring them out of CreateNavigationItemWithRewriters allows
GetItemAtIndex() to maintain its const contract.

There is no behavior change for LegacyNavigationManager or
CRWSessionController. The new behavior only affects
WKBasedNavigationManager.

Bug: 734150
Change-Id: Id39ef8151b268f44b2179977da5cc8ace10fafbe
Reviewed-on: https://chromium-review.googlesource.com/590150
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@{#491144}

ios/web/navigation/navigation_manager_impl.h

@@ -123,8 +123,8 @@ class NavigationManagerImpl : public NavigationManager {
 
   // Creates a NavigationItem using the given properties. Calling this method
   // resets the transient URLRewriters cached in this instance.
-  // TODO(crbug.com/738020): Make this private when WKBasedNavigationManagerImpl
-  // is merged into this class.
+  // TODO(crbug.com/738020): This method is only used by CRWSessionController.
+  // Remove it after switching to WKBasedNavigationManagerImpl.
   std::unique_ptr<NavigationItemImpl> CreateNavigationItem(
       const GURL& url,
       const Referrer& referrer,
@@ -154,6 +154,18 @@ class NavigationManagerImpl : public NavigationManager {
       const NavigationItem* inherit_from,
       NavigationItem* pending_item);
 
+  // Creates a NavigationItem using the given properties. If |url_rewriters| is
+  // not nullptr, apply them before applying the permanent URL rewriters from
+  // BrowserState.
+  // TODO(crbug.com/738020): Make this private when WKBasedNavigationManagerImpl
+  // is merged into this class.
+  std::unique_ptr<NavigationItemImpl> CreateNavigationItemWithRewriters(
+      const GURL& url,
+      const Referrer& referrer,
+      ui::PageTransition transition,
+      NavigationInitiationType initiation_type,
+      const std::vector<BrowserURLRewriter::URLRewriter>* url_rewriters) const;
+
   // Identical to GetItemAtIndex() but returns the underlying NavigationItemImpl
   // instead of the public NavigationItem interface. This is used by
   // SessionStorageBuilder to persist session state.

ios/web/navigation/navigation_manager_impl.mm

@@ -121,46 +121,9 @@ std::unique_ptr<NavigationItemImpl> NavigationManagerImpl::CreateNavigationItem(
     const Referrer& referrer,
     ui::PageTransition transition,
     NavigationInitiationType initiation_type) {
-  GURL loaded_url(url);
-
-  bool url_was_rewritten = false;
-  if (!transient_url_rewriters_.empty()) {
-    url_was_rewritten = web::BrowserURLRewriter::RewriteURLWithWriters(
-        &loaded_url, browser_state_, transient_url_rewriters_);
-  }
-
-  if (!url_was_rewritten) {
-    web::BrowserURLRewriter::GetInstance()->RewriteURLIfNecessary(
-        &loaded_url, browser_state_);
-  }
-
-  if (initiation_type == web::NavigationInitiationType::RENDERER_INITIATED &&
-      loaded_url != url && web::GetWebClient()->IsAppSpecificURL(loaded_url)) {
-    const NavigationItem* last_committed_item = GetLastCommittedItem();
-    bool last_committed_url_is_app_specific =
-        last_committed_item &&
-        web::GetWebClient()->IsAppSpecificURL(last_committed_item->GetURL());
-    if (!last_committed_url_is_app_specific) {
-      // The URL should not be changed to app-specific URL if the load was
-      // renderer-initiated requested by non app-specific URL. Pages with
-      // app-specific urls have elevated previledges and should not be allowed
-      // to open app-specific URLs.
-      loaded_url = url;
-    }
-  }
-
+  auto item = CreateNavigationItemWithRewriters(
+      url, referrer, transition, initiation_type, &transient_url_rewriters_);
   RemoveTransientURLRewriters();
-
-  auto item = base::MakeUnique<NavigationItemImpl>();
-  item->SetOriginalRequestURL(loaded_url);
-  item->SetURL(loaded_url);
-  item->SetReferrer(referrer);
-  item->SetTransitionType(transition);
-  item->SetNavigationInitiationType(initiation_type);
-  if (web::GetWebClient()->IsAppSpecificURL(loaded_url)) {
-    item->SetUserAgentType(web::UserAgentType::NONE);
-  }
-
   return item;
 }
 
@@ -199,4 +162,53 @@ void NavigationManagerImpl::Reload(ReloadType reload_type,
   delegate_->Reload();
 }
 
+std::unique_ptr<NavigationItemImpl>
+NavigationManagerImpl::CreateNavigationItemWithRewriters(
+    const GURL& url,
+    const Referrer& referrer,
+    ui::PageTransition transition,
+    NavigationInitiationType initiation_type,
+    const std::vector<BrowserURLRewriter::URLRewriter>* additional_rewriters)
+    const {
+  GURL loaded_url(url);
+
+  bool url_was_rewritten = false;
+  if (additional_rewriters && !additional_rewriters->empty()) {
+    url_was_rewritten = web::BrowserURLRewriter::RewriteURLWithWriters(
+        &loaded_url, browser_state_, *additional_rewriters);
+  }
+
+  if (!url_was_rewritten) {
+    web::BrowserURLRewriter::GetInstance()->RewriteURLIfNecessary(
+        &loaded_url, browser_state_);
+  }
+
+  if (initiation_type == web::NavigationInitiationType::RENDERER_INITIATED &&
+      loaded_url != url && web::GetWebClient()->IsAppSpecificURL(loaded_url)) {
+    const NavigationItem* last_committed_item = GetLastCommittedItem();
+    bool last_committed_url_is_app_specific =
+        last_committed_item &&
+        web::GetWebClient()->IsAppSpecificURL(last_committed_item->GetURL());
+    if (!last_committed_url_is_app_specific) {
+      // The URL should not be changed to app-specific URL if the load was
+      // renderer-initiated requested by non app-specific URL. Pages with
+      // app-specific urls have elevated previledges and should not be allowed
+      // to open app-specific URLs.
+      loaded_url = url;
+    }
+  }
+
+  auto item = base::MakeUnique<NavigationItemImpl>();
+  item->SetOriginalRequestURL(loaded_url);
+  item->SetURL(loaded_url);
+  item->SetReferrer(referrer);
+  item->SetTransitionType(transition);
+  item->SetNavigationInitiationType(initiation_type);
+  if (web::GetWebClient()->IsAppSpecificURL(loaded_url)) {
+    item->SetUserAgentType(web::UserAgentType::NONE);
+  }
+
+  return item;
+}
+
 }  // namespace web

ios/web/navigation/navigation_manager_impl_unittest.mm

@@ -40,6 +40,18 @@ bool UrlRewriter(GURL* url, BrowserState* browser_state) {
   return false;
 }
 
+// Query parameter that will be appended by AppendingUrlRewriter if it is
+// installed into NavigationManager by a test case.
+const char kRewrittenQueryParam[] = "navigationmanagerrewrittenquery";
+
+// Appends |kRewrittenQueryParam| to |url|.
+bool AppendingUrlRewriter(GURL* url, BrowserState* browser_state) {
+  GURL::Replacements query_replacements;
+  query_replacements.SetQueryStr(kRewrittenQueryParam);
+  *url = url->ReplaceComponents(query_replacements);
+  return false;
+}
+
 // Stub class for NavigationManagerDelegate.
 class TestNavigationManagerDelegate : public NavigationManagerDelegate {
  public:
@@ -1415,6 +1427,28 @@ TEST_P(NavigationManagerTest, RewritingAppSpecificUrls) {
   EXPECT_EQ(rewritten_url4, navigation_manager()->GetPendingItem()->GetURL());
 }
 
+// Tests that transient URLRewriters are applied for pending items.
+TEST_P(NavigationManagerTest, ApplyTransientRewriters) {
+  navigation_manager()->AddTransientURLRewriter(&AppendingUrlRewriter);
+  navigation_manager()->AddPendingItem(
+      GURL("http://www.0.com"), Referrer(), ui::PAGE_TRANSITION_LINK,
+      web::NavigationInitiationType::USER_INITIATED,
+      web::NavigationManager::UserAgentOverrideOption::INHERIT);
+
+  NavigationItem* pending_item = navigation_manager()->GetPendingItem();
+  EXPECT_EQ(kRewrittenQueryParam, pending_item->GetURL().query());
+
+  // Now that the transient rewriters are consumed, the next URL should not be
+  // changed.
+  GURL url("http://www.1.com");
+  navigation_manager()->AddPendingItem(
+      url, Referrer(), ui::PAGE_TRANSITION_LINK,
+      web::NavigationInitiationType::USER_INITIATED,
+      web::NavigationManager::UserAgentOverrideOption::INHERIT);
+
+  EXPECT_EQ(url, navigation_manager()->GetPendingItem()->GetURL());
+}
+
 // Tests that GetIndexOfItem() returns the correct values.
 TEST_P(NavigationManagerTest, GetIndexOfItem) {
   // Create two items and add them to the NavigationManagerImpl.

ios/web/navigation/wk_based_navigation_manager_impl.mm

@@ -96,9 +96,10 @@ CRWSessionController* WKBasedNavigationManagerImpl::GetSessionController()
 }
 
 void WKBasedNavigationManagerImpl::AddTransientItem(const GURL& url) {
-  transient_item_ =
-      CreateNavigationItem(url, Referrer(), ui::PAGE_TRANSITION_CLIENT_REDIRECT,
-                           NavigationInitiationType::USER_INITIATED);
+  transient_item_ = CreateNavigationItemWithRewriters(
+      url, Referrer(), ui::PAGE_TRANSITION_CLIENT_REDIRECT,
+      NavigationInitiationType::USER_INITIATED,
+      nullptr /* use default rewriters only */);
   transient_item_->SetTimestamp(
       time_smoother_.GetSmoothedTime(base::Time::Now()));
 
@@ -116,8 +117,10 @@ void WKBasedNavigationManagerImpl::AddPendingItem(
     UserAgentOverrideOption user_agent_override_option) {
   DiscardNonCommittedItems();
 
-  pending_item_ =
-      CreateNavigationItem(url, referrer, navigation_type, initiation_type);
+  pending_item_ = CreateNavigationItemWithRewriters(
+      url, referrer, navigation_type, initiation_type,
+      &transient_url_rewriters_);
+  RemoveTransientURLRewriters();
 
   // WKBasedNavigationManagerImpl does not track
   // native URLs yet so just inherit from the
@@ -343,18 +346,15 @@ NavigationItemImpl* WKBasedNavigationManagerImpl::GetNavigationItemImplAtIndex(
   // TODO(crbug.com/734150): Add a stat counter to track rebuilding frequency.
   WKBackForwardListItem* prev_wk_item =
       index == 0 ? nil : GetWKItemAtIndex(index - 1);
-  // TODO(crbug.com/734150): CreateNavigationItem is not const because it clears
-  // transient rewriters. Perhaps transient rewriters should not be used for
-  // lazily created items. Investigate and make a decision before this code goes
-  // live.
   std::unique_ptr<web::NavigationItemImpl> new_item =
-      const_cast<WKBasedNavigationManagerImpl*>(this)->CreateNavigationItem(
+      CreateNavigationItemWithRewriters(
           net::GURLWithNSURL(wk_item.URL),
           (prev_wk_item ? web::Referrer(net::GURLWithNSURL(prev_wk_item.URL),
                                         web::ReferrerPolicyAlways)
                         : web::Referrer()),
           ui::PageTransition::PAGE_TRANSITION_LINK,
-          NavigationInitiationType::RENDERER_INITIATED);
+          NavigationInitiationType::RENDERER_INITIATED,
+          nullptr /* use default rewriters only */);
   SetNavigationItemInWKItem(wk_item, std::move(new_item));
   return GetNavigationItemFromWKItem(wk_item);
 }

ios/web/navigation/wk_based_navigation_manager_impl_unittest.mm

@@ -23,6 +23,18 @@
 
 namespace web {
 
+// Query parameter that will be appended by AppendingUrlRewriter if it is
+// installed into NavigationManager by a test case.
+const char kRewrittenQueryParam[] = "wknavigationmanagerrewrittenquery";
+
+// Appends |kRewrittenQueryParam| to |url|.
+bool AppendingUrlRewriter(GURL* url, BrowserState* browser_state) {
+  GURL::Replacements query_replacements;
+  query_replacements.SetQueryStr(kRewrittenQueryParam);
+  *url = url->ReplaceComponents(query_replacements);
+  return false;
+}
+
 // Test fixture for WKBasedNavigationManagerImpl.
 class WKBasedNavigationManagerTest : public PlatformTest {
  protected:
@@ -188,6 +200,34 @@ TEST_F(WKBasedNavigationManagerTest, ReusePendingItemForHistoryNavigation) {
   EXPECT_EQ(original_item0, manager_->GetPendingItem());
 }
 
+// Tests that transient URL rewriters are only applied to a new pending item.
+TEST_F(WKBasedNavigationManagerTest,
+       TransientURLRewritersOnlyUsedForPendingItem) {
+  manager_->AddPendingItem(GURL("http://www.0.com"), Referrer(),
+                           ui::PAGE_TRANSITION_TYPED,
+                           NavigationInitiationType::USER_INITIATED,
+                           NavigationManager::UserAgentOverrideOption::INHERIT);
+
+  // Install transient URL rewriters.
+  manager_->AddTransientURLRewriter(&AppendingUrlRewriter);
+  [mock_wk_list_ setCurrentURL:@"http://www.0.com"];
+
+  // Transient URL rewriters do not apply to lazily synced items.
+  NavigationItem* item0 = manager_->GetItemAtIndex(0);
+  EXPECT_EQ(GURL("http://www.0.com"), item0->GetURL());
+
+  // Transient URL rewriters do not apply to transient items.
+  manager_->AddTransientItem(GURL("http://www.1.com"));
+  EXPECT_EQ(GURL("http://www.1.com"), manager_->GetTransientItem()->GetURL());
+
+  // Transient URL rewriters are applied to a new pending item.
+  manager_->AddPendingItem(GURL("http://www.2.com"), Referrer(),
+                           ui::PAGE_TRANSITION_TYPED,
+                           NavigationInitiationType::USER_INITIATED,
+                           NavigationManager::UserAgentOverrideOption::INHERIT);
+  EXPECT_EQ(kRewrittenQueryParam, manager_->GetPendingItem()->GetURL().query());
+}
+
 // Tests DiscardNonCommittedItems discards both pending and transient items.
 TEST_F(WKBasedNavigationManagerTest, DiscardNonCommittedItems) {
   manager_->AddPendingItem(