Improvements to tab title prefix eliding as per email discussions.

- Don't look for common title prefixes between pages from different hosts.

- Bring down the minimal width of a tab title area from 10 to 6 chars.

- Don't elide last 4 chars of the common prefix, and also don't elide if there is less than 7 common characters.

BUG=None
TEST=Try it!
Review URL: http://codereview.chromium.org/6783015

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

chrome/browser/ui/cocoa/tabs/tab_controller.h

@@ -9,6 +9,7 @@
 #import <Cocoa/Cocoa.h>
 #import "chrome/browser/ui/cocoa/hover_close_button.h"
 #include "chrome/browser/ui/tabs/tab_menu_model.h"
+#include "googleurl/src/gurl.h"
 
 // The loading/waiting state of the tab.
 enum TabLoadingState {
@@ -49,6 +50,7 @@ class MenuDelegate;
   BOOL mini_;
   BOOL pinned_;
   BOOL selected_;
+  GURL url_;
   TabLoadingState loadingState_;
   CGFloat iconTitleXOffset_;  // between left edges of icon and title
   id<TabControllerTarget> target_;  // weak, where actions are sent
@@ -66,6 +68,7 @@ class MenuDelegate;
 @property(assign, nonatomic) BOOL pinned;
 @property(assign, nonatomic) BOOL selected;
 @property(assign, nonatomic) id target;
+@property(assign, nonatomic) GURL url;
 @property(assign, nonatomic) NSView* iconView;
 @property(assign, nonatomic) NSTextField* titleView;
 @property(assign, nonatomic) HoverCloseButton* closeButton;

chrome/browser/ui/cocoa/tabs/tab_controller.mm

@@ -9,6 +9,7 @@
 #import "chrome/browser/ui/cocoa/tabs/tab_controller_target.h"
 #import "chrome/browser/ui/cocoa/tabs/tab_view.h"
 #import "chrome/browser/ui/cocoa/themed_window.h"
+#include "chrome/browser/ui/title_prefix_matcher.h"
 #import "chrome/common/extensions/extension.h"
 #include "grit/generated_resources.h"
 #import "third_party/GTM/AppKit/GTMFadeTruncatingTextFieldCell.h"
@@ -22,6 +23,7 @@
 @synthesize mini = mini_;
 @synthesize pinned = pinned_;
 @synthesize target = target_;
+@synthesize url = url_;
 @synthesize iconView = iconView_;
 @synthesize titleView = titleView_;
 @synthesize closeButton = closeButton_;
@@ -308,9 +310,10 @@ class MenuDelegate : public ui::SimpleMenuModel::Delegate {
   DCHECK([[titleView_ cell] isKindOfClass:
       [GTMFadeTruncatingTextFieldCell class]]);
   GTMFadeTruncatingTextFieldCell* cell = [titleView_ cell];
-  [cell setDesiredCharactersToTruncateFromHead:length];
-  [cell setTruncateMode:length > 0 ? GTMFadeTruncatingHeadAndTail :
-                                     GTMFadeTruncatingTail];
+  [cell setDesiredCharactersToTruncateFromHead:length -
+      TitlePrefixMatcher::kCommonCharsToShow];
+  [cell setTruncateMode:length > TitlePrefixMatcher::kMinElidingLength ?
+      GTMFadeTruncatingHeadAndTail : GTMFadeTruncatingTail];
 }
 
 @end

chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm

@@ -1104,6 +1104,7 @@ class NotificationBridge : public NotificationObserver {
   [newController setMini:tabStripModel_->IsMiniTab(modelIndex)];
   [newController setPinned:tabStripModel_->IsTabPinned(modelIndex)];
   [newController setApp:tabStripModel_->IsAppTab(modelIndex)];
+  [newController setUrl:contents->tab_contents()->GetURL()];
   [tabArray_ insertObject:newController atIndex:index];
   NSView* newView = [newController view];
 
@@ -1501,6 +1502,7 @@ class NotificationBridge : public NotificationObserver {
   [tabController setMini:tabStripModel_->IsMiniTab(modelIndex)];
   [tabController setPinned:tabStripModel_->IsTabPinned(modelIndex)];
   [tabController setApp:tabStripModel_->IsAppTab(modelIndex)];
+  [tabController setUrl:contents->tab_contents()->GetURL()];
   [self updateFaviconForContents:contents->tab_contents() atIndex:modelIndex];
   // If the tab is being restored and it's pinned, the mini state is set after
   // the tab has already been rendered, so re-layout the tabstrip. In all other
@@ -2043,7 +2045,7 @@ class NotificationBridge : public NotificationObserver {
     if (!title.empty() && ![tabController mini]) {
       titles.push_back(new string16(title));
       tabTitleInfos.push_back(TitlePrefixMatcher::TitleInfo(
-          titles[titles.size() - 1], tabIndex));
+          titles[titles.size() - 1], [tabController url], tabIndex));
     }
   }
 

chrome/browser/ui/title_prefix_matcher.cc

@@ -7,6 +7,7 @@
 #include "base/hash_tables.h"
 #include "base/i18n/break_iterator.h"
 #include "base/logging.h"
+#include "base/utf_string_conversions.h"
 
 namespace {
 // We use this value to identify that we have already seen the title associated
@@ -14,14 +15,23 @@ namespace {
 const size_t kPreviouslySeenIndex = 0xFFFFFFFF;
 }
 
-TitlePrefixMatcher::TitleInfo::TitleInfo(const string16* title,
-                                         int caller_value)
+// static
+const int TitlePrefixMatcher::kCommonCharsToShow = 4;
+const size_t TitlePrefixMatcher::kMinElidingLength =
+    TitlePrefixMatcher::kCommonCharsToShow + 3;
+
+TitlePrefixMatcher::TitleInfo::TitleInfo(
+    const string16* title, const GURL& url, int caller_value)
     : title(title),
+      url(url),
       prefix_length(0),
       caller_value(caller_value) {
   DCHECK(title != NULL);
 }
 
+TitlePrefixMatcher::TitleInfo::~TitleInfo() {
+}
+
 // static
 void TitlePrefixMatcher::CalculatePrefixLengths(
     std::vector<TitleInfo>* title_infos) {
@@ -41,12 +51,12 @@ void TitlePrefixMatcher::CalculatePrefixLengths(
   for (size_t i = 0; i < title_infos->size(); ++i) {
     // We use pairs to test existence and insert in one shot.
     std::pair<base::hash_map<string16, size_t>::iterator, bool> insert_result =
-        existing_title.insert(std::make_pair(*title_infos->at(i).title, i));
+        existing_title.insert(std::make_pair(*(*title_infos)[i].title, i));
     if (!insert_result.second) {
       // insert_result.second is false when we insert a duplicate in the set.
       // insert_result.first is a map iterator and thus
       // insert_result.first->first is the string title key of the map.
-      DCHECK(*title_infos->at(i).title == insert_result.first->first);
+      DCHECK_EQ(*(*title_infos)[i].title, insert_result.first->first);
       duplicate_titles.insert(i);
       // insert_result.first->second is the value of the title index and if it's
       // not kPreviouslySeenIndex yet, we must remember it as a duplicate too.
@@ -64,7 +74,11 @@ void TitlePrefixMatcher::CalculatePrefixLengths(
     // Duplicate titles are not to be included in this process.
     if (duplicate_titles.find(i) != duplicate_titles.end())
       continue;
-    const string16* title = title_infos->at(i).title;
+    const TitleInfo& title_info = (*title_infos)[i];
+    const string16* title = title_info.title;
+    // We prefix the hostname at the beginning, so that we only group
+    // titles that are from the same hostname.
+    string16 hostname = ASCIIToUTF16(title_info.url.host());
     // We only create prefixes at word boundaries.
     base::BreakIterator iter(title, base::BreakIterator::BREAK_WORD);
     // We ignore this title if we can't break it into words, or if it only
@@ -76,7 +90,7 @@ void TitlePrefixMatcher::CalculatePrefixLengths(
     // previous word and more easily ignore the last word while iterating.
     while (iter.Advance()) {
       if (iter.IsWord())
-        prefixes[title->substr(0, iter.prev())].push_back(i);
+        prefixes[hostname + title->substr(0, iter.prev())].push_back(i);
     }
   }
 
@@ -86,10 +100,16 @@ void TitlePrefixMatcher::CalculatePrefixLengths(
        prefixes.begin(); iter != prefixes.end(); ++iter) {
     // iter->first is the prefix string, iter->second is a vector of indices.
     if (iter->second.size() > 1) {
-      size_t prefix_length = iter->first.size();
+      // We need to subtract the hostname size since we added it to the prefix.
+      const TitleInfo& first_title_info = (*title_infos)[iter->second[0]];
+      DCHECK_GE(iter->first.size(), first_title_info.url.host().size());
+      size_t prefix_length = iter->first.size() -
+          first_title_info.url.host().size();
       for (size_t i = 0; i < iter->second.size(); ++i){
-        if (title_infos->at(iter->second[i]).prefix_length < prefix_length)
-          title_infos->at(iter->second[i]).prefix_length = prefix_length;
+        TitleInfo& title_info = (*title_infos)[iter->second[i]];
+        DCHECK_EQ(first_title_info.url.host(), title_info.url.host());
+        if (title_info.prefix_length < prefix_length)
+          title_info.prefix_length = prefix_length;
       }
     }
   }

chrome/browser/ui/title_prefix_matcher.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/string16.h"
+#include "googleurl/src/gurl.h"
 
 // This class exposes a static method that receives a vector of TitleInfo
 // objects so that it can find the length of the common prefixes among all
@@ -26,11 +27,14 @@
 class TitlePrefixMatcher {
  public:
   struct TitleInfo {
-    TitleInfo(const string16* title, int caller_value);
+    TitleInfo(const string16* title, const GURL& url, int caller_value);
+    ~TitleInfo();
     // We assume the title string will be valid throughout the execution of
     // the prefix lengths calculation, and so we use a pointer to avoid an
     // unnecessary string copy.
     const string16* title;
+    // We only look for common prefix when the URL has the same hostname.
+    GURL url;
     // This contains the number of characters at the beginning of title that
     // are common with other titles in the TitleInfo vector.
     size_t prefix_length;
@@ -39,6 +43,12 @@ class TitlePrefixMatcher {
   };
   static void CalculatePrefixLengths(std::vector<TitleInfo>* title_infos);
 
+  // We want to show the last few common chars of a page title in the tab,
+  // so we only do it if the common prefix is at least kMinElidingLength,
+  // otherwise, we could be replacing less characters than the ellipsis take.
+  static const int kCommonCharsToShow;
+  static const size_t kMinElidingLength;
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(TitlePrefixMatcher);
 };

chrome/browser/ui/title_prefix_matcher_unittest.cc

@@ -5,10 +5,15 @@
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/title_prefix_matcher.h"
+#include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
-
+const GURL kUrlA1("http://a.b.c/123");
+const GURL kUrlA2("http://a.b.c/alphabits");
+const GURL kUrlB1("http://www.here.com/here/and/there");
+const GURL kUrlB2("http://www.here.com/elsewhere");
+const GURL kUrlC1("http://www.here.ca/far/far/away");
 const string16 kFoofooAbcdef(ASCIIToUTF16("Foofoo abcdef"));
 const string16 kFoofooAbcdeg(ASCIIToUTF16("Foofoo abcdeg"));
 const string16 kFooAbcdef(ASCIIToUTF16("Foo abcdef"));
@@ -23,8 +28,10 @@ const string16 kFoo(ASCIIToUTF16("Foo"));
 
 TEST(TitlePrefixMatcherTest, BasicTests) {
   std::vector<TitlePrefixMatcher::TitleInfo> tab_title_infos;
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef, 0));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg, 1));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef,
+                                                          kUrlA1, 0));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg,
+                                                          kUrlA1, 1));
 
   TitlePrefixMatcher::CalculatePrefixLengths(&tab_title_infos);
   EXPECT_EQ(0, tab_title_infos[0].caller_value);
@@ -34,10 +41,14 @@ TEST(TitlePrefixMatcherTest, BasicTests) {
   EXPECT_EQ(7U, tab_title_infos[1].prefix_length);
 
   tab_title_infos.clear();
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef, 0));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg, 1));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef, 2));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg, 3));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef,
+                                                          kUrlA1, 0));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg,
+                                                          kUrlA1, 1));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef,
+                                                          kUrlA1, 2));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg,
+                                                          kUrlA1, 3));
 
   TitlePrefixMatcher::CalculatePrefixLengths(&tab_title_infos);
   EXPECT_EQ(0, tab_title_infos[0].caller_value);
