Lands http://codereview.chromium.org/1744011/show for Hans:

Make reopened tabs with "about:" URLs display properly. 

- Rename TabNavigation::url_ to virtual_url_. 
Make it more explicit that a TabNavigation stores the *virtual* URL of
a 
NavigationEntry. 

- Translate virtual URLs when restoring tabs. 
Instead of putting the virtual URL in the URL proper field when
creating a 
NavigationEntry from a TabNavigation, put it in the virtual URL field,
and 
then translate this to a proper URL in 
NavigationController::CreateNavigationEntriesFromTabNavigations(). 
This fixes bug 31905. 

Contributed by Hans Wennborg <hans@chromium.org> 

BUG=31905
TEST=ui_tests --gtest_filter=TabRestoreUITest.RestoreTabWithSpecialURL*

Review URL: http://codereview.chromium.org/1946002

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

7 files changed, 56 insertions, 53 deletions

diff --git a/chrome/browser/sessions/base_session_service.cc b/chrome/browser/sessions/base_session_service.cc
index acd685b..45479a9 100644
--- a/chrome/browser/sessions/base_session_service.cc
+++ b/chrome/browser/sessions/base_session_service.cc
@@ -229,7 +229,7 @@ bool BaseSessionService::RestoreUpdateTabNavigationCommand(
       navigation->referrer_ = GURL(referrer_spec);
   }
 
-  navigation->url_ = GURL(url_spec);
+  navigation->virtual_url_ = GURL(url_spec);
   return true;
 }
 
@@ -251,7 +251,7 @@ bool BaseSessionService::ShouldTrackEntry(const NavigationEntry& entry) {
 }
 
 bool BaseSessionService::ShouldTrackEntry(const TabNavigation& navigation) {
-  return navigation.url().is_valid();
+  return navigation.virtual_url().is_valid();
 }
 
 BaseSessionService::Handle BaseSessionService::ScheduleGetLastSessionCommands(
diff --git a/chrome/browser/sessions/session_service_test_helper.cc b/chrome/browser/sessions/session_service_test_helper.cc
index 7703bc8..a36562d 100644
--- a/chrome/browser/sessions/session_service_test_helper.cc
+++ b/chrome/browser/sessions/session_service_test_helper.cc
@@ -69,7 +69,7 @@ void SessionServiceTestHelper::AssertTabEquals(
 void SessionServiceTestHelper::AssertNavigationEquals(
     const TabNavigation& expected,
     const TabNavigation& actual) {
-  EXPECT_TRUE(expected.url() == actual.url());
+  EXPECT_TRUE(expected.virtual_url() == actual.virtual_url());
   EXPECT_EQ(expected.referrer(), actual.referrer());
   EXPECT_EQ(expected.title(), actual.title());
   EXPECT_EQ(expected.state(), actual.state());
diff --git a/chrome/browser/sessions/session_service_unittest.cc b/chrome/browser/sessions/session_service_unittest.cc
index 3a32b55..3a4e905 100644
--- a/chrome/browser/sessions/session_service_unittest.cc
+++ b/chrome/browser/sessions/session_service_unittest.cc
@@ -53,7 +53,7 @@ class SessionServiceTest : public BrowserWithTestWindowTest {
                         int index,
                         bool select) {
     NavigationEntry entry;
-    entry.set_url(navigation.url());
+    entry.set_url(navigation.virtual_url());
     entry.set_referrer(navigation.referrer());
     entry.set_title(navigation.title());
     entry.set_content_state(navigation.state());
@@ -520,9 +520,12 @@ TEST_F(SessionServiceTest, PruneFromFront) {
   SessionTab* tab = windows[0]->tabs[0];
   ASSERT_EQ(1, tab->current_navigation_index);
   EXPECT_EQ(3U, tab->navigations.size());
-  EXPECT_TRUE(GURL(base_url + IntToString(2)) == tab->navigations[0].url());
-  EXPECT_TRUE(GURL(base_url + IntToString(3)) == tab->navigations[1].url());
-  EXPECT_TRUE(GURL(base_url + IntToString(4)) == tab->navigations[2].url());
+  EXPECT_TRUE(GURL(base_url + IntToString(2)) ==
+              tab->navigations[0].virtual_url());
+  EXPECT_TRUE(GURL(base_url + IntToString(3)) ==
+              tab->navigations[1].virtual_url());
+  EXPECT_TRUE(GURL(base_url + IntToString(4)) ==
+              tab->navigations[2].virtual_url());
 }
 
 // Prunes from front so that we have no entries.
@@ -592,14 +595,14 @@ class GetCurrentSessionCallbackHandler {
     EXPECT_EQ(2U, (*windows)[0]->tabs.size());
     EXPECT_EQ(2U, (*windows)[0]->tabs[0]->navigations.size());
     EXPECT_EQ(GURL("http://bar/1"),
-              (*windows)[0]->tabs[0]->navigations[0].url());
+              (*windows)[0]->tabs[0]->navigations[0].virtual_url());
     EXPECT_EQ(GURL("http://bar/2"),
-              (*windows)[0]->tabs[0]->navigations[1].url());
+              (*windows)[0]->tabs[0]->navigations[1].virtual_url());
     EXPECT_EQ(2U, (*windows)[0]->tabs[1]->navigations.size());
     EXPECT_EQ(GURL("http://foo/1"),
-              (*windows)[0]->tabs[1]->navigations[0].url());
+              (*windows)[0]->tabs[1]->navigations[0].virtual_url());
     EXPECT_EQ(GURL("http://foo/2"),
-              (*windows)[0]->tabs[1]->navigations[1].url());
+              (*windows)[0]->tabs[1]->navigations[1].virtual_url());
   }
 };
 
