Change NavigationEntry's title fields to carry the text direction.

Mark most of the users with a tag pointing at the bug, so they can
be fixed incrementally.

BUG=27094

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

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

10 files changed, 96 insertions, 64 deletions

diff --git a/content/browser/site_instance_unittest.cc b/content/browser/site_instance_unittest.cc
index b2cb51ca..12e269d 100644
--- a/content/browser/site_instance_unittest.cc
+++ b/content/browser/site_instance_unittest.cc
@@ -137,7 +137,7 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) {
   EXPECT_EQ(0, siteDeleteCounter);
 
   NavigationEntry* e1 = new NavigationEntry(instance, 0, url, GURL(),
-                                            string16(),
+                                            base::i18n::String16WithDirection(),
                                             PageTransition::LINK);
 
   // Redundantly setting e1's SiteInstance shouldn't affect the ref count.
@@ -145,8 +145,8 @@ TEST_F(SiteInstanceTest, SiteInstanceDestructor) {
   EXPECT_EQ(0, siteDeleteCounter);
 
   // Add a second reference
-  NavigationEntry* e2 = new NavigationEntry(instance, 0, url,
-                                            GURL(), string16(),
+  NavigationEntry* e2 = new NavigationEntry(instance, 0, url, GURL(),
+                                            base::i18n::String16WithDirection(),
                                             PageTransition::LINK);
 
   // Now delete both entries and be sure the SiteInstance goes away.
@@ -197,7 +197,7 @@ TEST_F(SiteInstanceTest, CloneNavigationEntry) {
                                                &browsingDeleteCounter);
 
   NavigationEntry* e1 = new NavigationEntry(instance1, 0, url, GURL(),
-                                            string16(),
+                                            base::i18n::String16WithDirection(),
                                             PageTransition::LINK);
   // Clone the entry
   NavigationEntry* e2 = new NavigationEntry(*e1);
diff --git a/content/browser/tab_contents/interstitial_page.cc b/content/browser/tab_contents/interstitial_page.cc
index c3d7291..269336c 100644
--- a/content/browser/tab_contents/interstitial_page.cc
+++ b/content/browser/tab_contents/interstitial_page.cc
@@ -270,7 +270,7 @@ void InterstitialPage::Hide() {
   // Let's revert to the original title if necessary.
   NavigationEntry* entry = tab_->controller().GetActiveEntry();
   if (!new_navigation_ && should_revert_tab_title_) {
-    entry->set_title(WideToUTF16Hack(original_tab_title_));
+    entry->set_title(original_tab_title_);
     tab_->NotifyNavigationStateChanged(TabContents::INVALIDATE_TITLE);
   }
   delete this;
@@ -405,7 +405,7 @@ void InterstitialPage::UpdateTitle(
   // If this interstitial is shown on an existing navigation entry, we'll need
   // to remember its title so we can revert to it when hidden.
   if (!new_navigation_ && !should_revert_tab_title_) {
-    original_tab_title_ = UTF16ToWideHack(entry->title());
+    original_tab_title_ = entry->title();
     should_revert_tab_title_ = true;
   }
   entry->set_title(title);
diff --git a/content/browser/tab_contents/interstitial_page.h b/content/browser/tab_contents/interstitial_page.h
index 7523050..759d8b3 100644
--- a/content/browser/tab_contents/interstitial_page.h
+++ b/content/browser/tab_contents/interstitial_page.h
@@ -9,6 +9,7 @@
 #include <map>
 #include <string>
 
+#include "base/i18n/rtl.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/process_util.h"
 #include "content/browser/renderer_host/render_view_host_delegate.h"
@@ -227,7 +228,7 @@ class InterstitialPage : public NotificationObserver,
 
   // The original title of the tab that should be reverted to when the
   // interstitial is hidden.
-  std::wstring original_tab_title_;
+  base::i18n::String16WithDirection original_tab_title_;
 
   // Our RenderViewHostViewDelegate, necessary for accelerators to work.
   scoped_ptr<InterstitialPageRVHViewDelegate> rvh_view_delegate_;
diff --git a/content/browser/tab_contents/navigation_controller.cc b/content/browser/tab_contents/navigation_controller.cc
index 310e1cf..0049d68 100644
--- a/content/browser/tab_contents/navigation_controller.cc
+++ b/content/browser/tab_contents/navigation_controller.cc
@@ -242,7 +242,7 @@ NavigationEntry* NavigationController::CreateNavigationEntry(
       -1,
       loaded_url,
       referrer,
-      string16(),
+      base::i18n::String16WithDirection(),
       transition);
   entry->set_virtual_url(url);
   entry->set_user_typed_url(url);
diff --git a/content/browser/tab_contents/navigation_entry.cc b/content/browser/tab_contents/navigation_entry.cc
index 01a8c77..099d2e4 100644
--- a/content/browser/tab_contents/navigation_entry.cc
+++ b/content/browser/tab_contents/navigation_entry.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -54,7 +54,7 @@ NavigationEntry::NavigationEntry(SiteInstance* instance,
                                  int page_id,
                                  const GURL& url,
                                  const GURL& referrer,
-                                 const string16& title,
+                                 const base::i18n::String16WithDirection& title,
                                  PageTransition::Type transition_type)
     : unique_id_(GetUniqueID()),
       site_instance_(instance),
@@ -76,16 +76,16 @@ void NavigationEntry::set_site_instance(SiteInstance* site_instance) {
   site_instance_ = site_instance;
 }
 
-const string16& NavigationEntry::GetTitleForDisplay(
+const base::i18n::String16WithDirection& NavigationEntry::GetTitleForDisplay(
     const std::string& languages) {
   // Most pages have real titles. Don't even bother caching anything if this is
   // the case.
-  if (!title_.empty())
+  if (!title_.is_empty())
     return title_;
 
   // More complicated cases will use the URLs as the title. This result we will
   // cache since it's more complicated to compute.
-  if (!cached_display_title_.empty())
+  if (!cached_display_title_.is_empty())
     return cached_display_title_;
 
   // Use the virtual URL first if any, and fall back on using the real URL.
@@ -103,7 +103,13 @@ const string16& NavigationEntry::GetTitleForDisplay(
       title = title.substr(slashpos + 1);
   }
 
-  ui::ElideString(title, content::kMaxTitleChars, &cached_display_title_);
+  string16 elided_title;
+  ui::ElideString(title, content::kMaxTitleChars, &elided_title);
+
+  // The computed title is a URL or a filename; assume it's LTR.
+  cached_display_title_ =
+      base::i18n::String16WithDirection(elided_title,
+                                        base::i18n::LEFT_TO_RIGHT);
   return cached_display_title_;
 }
 
diff --git a/content/browser/tab_contents/navigation_entry.h b/content/browser/tab_contents/navigation_entry.h
index 2a25f57..28ffa99 100644
--- a/content/browser/tab_contents/navigation_entry.h
+++ b/content/browser/tab_contents/navigation_entry.h
@@ -184,7 +184,7 @@ class NavigationEntry {
                   int page_id,
                   const GURL& url,
                   const GURL& referrer,
-                  const string16& title,
+                  const base::i18n::String16WithDirection& title,
                   PageTransition::Type transition_type);
   ~NavigationEntry();
 
@@ -227,7 +227,7 @@ class NavigationEntry {
   // the user.
   void set_url(const GURL& url) {
     url_ = url;
-    cached_display_title_.clear();
+    cached_display_title_ = base::i18n::String16WithDirection();
   }
   const GURL& url() const {
     return url_;
@@ -250,7 +250,7 @@ class NavigationEntry {
   // if there is no overridden display URL, it will return the actual one.
   void set_virtual_url(const GURL& url) {
     virtual_url_ = (url == url_) ? GURL() : url;
-    cached_display_title_.clear();
+    cached_display_title_ = base::i18n::String16WithDirection();
   }
   bool has_virtual_url() const {
     return !virtual_url_.is_empty();
@@ -271,15 +271,10 @@ class NavigationEntry {
   // displaying the appropriate "Untitled" label if this is being displayed to
   // the user.
   void set_title(const base::i18n::String16WithDirection& title) {
-    set_title(title.string());
-  }
-  // TODO(evan): remove the string16-setter once callers are updated.
-  // http://code.google.com/p/chromium/issues/detail?id=27094
-  void set_title(const string16& title) {
     title_ = title;
-    cached_display_title_.clear();
+    cached_display_title_ = base::i18n::String16WithDirection();
   }
-  const string16& title() const {
+  const base::i18n::String16WithDirection& title() const {
     return title_;
   }
 
@@ -330,7 +325,8 @@ class NavigationEntry {
   // the page if it is available or the URL. |languages| is the list of
   // accpeted languages (e.g., prefs::kAcceptLanguages) or empty if proper
   // URL formatting isn't needed (e.g., unit tests).
-  const string16& GetTitleForDisplay(const std::string& languages);
+  const base::i18n::String16WithDirection& GetTitleForDisplay(
+      const std::string& languages);
 
   // Returns true if the current tab is in view source mode. This will be false
   // if there is no navigation.
@@ -413,7 +409,7 @@ class NavigationEntry {
   GURL referrer_;
   GURL virtual_url_;
   bool update_virtual_url_with_url_;
-  string16 title_;
+  base::i18n::String16WithDirection title_;
   FaviconStatus favicon_;
   std::string content_state_;
   int32 page_id_;
@@ -427,7 +423,7 @@ class NavigationEntry {
   // us from having to do URL formatting on the URL evey time the title is
   // displayed. When the URL, virtual URL, or title is set, this should be
   // cleared to force a refresh.
-  string16 cached_display_title_;
+  base::i18n::String16WithDirection cached_display_title_;
 
   // Copy and assignment is explicitly allowed for this class.
 };
diff --git a/content/browser/tab_contents/navigation_entry_unittest.cc b/content/browser/tab_contents/navigation_entry_unittest.cc
index 9d5edc83..6fbc98f 100644
--- a/content/browser/tab_contents/navigation_entry_unittest.cc
+++ b/content/browser/tab_contents/navigation_entry_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -18,11 +18,13 @@ class NavigationEntryTest : public testing::Test {
     entry1_.reset(new NavigationEntry);
 
     instance_ = SiteInstance::CreateSiteInstance(NULL);
-    entry2_.reset(new NavigationEntry(instance_, 3,
-                                      GURL("test:url"),
-                                      GURL("from"),
-                                      ASCIIToUTF16("title"),
-                                      PageTransition::TYPED));
+    entry2_.reset(new NavigationEntry(
+        instance_, 3,
+        GURL("test:url"),
+        GURL("from"),
+        base::i18n::String16WithDirection(ASCIIToUTF16("title"),
+                                          base::i18n::LEFT_TO_RIGHT),
+        PageTransition::TYPED));
   }
 
   virtual void TearDown() {
@@ -53,23 +55,26 @@ TEST_F(NavigationEntryTest, NavigationEntryURLs) {
 
   EXPECT_EQ(GURL(), entry1_.get()->url());
   EXPECT_EQ(GURL(), entry1_.get()->virtual_url());
-  EXPECT_TRUE(entry1_.get()->GetTitleForDisplay("").empty());
+  EXPECT_TRUE(entry1_.get()->GetTitleForDisplay("").is_empty());
 
   // Setting URL affects virtual_url and GetTitleForDisplay
   entry1_.get()->set_url(GURL("http://www.google.com"));
   EXPECT_EQ(GURL("http://www.google.com"), entry1_.get()->url());
   EXPECT_EQ(GURL("http://www.google.com"), entry1_.get()->virtual_url());
   EXPECT_EQ(ASCIIToUTF16("www.google.com"),
-            entry1_.get()->GetTitleForDisplay(""));
+            entry1_.get()->GetTitleForDisplay("").string());
 
   // file:/// URLs should only show the filename.
   entry1_.get()->set_url(GURL("file:///foo/bar baz.txt"));
   EXPECT_EQ(ASCIIToUTF16("bar baz.txt"),
-            entry1_.get()->GetTitleForDisplay(""));
+            entry1_.get()->GetTitleForDisplay("").string());
 
   // Title affects GetTitleForDisplay
-  entry1_.get()->set_title(ASCIIToUTF16("Google"));
-  EXPECT_EQ(ASCIIToUTF16("Google"), entry1_.get()->GetTitleForDisplay(""));
+  entry1_.get()->set_title(
+      base::i18n::String16WithDirection(ASCIIToUTF16("Google"),
+                                        base::i18n::LEFT_TO_RIGHT));
+  EXPECT_EQ(ASCIIToUTF16("Google"),
+            entry1_.get()->GetTitleForDisplay("").string());
 
   // Setting virtual_url doesn't affect URL
   entry2_.get()->set_virtual_url(GURL("display:url"));
@@ -78,7 +83,8 @@ TEST_F(NavigationEntryTest, NavigationEntryURLs) {
   EXPECT_EQ(GURL("display:url"), entry2_.get()->virtual_url());
 
   // Having a title set in constructor overrides virtual URL
-  EXPECT_EQ(ASCIIToUTF16("title"), entry2_.get()->GetTitleForDisplay(""));
+  EXPECT_EQ(ASCIIToUTF16("title"),
+            entry2_.get()->GetTitleForDisplay("").string());
 
   // User typed URL is independent of the others
   EXPECT_EQ(GURL(), entry1_.get()->user_typed_url());
@@ -151,10 +157,12 @@ TEST_F(NavigationEntryTest, NavigationEntryAccessors) {
   EXPECT_EQ(GURL("from2"), entry2_.get()->referrer());
 
   // Title
-  EXPECT_EQ(string16(), entry1_.get()->title());
-  EXPECT_EQ(ASCIIToUTF16("title"), entry2_.get()->title());
-  entry2_.get()->set_title(ASCIIToUTF16("title2"));
-  EXPECT_EQ(ASCIIToUTF16("title2"), entry2_.get()->title());
+  EXPECT_EQ(string16(), entry1_.get()->title().string());
+  EXPECT_EQ(ASCIIToUTF16("title"), entry2_.get()->title().string());
+  entry2_.get()->set_title(
+      base::i18n::String16WithDirection(ASCIIToUTF16("title2"),
+                                        base::i18n::LEFT_TO_RIGHT));
+  EXPECT_EQ(ASCIIToUTF16("title2"), entry2_.get()->title().string());
 
   // State
   EXPECT_EQ(std::string(), entry1_.get()->content_state());
diff --git a/content/browser/tab_contents/render_view_host_manager_unittest.cc b/content/browser/tab_contents/render_view_host_manager_unittest.cc
index 3ff2092..7462e03 100644
--- a/content/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/content/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -176,7 +176,8 @@ TEST_F(RenderViewHostManagerTest, Navigate) {
   // 1) The first navigation. --------------------------
   GURL url1("http://www.google.com/");
   NavigationEntry entry1(NULL /* instance */, -1 /* page_id */, url1,
-                         GURL() /* referrer */, string16() /* title */,
+                         GURL() /* referrer */,
+                         base::i18n::String16WithDirection() /* title */,
                          PageTransition::TYPED);
   host = manager.Navigate(entry1);
 
@@ -195,7 +196,8 @@ TEST_F(RenderViewHostManagerTest, Navigate) {
   // 2) Navigate to next site. -------------------------
   GURL url2("http://www.google.com/foo");
   NavigationEntry entry2(NULL /* instance */, -1 /* page_id */, url2,
-                         url1 /* referrer */, string16() /* title */,
+                         url1 /* referrer */,
+                         base::i18n::String16WithDirection() /* title */,
                          PageTransition::LINK);
   host = manager.Navigate(entry2);
 
@@ -212,7 +214,8 @@ TEST_F(RenderViewHostManagerTest, Navigate) {
   // 3) Cross-site navigate to next site. --------------
   GURL url3("http://webkit.org/");
   NavigationEntry entry3(NULL /* instance */, -1 /* page_id */, url3,
-                         url2 /* referrer */, string16() /* title */,
+                         url2 /* referrer */,
+                         base::i18n::String16WithDirection() /* title */,
                          PageTransition::LINK);
   host = manager.Navigate(entry3);
 
@@ -247,7 +250,8 @@ TEST_F(RenderViewHostManagerTest, WebUI) {
 
   GURL url(chrome::kChromeUINewTabURL);
   NavigationEntry entry(NULL /* instance */, -1 /* page_id */, url,
-                        GURL() /* referrer */, string16() /* title */,
+                        GURL() /* referrer */,
+                        base::i18n::String16WithDirection() /* title */,
                         PageTransition::TYPED);
   RenderViewHost* host = manager.Navigate(entry);
 
@@ -285,7 +289,8 @@ TEST_F(RenderViewHostManagerTest, NonWebUIChromeURLs) {
   // NTP is a Web UI page.
   GURL ntp_url(chrome::kChromeUINewTabURL);
   NavigationEntry ntp_entry(NULL /* instance */, -1 /* page_id */, ntp_url,
-                            GURL() /* referrer */, string16() /* title */,
+                            GURL() /* referrer */,
+                            base::i18n::String16WithDirection() /* title */,
                             PageTransition::TYPED);
 
   // about: URLs are not Web UI pages.
@@ -295,7 +300,8 @@ TEST_F(RenderViewHostManagerTest, NonWebUIChromeURLs) {
   BrowserURLHandler::RewriteURLIfNecessary(
       &about_url, profile_.get(), &reverse_on_redirect);
   NavigationEntry about_entry(NULL /* instance */, -1 /* page_id */, about_url,
-                              GURL() /* referrer */, string16() /* title */,
+                              GURL() /* referrer */,
+                              base::i18n::String16WithDirection() /* title */,
                               PageTransition::TYPED);
 
   EXPECT_TRUE(ShouldSwapProcesses(&manager, &ntp_entry, &about_entry));
diff --git a/content/browser/tab_contents/tab_contents.cc b/content/browser/tab_contents/tab_contents.cc
index e5142a5..1b50925 100644
--- a/content/browser/tab_contents/tab_contents.cc
+++ b/content/browser/tab_contents/tab_contents.cc
@@ -446,8 +446,10 @@ const string16& TabContents::GetTitle() const {
   // that are shown on top of existing pages.
   NavigationEntry* entry = controller_.GetTransientEntry();
   if (entry) {
+    // TODO(evan): use directionality of title.
+    // http://code.google.com/p/chromium/issues/detail?id=27094
     return entry->GetTitleForDisplay(profile()->GetPrefs()->
-        GetString(prefs::kAcceptLanguages));
+        GetString(prefs::kAcceptLanguages)).string();
   }
   WebUI* our_web_ui = render_manager_.pending_web_ui() ?
       render_manager_.pending_web_ui() : render_manager_.web_ui();
@@ -456,6 +458,8 @@ const string16& TabContents::GetTitle() const {
     entry = controller_.GetActiveEntry();
     if (!(entry && entry->IsViewSourceMode())) {
       // Give the Web UI the chance to override our title.
+      // TODO(evan): use directionality of title.
+      // http://code.google.com/p/chromium/issues/detail?id=27094
       const string16& title = our_web_ui->overridden_title();
       if (!title.empty())
         return title;
@@ -468,8 +472,10 @@ const string16& TabContents::GetTitle() const {
   // title.
   entry = controller_.GetLastCommittedEntry();
   if (entry) {
+    // TODO(evan): use directionality of title.
+    // http://code.google.com/p/chromium/issues/detail?id=27094
     return entry->GetTitleForDisplay(profile()->GetPrefs()->
-        GetString(prefs::kAcceptLanguages));
+        GetString(prefs::kAcceptLanguages)).string();
   }
   return EmptyString16();
 }
@@ -1093,8 +1099,11 @@ void TabContents::UpdateHistoryPageTitle(const NavigationEntry& entry) {
     return;
 
   HistoryService* hs = profile()->GetHistoryService(Profile::IMPLICIT_ACCESS);
-  if (hs)
-    hs->SetPageTitle(entry.virtual_url(), entry.title());
+  if (hs) {
+    // TODO(evan): use directionality of title.
+    // http://code.google.com/p/chromium/issues/detail?id=27094
+    hs->SetPageTitle(entry.virtual_url(), entry.title().string());
+  }
 }
 
 double TabContents::GetZoomLevel() const {
@@ -1562,18 +1571,23 @@ TabContents::CreateHistoryAddPageArgs(
   return add_page_args;
 }
 
-bool TabContents::UpdateTitleForEntry(NavigationEntry* entry,
-                                      const std::wstring& title) {
+bool TabContents::UpdateTitleForEntry(
+    NavigationEntry* entry,
+    const base::i18n::String16WithDirection& title) {
   // For file URLs without a title, use the pathname instead. In the case of a
   // synthesized title, we don't want the update to count toward the "one set
   // per page of the title to history."
-  string16 final_title;
+  base::i18n::String16WithDirection final_title;
   bool explicit_set;
-  if (entry->url().SchemeIsFile() && title.empty()) {
-    final_title = UTF8ToUTF16(entry->url().ExtractFileName());
+  if (entry->url().SchemeIsFile() && title.is_empty()) {
+    final_title = base::i18n::String16WithDirection(
+        UTF8ToUTF16(entry->url().ExtractFileName()),
+        base::i18n::LEFT_TO_RIGHT);
     explicit_set = false;  // Don't count synthetic titles toward the set limit.
   } else {
-    TrimWhitespace(WideToUTF16Hack(title), TRIM_ALL, &final_title);
+    string16 trimmed;
+    TrimWhitespace(title.string(), TRIM_ALL, &trimmed);
+    final_title = base::i18n::String16WithDirection(trimmed, title.direction());
     explicit_set = true;
   }
 
@@ -1588,7 +1602,9 @@ bool TabContents::UpdateTitleForEntry(NavigationEntry* entry,
   }
 
   // Lastly, set the title for the view.
-  view_->SetPageTitle(UTF16ToWideHack(final_title));
+  // TODO(evan): use directionality of title.
+  // http://code.google.com/p/chromium/issues/detail?id=27094
+  view_->SetPageTitle(UTF16ToWide(final_title.string()));
 
   NotificationService::current()->Notify(
       NotificationType::TAB_CONTENTS_TITLE_UPDATED,
@@ -1894,9 +1910,7 @@ void TabContents::UpdateTitle(RenderViewHost* rvh,
   DCHECK(rvh == render_view_host());
   NavigationEntry* entry = controller_.GetEntryWithPageID(rvh->site_instance(),
                                                           page_id);
-  // TODO(evan): use directionality of title.
-  // http://code.google.com/p/chromium/issues/detail?id=27094
-  if (!entry || !UpdateTitleForEntry(entry, UTF16ToWide(title.string())))
+  if (!entry || !UpdateTitleForEntry(entry, title))
     return;
 
   // Broadcast notifications when the UI should be updated.
diff --git a/content/browser/tab_contents/tab_contents.h b/content/browser/tab_contents/tab_contents.h
index 59277fc..42c6139 100644
--- a/content/browser/tab_contents/tab_contents.h
+++ b/content/browser/tab_contents/tab_contents.h
@@ -745,7 +745,8 @@ class TabContents : public PageNavigator,
   // This is used as the backend for state updates, which include a new title,
   // or the dedicated set title message. It returns true if the new title is
   // different and was therefore updated.
-  bool UpdateTitleForEntry(NavigationEntry* entry, const std::wstring& title);
+  bool UpdateTitleForEntry(NavigationEntry* entry,
+                           const base::i18n::String16WithDirection& title);
 
   // Causes the TabContents to navigate in the right renderer to |entry|, which
   // must be already part of the entries in the navigation controller.