@@ -55,11 +66,16 @@ TEST(TitlePrefixMatcherTest, BasicTests) {
 
 TEST(TitlePrefixMatcherTest, Duplicates) {
   std::vector<TitlePrefixMatcher::TitleInfo> tab_title_infos;
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef, 0));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg, 1));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef, 2));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg, 3));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef, 4));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef,
+                                                          kUrlA1, 0));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg,
+                                                          kUrlA1, 1));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef,
+                                                          kUrlA1, 2));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg,
+                                                          kUrlA1, 3));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef,
+                                                          kUrlA1, 4));
 
   TitlePrefixMatcher::CalculatePrefixLengths(&tab_title_infos);
   EXPECT_EQ(0, tab_title_infos[0].caller_value);
@@ -80,13 +96,20 @@ TEST(TitlePrefixMatcherTest, Duplicates) {
 
 TEST(TitlePrefixMatcherTest, MultiplePrefixes) {
   std::vector<TitlePrefixMatcher::TitleInfo> tab_title_infos;
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef, 0));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg, 1));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbcDef, 2));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbcDeg, 3));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbdDef, 4));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBar, 5));
-  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoo, 6));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef,
+                                                          kUrlA1, 0));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg,
+                                                          kUrlA1, 1));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbcDef,
+                                                          kUrlA1, 2));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbcDeg,
+                                                          kUrlA1, 3));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbdDef,
+                                                          kUrlA1, 4));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBar,
+                                                          kUrlA1, 5));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoo,
+                                                          kUrlA1, 6));
 
   TitlePrefixMatcher::CalculatePrefixLengths(&tab_title_infos);
   EXPECT_EQ(0, tab_title_infos[0].caller_value);