diff --git a/chrome/browser/sessions/session_types.cc b/chrome/browser/sessions/session_types.cc
index 852fe8b..27caf057 100644
--- a/chrome/browser/sessions/session_types.cc
+++ b/chrome/browser/sessions/session_types.cc
@@ -5,30 +5,32 @@
 #include "chrome/browser/sessions/session_types.h"
 
 #include "base/string_util.h"
+#include "chrome/browser/tab_contents/navigation_controller.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 
 // TabNavigation --------------------------------------------------------------
 
 // static
-NavigationEntry* TabNavigation::ToNavigationEntry(int page_id) const {
-  NavigationEntry* entry = new NavigationEntry(
-      NULL,  // The site instance for restored tabs is sent on navigation
-             // (TabContents::GetSiteInstanceForEntry).
-      page_id,
-      url_,
+NavigationEntry* TabNavigation::ToNavigationEntry(int page_id,
+                                                  Profile *profile) const {
+  NavigationEntry* entry = NavigationController::CreateNavigationEntry(
+      virtual_url_,
       referrer_,
-      title_,
       // Use a transition type of reload so that we don't incorrectly
       // increase the typed count.
-      PageTransition::RELOAD);
-  entry->set_virtual_url(url_);
+      PageTransition::RELOAD,
+      profile);
+
+  entry->set_page_id(page_id);
+  entry->set_title(title_);
   entry->set_content_state(state_);
   entry->set_has_post_data(type_mask_ & TabNavigation::HAS_POST_DATA);
+
   return entry;
 }
 
 void TabNavigation::SetFromNavigationEntry(const NavigationEntry& entry) {
-  url_ = entry.virtual_url();
+  virtual_url_ = entry.virtual_url();
   referrer_ = entry.referrer();
   title_ = entry.title();
   state_ = entry.content_state();
diff --git a/chrome/browser/sessions/session_types.h b/chrome/browser/sessions/session_types.h
index 0d31d15..b6f1dbc 100644
--- a/chrome/browser/sessions/session_types.h
+++ b/chrome/browser/sessions/session_types.h
@@ -37,12 +37,12 @@ class TabNavigation {
   }
 
   TabNavigation(int index,
-                const GURL& url,
+                const GURL& virtual_url,
                 const GURL& referrer,
                 const string16& title,
                 const std::string& state,
                 PageTransition::Type transition)
-      : url_(url),
+      : virtual_url_(virtual_url),
         referrer_(referrer),
         title_(title),
         state_(state),
@@ -52,14 +52,14 @@ class TabNavigation {
 
   // Converts this TabNavigation into a NavigationEntry with a page id of
   // |page_id|. The caller owns the returned NavigationEntry.
-  NavigationEntry* ToNavigationEntry(int page_id) const;
+  NavigationEntry* ToNavigationEntry(int page_id, Profile* profile) const;
 
   // Resets this TabNavigation from |entry|.
   void SetFromNavigationEntry(const NavigationEntry& entry);
 
-  // URL of the page.
-  void set_url(const GURL& url) { url_ = url; }
-  const GURL& url() const { return url_; }
+  // Virtual URL of the page. See NavigationEntry::virtual_url() for details.
+  void set_virtual_url(const GURL& url) { virtual_url_ = url; }
+  const GURL& virtual_url() const { return virtual_url_; }
 
   // The referrer.
   const GURL& referrer() const { return referrer_; }
@@ -92,7 +92,7 @@ class TabNavigation {
  private:
   friend class BaseSessionService;
 
-  GURL url_;
+  GURL virtual_url_;
   GURL referrer_;
   string16 title_;
   std::string state_;
diff --git a/chrome/browser/sessions/tab_restore_service.cc b/chrome/browser/sessions/tab_restore_service.cc
index 47fe4e1..4dea440 100644
--- a/chrome/browser/sessions/tab_restore_service.cc
+++ b/chrome/browser/sessions/tab_restore_service.cc
@@ -534,7 +534,7 @@ void TabRestoreService::ScheduleCommandsForTab(const Tab& tab,
       // this, but it simplifies the code and makes it less error prone as we
       // add new data to NavigationEntry.
       scoped_ptr<NavigationEntry> entry(
-          navigations[i].ToNavigationEntry(wrote_count));
+          navigations[i].ToNavigationEntry(wrote_count, profile()));
       ScheduleCommand(
           CreateUpdateTabNavigationCommand(kCommandUpdateTabNavigation, tab.id,
                                            wrote_count++, *entry));
@@ -947,4 +947,3 @@ void TabRestoreService::LoadStateChanged() {
 Time TabRestoreService::TimeNow() const {
   return time_factory_ ? time_factory_->TimeNow() : Time::Now();
 }
-
diff --git a/chrome/browser/sessions/tab_restore_service_unittest.cc b/chrome/browser/sessions/tab_restore_service_unittest.cc
index 044dfdd..4af768d 100644
--- a/chrome/browser/sessions/tab_restore_service_unittest.cc
+++ b/chrome/browser/sessions/tab_restore_service_unittest.cc
@@ -129,9 +129,9 @@ TEST_F(TabRestoreServiceTest, Basic) {
   EXPECT_FALSE(tab->pinned);
   EXPECT_TRUE(tab->app_extension_id.empty());
   ASSERT_EQ(3U, tab->navigations.size());
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_TRUE(url3_ == tab->navigations[2].url());
+  EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
+  EXPECT_TRUE(url2_ == tab->navigations[1].virtual_url());
+  EXPECT_TRUE(url3_ == tab->navigations[2].virtual_url());
   EXPECT_EQ(2, tab->current_navigation_index);
   EXPECT_EQ(time_factory_->TimeNow().ToInternalValue(),
             tab->timestamp.ToInternalValue());
@@ -150,9 +150,9 @@ TEST_F(TabRestoreServiceTest, Basic) {
   tab = static_cast<TabRestoreService::Tab*>(entry);
   EXPECT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_TRUE(url3_ == tab->navigations[2].url());
+  EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
+  EXPECT_TRUE(url2_ == tab->navigations[1].virtual_url());
+  EXPECT_TRUE(url3_ == tab->navigations[2].virtual_url());
   EXPECT_EQ(1, tab->current_navigation_index);
   EXPECT_EQ(time_factory_->TimeNow().ToInternalValue(),
             tab->timestamp.ToInternalValue());
@@ -184,9 +184,9 @@ TEST_F(TabRestoreServiceTest, Restore) {
   TabRestoreService::Tab* tab = static_cast<TabRestoreService::Tab*>(entry);
   EXPECT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_TRUE(url3_ == tab->navigations[2].url());
+  EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
+  EXPECT_TRUE(url2_ == tab->navigations[1].virtual_url());
+  EXPECT_TRUE(url3_ == tab->navigations[2].virtual_url());
   EXPECT_EQ(2, tab->current_navigation_index);
   EXPECT_EQ(time_factory_->TimeNow().ToInternalValue(),
             tab->timestamp.ToInternalValue());
@@ -223,9 +223,9 @@ TEST_F(TabRestoreServiceTest, RestorePinnedAndApp) {
   tab = static_cast<TabRestoreService::Tab*>(entry);
   EXPECT_TRUE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_TRUE(url3_ == tab->navigations[2].url());
+  EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
+  EXPECT_TRUE(url2_ == tab->navigations[1].virtual_url());
+  EXPECT_TRUE(url3_ == tab->navigations[2].virtual_url());
   EXPECT_EQ(2, tab->current_navigation_index);
   EXPECT_TRUE(app_extension_id == tab->app_extension_id);
 }
@@ -297,7 +297,7 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSession) {
   ASSERT_EQ(1U, window->tabs[0].navigations.size());
   EXPECT_EQ(0, window->tabs[0].current_navigation_index);
   EXPECT_EQ(0, window->tabs[0].timestamp.ToInternalValue());
-  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].url());
+  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].virtual_url());
 }
 
 // Makes sure we don't attempt to load previous sessions after a restore.
@@ -352,7 +352,7 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabs) {
   ASSERT_EQ(1U, window->tabs[0].navigations.size());
   EXPECT_EQ(0, window->tabs[0].current_navigation_index);
   EXPECT_EQ(0, window->tabs[0].timestamp.ToInternalValue());
-  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].url());
+  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].virtual_url());
 
   // Then the closed tab.
   entry = *(++service_->entries().begin());
