history.replaceState(..., location.href) clearing page-action icons.

content/browser/web_contents/navigation_controller_impl_unittest.cc

Index: content/browser/web_contents/navigation_controller_impl_unittest.cc
===================================================================
--- content/browser/web_contents/navigation_controller_impl_unittest.cc	(revision 153705)
+++ content/browser/web_contents/navigation_controller_impl_unittest.cc	(working copy)
@@ -1435,7 +1435,27 @@
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
 
-  // First navigation.
+  // Distinguish reload vs. history.replaceState() of same url.

[Charlie Reis, 2012/08/28 21:48:21]

This comment is confusing, since you don't test reload here.  Maybe rephrase as
"Ensure history.replaceState() of the same URL is not treated as a reload"?

[Tom Sepez, 2012/08/28 21:57:59]

Done.

+  ViewHostMsg_FrameNavigate_Params self_params;
+  self_params.page_id = 1;
+  self_params.url = url1;
+  self_params.transition = content::PAGE_TRANSITION_LINK;
+  self_params.should_update_history = false;
+  self_params.gesture = NavigationGestureUser;
+  self_params.is_post = false;
+  self_params.content_state = webkit_glue::CreateHistoryStateForURL(GURL(url1));
+  self_params.was_within_same_page = true;
+
+  // This should generate a new entry.

[Charlie Reis, 2012/08/28 21:48:21]

Again, this is tricky to follow because there's a difference between generating
a new entry and whether it's considered in-page.  Maybe "This should generate a
new in-page entry" would be clearer.

Either way, this seems backwards to me.  replaceState is supposed to replace the
current entry, so I would not expect this to generate a new entry.  In contrast,
fragment navigations (below) seem like they should generate a new entry (as we
used to verify), and I don't understand why that changed.

What am I missing?

[Tom Sepez, 2012/08/28 21:57:59]

This appears to be an artifact of the interaction between
test_rvh()->SendNavigate() and controller.RendererDidNavigate().  The first such
controller.RendererDidNavigate() call is being treated as a new page, but the
subsequent ones have the context to return the right result.  

[Charlie Reis, 2012/08/28 22:17:23]

Can we just call RendererDidNavigate() on the initial "main page" navigation, to
avoid this strange interaction?  It seems to be clouding the meaning of
replaceState.

(I also still think fragment navigations are supposed to generate new entries,
since you can go back/forward to them.)

+  content::LoadCommittedDetails details;
+  EXPECT_TRUE(controller.RendererDidNavigate(self_params, &details));
+  EXPECT_TRUE(notifications.Check1AndReset(
+      content::NOTIFICATION_NAV_ENTRY_COMMITTED));
+  EXPECT_TRUE(details.is_in_page);
+  EXPECT_FALSE(details.did_replace_entry);
+  EXPECT_EQ(2, controller.GetEntryCount());
+
+  // Fragment navigation.
   const GURL url2("http://foo#a");
   ViewHostMsg_FrameNavigate_Params params;
   params.page_id = 1;
@@ -1446,13 +1466,12 @@
   params.is_post = false;
   params.content_state = webkit_glue::CreateHistoryStateForURL(GURL(url2));
 
-  // This should generate a new entry.
-  content::LoadCommittedDetails details;
+  // This should not generate a new entry.
   EXPECT_TRUE(controller.RendererDidNavigate(params, &details));
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
   EXPECT_TRUE(details.is_in_page);
-  EXPECT_FALSE(details.did_replace_entry);
+  EXPECT_TRUE(details.did_replace_entry);
   EXPECT_EQ(2, controller.GetEntryCount());
 
   // Go back one.