@@ -110,3 +133,43 @@ TEST(TitlePrefixMatcherTest, MultiplePrefixes) {
   EXPECT_EQ(6, tab_title_infos[6].caller_value);
   EXPECT_EQ(0U, tab_title_infos[6].prefix_length);
 }
+
+TEST(TitlePrefixMatcherTest, DifferentHosts) {
+  std::vector<TitlePrefixMatcher::TitleInfo> tab_title_infos;
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdef,
+                                                          kUrlA1, 0));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFoofooAbcdeg,
+                                                          kUrlA2, 1));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdef,
+                                                          kUrlB1, 2));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbdDef,
+                                                          kUrlC1, 3));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbcDef,
+                                                          kUrlA1, 4));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kBarAbcDeg,
+                                                          kUrlA2, 5));
+  tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(&kFooAbcdeg,
+                                                          kUrlB2, 6));
+
+  TitlePrefixMatcher::CalculatePrefixLengths(&tab_title_infos);
+  EXPECT_EQ(0, tab_title_infos[0].caller_value);
+  EXPECT_EQ(7U, tab_title_infos[0].prefix_length);
+
+  EXPECT_EQ(1, tab_title_infos[1].caller_value);
+  EXPECT_EQ(7U, tab_title_infos[1].prefix_length);
+
+  EXPECT_EQ(2, tab_title_infos[2].caller_value);
+  EXPECT_EQ(4U, tab_title_infos[2].prefix_length);
+
+  EXPECT_EQ(3, tab_title_infos[3].caller_value);
+  EXPECT_EQ(0U, tab_title_infos[3].prefix_length);
+
+  EXPECT_EQ(4, tab_title_infos[4].caller_value);
+  EXPECT_EQ(8U, tab_title_infos[4].prefix_length);
+
+  EXPECT_EQ(5, tab_title_infos[5].caller_value);
+  EXPECT_EQ(8U, tab_title_infos[5].prefix_length);
+
+  EXPECT_EQ(6, tab_title_infos[6].caller_value);
+  EXPECT_EQ(4U, tab_title_infos[6].prefix_length);
+}