@@ -363,9 +363,9 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabs) {
   EXPECT_EQ(2, tab->current_navigation_index);
   EXPECT_EQ(time_factory_->TimeNow().ToInternalValue(),
             tab->timestamp.ToInternalValue());
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_TRUE(url3_ == tab->navigations[2].url());
+  EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
+  EXPECT_TRUE(url2_ == tab->navigations[1].virtual_url());
+  EXPECT_TRUE(url3_ == tab->navigations[2].virtual_url());
 }
 
 // Make sure pinned state is correctly loaded from session service.
@@ -393,7 +393,7 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabsPinned) {
   EXPECT_TRUE(window->tabs[0].pinned);
   ASSERT_EQ(1U, window->tabs[0].navigations.size());
   EXPECT_EQ(0, window->tabs[0].current_navigation_index);
-  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].url());
+  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].virtual_url());
 
   // Then the closed tab.
   entry = *(++service_->entries().begin());
@@ -402,9 +402,9 @@ TEST_F(TabRestoreServiceTest, LoadPreviousSessionAndTabsPinned) {
   ASSERT_FALSE(tab->pinned);
   ASSERT_EQ(3U, tab->navigations.size());
   EXPECT_EQ(2, tab->current_navigation_index);
-  EXPECT_TRUE(url1_ == tab->navigations[0].url());
-  EXPECT_TRUE(url2_ == tab->navigations[1].url());
-  EXPECT_TRUE(url3_ == tab->navigations[2].url());
+  EXPECT_TRUE(url1_ == tab->navigations[0].virtual_url());
+  EXPECT_TRUE(url2_ == tab->navigations[1].virtual_url());
+  EXPECT_TRUE(url3_ == tab->navigations[2].virtual_url());
 }
 
 // Creates TabRestoreService::kMaxEntries + 1 windows in the session service
@@ -438,7 +438,7 @@ TEST_F(TabRestoreServiceTest, ManyWindowsInSessionService) {
   ASSERT_EQ(1U, window->tabs[0].navigations.size());
   EXPECT_EQ(0, window->tabs[0].current_navigation_index);
   EXPECT_EQ(0, window->tabs[0].timestamp.ToInternalValue());
-  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].url());
+  EXPECT_TRUE(url1_ == window->tabs[0].navigations[0].virtual_url());
 }
 
 // Makes sure we restore the time stamp correctly.
@@ -475,4 +475,3 @@ TEST_F(TabRestoreServiceTest, TimestampSurvivesRestore) {
   EXPECT_EQ(tab_timestamp.ToInternalValue(),
             restored_tab->timestamp.ToInternalValue());
 }
-