Add a timestamp field to NavigationEntry

This was split off from 11000014 (to narrow down what's causing
the chrome frame test failures).

Set the active NavigationEntry's timestamp in NavigationControllerImpl::RendererDidNavigate.

BUG=128449
TBR=avi@chromium.org,sky@chromium.org

Review URL: https://codereview.chromium.org/10983081

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@159209 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 133 insertions, 3 deletions

diff --git a/content/browser/web_contents/navigation_controller_impl.cc b/content/browser/web_contents/navigation_controller_impl.cc
index 519d64e..750ee37 100644
--- a/content/browser/web_contents/navigation_controller_impl.cc
+++ b/content/browser/web_contents/navigation_controller_impl.cc
@@ -305,7 +305,7 @@ void NavigationControllerImpl::ReloadInternal(bool check_for_repost,
     if (site_instance &&
         site_instance->HasWrongProcessForURL(entry->GetURL())) {
       // Create a navigation entry that resembles the current one, but do not
-      // copy page id, site instance, and content state.
+      // copy page id, site instance, content state, or timestamp.
       NavigationEntryImpl* nav_entry = NavigationEntryImpl::FromNavigationEntry(
           CreateNavigationEntry(
               entry->GetURL(), entry->GetReferrer(), entry->GetTransitionType(),
@@ -739,11 +739,25 @@ bool NavigationControllerImpl::RendererDidNavigate(
       NOTREACHED();
   }
 
+  // At this point, we know that the navigation has just completed, so
+  // record the time.
+  //
+  // TODO(akalin): Use "sane time" as described in
+  // http://www.chromium.org/developers/design-documents/sane-time .
+  //
+  // TODO(akalin): Make sure (to at least a high probability) that the
+  // generated timestamp is unique.  (Move the uniquifying logic from
+  // history_backend.cc.)
+  const base::Time timestamp = base::Time::Now();
+  DVLOG(1) << "Navigation finished at timestamp "
+           << timestamp.ToInternalValue();
+
   // All committed entries should have nonempty content state so WebKit doesn't
   // get confused when we go back to them (see the function for details).
   DCHECK(!params.content_state.empty());
   NavigationEntryImpl* active_entry =
       NavigationEntryImpl::FromNavigationEntry(GetActiveEntry());
+  active_entry->SetTimestamp(timestamp);
   active_entry->SetContentState(params.content_state);
   // No longer needed since content state will hold the post data if any.
   active_entry->SetBrowserInitiatedPostData(NULL);
diff --git a/content/browser/web_contents/navigation_controller_impl_unittest.cc b/content/browser/web_contents/navigation_controller_impl_unittest.cc
index 867363e..9a0d585 100644
--- a/content/browser/web_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/web_contents/navigation_controller_impl_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
+#include "base/time.h"
 #include "base/utf_string_conversions.h"
 //  These are only used for commented out tests.  If someone wants to enable
 //  them, they should be moved to chrome first.
@@ -101,6 +102,8 @@ TEST_F(NavigationControllerTest, Defaults) {
   NavigationControllerImpl& controller = controller_impl();
 
   EXPECT_FALSE(controller.GetPendingEntry());
+  EXPECT_FALSE(controller.GetActiveEntry());
+  EXPECT_FALSE(controller.GetVisibleEntry());
   EXPECT_FALSE(controller.GetLastCommittedEntry());
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_EQ(controller.GetLastCommittedEntryIndex(), -1);
@@ -128,11 +131,16 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_EQ(controller.GetLastCommittedEntryIndex(), -1);
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_FALSE(controller.GetLastCommittedEntry());
-  EXPECT_TRUE(controller.GetPendingEntry());
+  ASSERT_TRUE(controller.GetPendingEntry());
+  EXPECT_EQ(controller.GetPendingEntry(), controller.GetActiveEntry());
+  EXPECT_EQ(controller.GetPendingEntry(), controller.GetVisibleEntry());
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
   EXPECT_EQ(contents()->GetMaxPageID(), -1);
 
+  // The timestamp should not have been set yet.
+  EXPECT_TRUE(controller.GetPendingEntry()->GetTimestamp().is_null());
+
   // We should have gotten no notifications from the preceeding checks.
   EXPECT_EQ(0U, notifications.size());
 
@@ -146,10 +154,15 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_TRUE(controller.GetLastCommittedEntry());
   EXPECT_FALSE(controller.GetPendingEntry());
+  ASSERT_TRUE(controller.GetActiveEntry());
+  EXPECT_EQ(controller.GetActiveEntry(), controller.GetVisibleEntry());
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
   EXPECT_EQ(contents()->GetMaxPageID(), 0);
 
+  // The timestamp should have been set.
+  EXPECT_FALSE(controller.GetActiveEntry()->GetTimestamp().is_null());
+
   // Load another...
   controller.LoadURL(
       url2, content::Referrer(), content::PAGE_TRANSITION_TYPED, std::string());
@@ -159,12 +172,16 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_EQ(controller.GetLastCommittedEntryIndex(), 0);
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_TRUE(controller.GetLastCommittedEntry());
-  EXPECT_TRUE(controller.GetPendingEntry());
+  ASSERT_TRUE(controller.GetPendingEntry());
+  EXPECT_EQ(controller.GetPendingEntry(), controller.GetActiveEntry());
+  EXPECT_EQ(controller.GetPendingEntry(), controller.GetVisibleEntry());
   // TODO(darin): maybe this should really be true?
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
   EXPECT_EQ(contents()->GetMaxPageID(), 0);
 
+  EXPECT_TRUE(controller.GetPendingEntry()->GetTimestamp().is_null());
+
   // Simulate the beforeunload ack for the cross-site transition, and then the
   // commit.
   test_rvh()->SendShouldCloseACK(true);
@@ -179,9 +196,13 @@ TEST_F(NavigationControllerTest, LoadURL) {
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_TRUE(controller.GetLastCommittedEntry());
   EXPECT_FALSE(controller.GetPendingEntry());
+  ASSERT_TRUE(controller.GetActiveEntry());
+  EXPECT_EQ(controller.GetActiveEntry(), controller.GetVisibleEntry());
   EXPECT_TRUE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
   EXPECT_EQ(contents()->GetMaxPageID(), 1);
+
+  EXPECT_FALSE(controller.GetActiveEntry()->GetTimestamp().is_null());
 }
 
 void CheckNavigationEntryMatchLoadParams(
@@ -228,6 +249,10 @@ TEST_F(NavigationControllerTest, LoadURLWithParams) {
       NavigationEntryImpl::FromNavigationEntry(
           controller.GetPendingEntry());
 
+  // The timestamp should not have been set yet.
+  ASSERT_TRUE(entry);
+  EXPECT_TRUE(entry->GetTimestamp().is_null());
+
   CheckNavigationEntryMatchLoadParams(load_params, entry);
 }
 
@@ -293,6 +318,10 @@ TEST_F(NavigationControllerTest, LoadURL_SamePage) {
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
 
+  ASSERT_TRUE(controller.GetActiveEntry());
+  const base::Time timestamp = controller.GetActiveEntry()->GetTimestamp();
+  EXPECT_FALSE(timestamp.is_null());
+
   controller.LoadURL(
       url1, content::Referrer(), content::PAGE_TRANSITION_TYPED, std::string());
   EXPECT_EQ(0U, notifications.size());
@@ -306,8 +335,15 @@ TEST_F(NavigationControllerTest, LoadURL_SamePage) {
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_TRUE(controller.GetLastCommittedEntry());
   EXPECT_FALSE(controller.GetPendingEntry());
+  ASSERT_TRUE(controller.GetActiveEntry());
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
+
+  // The timestamp should have been updated.
+  //
+  // TODO(akalin): Change this EXPECT_GE (and other similar ones) to
+  // EXPECT_GT once we guarantee that timestamps are unique.
+  EXPECT_GE(controller.GetActiveEntry()->GetTimestamp(), timestamp);
 }
 
 // Tests loading a URL but discarding it before the load commits.
@@ -326,6 +362,10 @@ TEST_F(NavigationControllerTest, LoadURL_Discarded) {
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
 
+  ASSERT_TRUE(controller.GetActiveEntry());
+  const base::Time timestamp = controller.GetActiveEntry()->GetTimestamp();
+  EXPECT_FALSE(timestamp.is_null());
+
   controller.LoadURL(
       url2, content::Referrer(), content::PAGE_TRANSITION_TYPED, std::string());
   controller.DiscardNonCommittedEntries();
@@ -337,8 +377,12 @@ TEST_F(NavigationControllerTest, LoadURL_Discarded) {
   EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
   EXPECT_TRUE(controller.GetLastCommittedEntry());
   EXPECT_FALSE(controller.GetPendingEntry());
+  ASSERT_TRUE(controller.GetActiveEntry());
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
+
+  // Timestamp should not have changed.
+  EXPECT_EQ(timestamp, controller.GetActiveEntry()->GetTimestamp());
 }
 
 // Tests navigations that come in unrequested. This happens when the user
@@ -671,10 +715,14 @@ TEST_F(NavigationControllerTest, Reload) {
   test_rvh()->SendNavigate(0, url1);
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
+  ASSERT_TRUE(controller.GetActiveEntry());
   controller.GetActiveEntry()->SetTitle(ASCIIToUTF16("Title"));
   controller.Reload(true);
   EXPECT_EQ(0U, notifications.size());
 
+  const base::Time timestamp = controller.GetActiveEntry()->GetTimestamp();
+  EXPECT_FALSE(timestamp.is_null());
+
   // The reload is pending.
   EXPECT_EQ(controller.GetEntryCount(), 1);
   EXPECT_EQ(controller.GetLastCommittedEntryIndex(), 0);
@@ -700,6 +748,10 @@ TEST_F(NavigationControllerTest, Reload) {
   EXPECT_FALSE(controller.GetPendingEntry());
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
+
+  // The timestamp should have been updated.
+  ASSERT_TRUE(controller.GetActiveEntry());
+  EXPECT_GE(controller.GetActiveEntry()->GetTimestamp(), timestamp);
 }
 
 // Tests what happens when a reload navigation produces a new page.
@@ -838,6 +890,11 @@ TEST_F(NavigationControllerTest, Back) {
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_TRUE(controller.CanGoForward());
 
+  // Timestamp for entry 1 should be on or after that of entry 0.
+  EXPECT_FALSE(controller.GetEntryAtIndex(0)->GetTimestamp().is_null());
+  EXPECT_GE(controller.GetEntryAtIndex(1)->GetTimestamp(),
+            controller.GetEntryAtIndex(0)->GetTimestamp());
+
   test_rvh()->SendNavigate(0, url2);
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
@@ -850,6 +907,11 @@ TEST_F(NavigationControllerTest, Back) {
   EXPECT_FALSE(controller.GetPendingEntry());
   EXPECT_FALSE(controller.CanGoBack());
   EXPECT_TRUE(controller.CanGoForward());
+
+  // Timestamp for entry 0 should be on or after that of entry 1
+  // (since we went back to it).
+  EXPECT_GE(controller.GetEntryAtIndex(0)->GetTimestamp(),
+            controller.GetEntryAtIndex(1)->GetTimestamp());
 }
 
 // Tests what happens when a back navigation produces a new page.
@@ -1017,6 +1079,12 @@ TEST_F(NavigationControllerTest, Forward) {
   EXPECT_TRUE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
 
+  // Timestamp for entry 0 should be on or after that of entry 1
+  // (since we went back to it).
+  EXPECT_FALSE(controller.GetEntryAtIndex(0)->GetTimestamp().is_null());
+  EXPECT_GE(controller.GetEntryAtIndex(0)->GetTimestamp(),
+            controller.GetEntryAtIndex(1)->GetTimestamp());
+
   test_rvh()->SendNavigate(1, url2);
   EXPECT_TRUE(notifications.Check1AndReset(
       content::NOTIFICATION_NAV_ENTRY_COMMITTED));
@@ -1029,6 +1097,11 @@ TEST_F(NavigationControllerTest, Forward) {
   EXPECT_FALSE(controller.GetPendingEntry());
   EXPECT_TRUE(controller.CanGoBack());
   EXPECT_FALSE(controller.CanGoForward());
+
+  // Timestamp for entry 1 should be on or after that of entry 0
+  // (since we went forward to it).
+  EXPECT_GE(controller.GetEntryAtIndex(1)->GetTimestamp(),
+            controller.GetEntryAtIndex(0)->GetTimestamp());
 }
 
 // Tests what happens when a forward navigation produces a new page.
@@ -1744,6 +1817,8 @@ TEST_F(NavigationControllerTest, RestoreNavigate) {
   entry->SetPageID(0);
   entry->SetTitle(ASCIIToUTF16("Title"));
   entry->SetContentState("state");
+  const base::Time timestamp = base::Time::Now();
+  entry->SetTimestamp(timestamp);
   entries.push_back(entry);
   scoped_ptr<WebContentsImpl> our_contents(
       WebContentsImpl::Create(browser_context(), NULL, MSG_ROUTING_NONE,
@@ -1754,6 +1829,7 @@ TEST_F(NavigationControllerTest, RestoreNavigate) {
 
   // Before navigating to the restored entry, it should have a restore_type
   // and no SiteInstance.
+  ASSERT_EQ(1, our_controller.GetEntryCount());
   EXPECT_EQ(NavigationEntryImpl::RESTORE_LAST_SESSION,
             NavigationEntryImpl::FromNavigationEntry(
                 our_controller.GetEntryAtIndex(0))->restore_type());
@@ -1773,6 +1849,9 @@ TEST_F(NavigationControllerTest, RestoreNavigate) {
   EXPECT_TRUE(NavigationEntryImpl::FromNavigationEntry(
       our_controller.GetEntryAtIndex(0))->site_instance());
 
+  // Timestamp should remain the same before the navigation finishes.
+  EXPECT_EQ(timestamp, our_controller.GetEntryAtIndex(0)->GetTimestamp());
+
   // Say we navigated to that entry.
   ViewHostMsg_FrameNavigate_Params params;
   params.page_id = 0;
@@ -1798,6 +1877,9 @@ TEST_F(NavigationControllerTest, RestoreNavigate) {
   EXPECT_EQ(NavigationEntryImpl::RESTORE_NONE,
             NavigationEntryImpl::FromNavigationEntry(
                 our_controller.GetEntryAtIndex(0))->restore_type());
+
+  // Timestamp should have been updated.
+  EXPECT_GE(our_controller.GetEntryAtIndex(0)->GetTimestamp(), timestamp);
 }
 
 // Tests that we can still navigate to a restored entry after a different
diff --git a/content/browser/web_contents/navigation_entry_impl.cc b/content/browser/web_contents/navigation_entry_impl.cc
index 4505f51..a3f6b90 100644
--- a/content/browser/web_contents/navigation_entry_impl.cc
+++ b/content/browser/web_contents/navigation_entry_impl.cc
@@ -255,4 +255,12 @@ bool NavigationEntryImpl::GetIsOverridingUserAgent() const {
   return is_overriding_user_agent_;
 }
 
+void NavigationEntryImpl::SetTimestamp(base::Time timestamp) {
+  timestamp_ = timestamp;
+}
+
+base::Time NavigationEntryImpl::GetTimestamp() const {
+  return timestamp_;
+}
+
 }  // namespace content
diff --git a/content/browser/web_contents/navigation_entry_impl.h b/content/browser/web_contents/navigation_entry_impl.h
index 36ea014..f404b5d 100644
--- a/content/browser/web_contents/navigation_entry_impl.h
+++ b/content/browser/web_contents/navigation_entry_impl.h
@@ -69,6 +69,8 @@ class CONTENT_EXPORT NavigationEntryImpl
   virtual const GURL& GetOriginalRequestURL() const OVERRIDE;
   virtual void SetIsOverridingUserAgent(bool override) OVERRIDE;
   virtual bool GetIsOverridingUserAgent() const OVERRIDE;
+  virtual void SetTimestamp(base::Time timestamp) OVERRIDE;
+  virtual base::Time GetTimestamp() const OVERRIDE;
 
   void set_unique_id(int unique_id) {
     unique_id_ = unique_id;
@@ -188,6 +190,7 @@ class CONTENT_EXPORT NavigationEntryImpl
   RestoreType restore_type_;
   GURL original_request_url_;
   bool is_overriding_user_agent_;
+  base::Time timestamp_;
 
   // This member is not persisted with session restore because it is transient.
   // If the post request succeeds, this field is cleared since the same
diff --git a/content/browser/web_contents/navigation_entry_impl_unittest.cc b/content/browser/web_contents/navigation_entry_impl_unittest.cc
index 5f9212f..e4f2dc1 100644
--- a/content/browser/web_contents/navigation_entry_impl_unittest.cc
+++ b/content/browser/web_contents/navigation_entry_impl_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "base/string16.h"
 #include "base/string_util.h"
+#include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/web_contents/navigation_entry_impl.h"
@@ -200,4 +201,12 @@ TEST_F(NavigationEntryTest, NavigationEntryAccessors) {
       entry2_->GetBrowserInitiatedPostData()->front());
 }
 
+// Test timestamps.
+TEST_F(NavigationEntryTest, NavigationEntryTimestamps) {
+  EXPECT_EQ(base::Time(), entry1_->GetTimestamp());
+  const base::Time now = base::Time::Now();
+  entry1_->SetTimestamp(now);
+  EXPECT_EQ(now, entry1_->GetTimestamp());
+}
+
 }  // namespace content
diff --git a/content/public/browser/navigation_entry.h b/content/public/browser/navigation_entry.h
index 63f0104..c907045 100644
--- a/content/public/browser/navigation_entry.h
+++ b/content/public/browser/navigation_entry.h
@@ -9,6 +9,7 @@
 
 #include "base/memory/ref_counted_memory.h"
 #include "base/string16.h"
+#include "base/time.h"
 #include "content/common/content_export.h"
 #include "content/public/common/page_transition_types.h"
 #include "content/public/common/page_type.h"
@@ -163,6 +164,19 @@ class NavigationEntry {
   // Store whether or not we're overriding the user agent.
   virtual void SetIsOverridingUserAgent(bool override) = 0;
   virtual bool GetIsOverridingUserAgent() const = 0;
+
+  // The time at which the last known local navigation has
+  // completed. (A navigation can be completed more than once if the
+  // page is reloaded.)
+  //
+  // If GetTimestamp() returns a null time, that means that either:
+  //
+  //   - this navigation hasn't completed yet;
+  //   - this navigation was restored and for some reason the
+  //     timestamp wasn't available;
+  //   - or this navigation was copied from a foreign session.
+  virtual void SetTimestamp(base::Time timestamp) = 0;
+  virtual base::Time GetTimestamp() const = 0;
 };
 
 }  // namespace content