chrome/browser/ui/views/tabs/base_tab.cc

@@ -10,6 +10,7 @@
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
+#include "chrome/browser/ui/title_prefix_matcher.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/tabs/tab_controller.h"
 #include "chrome/common/chrome_switches.h"
@@ -479,11 +480,14 @@ void BaseTab::PaintTitle(gfx::Canvas* canvas, SkColor title_color) {
     Browser::FormatTitleForDisplay(&title);
     // If we'll need to truncate, check if we should also truncate
     // a common prefix, but only if there is enough room for it.
-    // We arbitrarily choose to request enough room for 10 average chars.
-    if (data().common_prefix_length > 0 &&
-        font_->GetExpectedTextWidth(10) < title_bounds.width() &&
+    // We arbitrarily choose to request enough room for 6 average chars.
+    if (data().common_prefix_length > TitlePrefixMatcher::kMinElidingLength &&
+        font_->GetExpectedTextWidth(6) < title_bounds.width() &&
         font_->GetStringWidth(title) > title_bounds.width()) {
-      title.replace(0, data().common_prefix_length, UTF8ToUTF16(ui::kEllipsis));
+      title.replace(0,
+                    data().common_prefix_length -
+                        TitlePrefixMatcher::kCommonCharsToShow,
+                    UTF8ToUTF16(ui::kEllipsis));
     }
   }
   canvas->DrawStringInt(title, *font_, title_color,

chrome/browser/ui/views/tabs/base_tab_strip.cc

@@ -476,7 +476,9 @@ void BaseTabStrip::UpdateCommonTitlePrefix() {
     DCHECK(tab_data_[tab_index].tab != NULL);
     if (!IgnoreTitlePrefixEliding(tab_data_[tab_index].tab)) {
       tab_title_infos.push_back(TitlePrefixMatcher::TitleInfo(
-          &tab_data_[tab_index].tab->data().title, tab_index));
+          &tab_data_[tab_index].tab->data().title,
+          tab_data_[tab_index].tab->data().url,
+          tab_index));
     }
   }
   TitlePrefixMatcher::CalculatePrefixLengths(&tab_title_infos);

chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc

@@ -403,6 +403,7 @@ void BrowserTabStripController::SetTabRendererDataFromModel(
     data->favicon = contents->GetFavicon();
   data->network_state = TabContentsNetworkState(contents);
   data->title = contents->GetTitle();
+  data->url = contents->GetURL();
   data->loading = contents->is_loading();
   data->crashed_status = contents->crashed_status();
   data->incognito = contents->profile()->IsOffTheRecord();

chrome/browser/ui/views/tabs/tab_renderer_data.cc

@@ -25,6 +25,7 @@ bool TabRendererData::Equals(const TabRendererData& data) {
       favicon.pixelRefOffset() == data.favicon.pixelRefOffset() &&
       network_state == data.network_state &&
       title == data.title &&
+      url == data.url &&
       common_prefix_length == data.common_prefix_length &&
       loading == data.loading &&
       crashed_status == data.crashed_status &&

chrome/browser/ui/views/tabs/tab_renderer_data.h

@@ -8,6 +8,7 @@
 
 #include "base/process_util.h"
 #include "base/string16.h"
+#include "googleurl/src/gurl.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 // Wraps the state needed by the renderers.
@@ -40,6 +41,7 @@ struct TabRendererData {
   SkBitmap favicon;
   NetworkState network_state;
   string16 title;
+  GURL url;
   // Identifies the number of chars at the beginning of the string
   // that are common to other tab titles.
   size_t common_prefix_length;