Elides the beginning of tab titles that have common prefixes. ...

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

Index: chrome/browser/ui/views/tabs/base_tab_strip.cc
===================================================================
--- chrome/browser/ui/views/tabs/base_tab_strip.cc	(revision 75998)
+++ chrome/browser/ui/views/tabs/base_tab_strip.cc	(working copy)
@@ -4,10 +4,16 @@
 
 #include "chrome/browser/ui/views/tabs/base_tab_strip.h"
 
+#include <vector>
+
+#include "base/command_line.h"
+#include "base/hash_tables.h"
 #include "base/logging.h"
+#include "base/string_split.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/tabs/dragged_tab_controller.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
+#include "chrome/common/chrome_switches.h"
 #include "views/widget/root_view.h"
 #include "views/window/window.h"
 
@@ -145,6 +151,7 @@
 
   TabData d = { tab, gfx::Rect() };
   tab_data_.insert(tab_data_.begin() + ModelIndexToTabIndex(model_index), d);
+  UpdateCommonTitlePrefix();
 
   AddChildView(tab);
 
@@ -175,6 +182,7 @@
   BaseTab* tab = GetBaseTabAtModelIndex(model_index);
   bool mini_state_changed = tab->data().mini != data.mini;
   tab->SetData(data);
+  UpdateCommonTitlePrefix();
   tab->SchedulePaint();
 
   if (mini_state_changed) {
@@ -404,8 +412,94 @@
   tab_data_.erase(tab_data_.begin() + tab_data_index);
 
   delete tab;
+  UpdateCommonTitlePrefix();
 }
 
+void BaseTabStrip::UpdateCommonTitlePrefix() {
+  // Hidden behind a command line flag until we want to enable it always.
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kElideTabTitlePrefix)) {
+    return;
+  }
+
+  // First, we need to identify if there are identical titles,
+  // because we don't want to remove prefixes for those at all.
+  // We do it as a separate pass so that we don't need to remove
+  // previously parsed titles when we find a duplicate title later on.
+  // This set will contain the indexes of the tab that have a duplicate.
+  base::hash_set<int> no_prefix_tab;
+  // This map is used to remember the existing title and the index of the
+  // tab we saw them in first, once we put this other index in the

[sky, 2011/02/25 17:02:07]

This is a bit confusing. I think you mean:
'This map is used to remember the title and the index of a tab. If multiple tabs
have the same title the index is set to -1 and the indices of all tabs sharing
the same title are placed in no_prefix_tab.'

[MAD, 2011/02/25 20:09:13]

Thanks... Done...

+  // no_prefix_tab set, we set the index to -1.
+  base::hash_map<string16, int> existing_title;
+  for (int tab_index = 0; tab_index < tab_count(); ++tab_index) {
+    DCHECK(tab_data_[tab_index].tab != NULL);

[sky, 2011/02/25 17:02:07]

Shouldn't you skip over mini tabs?

[MAD, 2011/02/25 20:09:13]

Good point, it would save some processing time since we ignore them later
anyway... Done...

[sky, 2011/02/25 21:04:44]

I wasn't so much concerned about that, but rather if you consider the title here
you might incorrectly think a title was duplicated.

[MAD, 2011/02/25 21:49:24]

Yes, that too... :-)
Fixed now, right?

[sky, 2011/02/25 22:59:27]

Yes, thanks.

+    // We use pairs to test existence and insert in one shot.
+    std::pair<base::hash_map<string16, int>::iterator, bool> insert_result =
+        existing_title.insert(
+            std::make_pair(tab_data_[tab_index].tab->data().title, tab_index));
+    if (!insert_result.second) {
+      DCHECK(tab_data_[tab_index].tab->data().title ==

[sky, 2011/02/25 17:02:07]

This code is tricky to read, how about a comment here saying the title is a
duplicate.

[MAD, 2011/02/25 20:09:13]

Done...

+             insert_result.first->first);
+      no_prefix_tab.insert(tab_index);
+      if (insert_result.first->second != -1) {
+        no_prefix_tab.insert(insert_result.first->second);
+        insert_result.first->second = -1;
+      }
+    }
+  }
+
+  // This next loop accumulates all the potential prefixes,
+  // and remember on which tabs we saw them.
+  base::hash_map<string16, std::vector<size_t> > prefixes;

[sky, 2011/02/25 17:02:07]

Shouldn't this be std::vector<int> ?

[MAD, 2011/02/25 20:09:13]

Ho, right, thanks again... Done...

+  for (int tab_index = 0; tab_index < tab_count(); ++tab_index) {
+    const TabRendererData& tab_data = tab_data_[tab_index].tab->data();
+    // Mini, title-less, and duplicate title tabs
+    // are not to be included in this process.
+    if (tab_data.mini || tab_data.title.empty() ||
+        no_prefix_tab.find(tab_index) != no_prefix_tab.end()) {
+      continue;
+    }
+
+    // We only create prefixes at word boundaries.
+    std::vector<string16> words;
+    base::SplitStringAlongWhitespace(tab_data.title, &words);

[Peter Kasting, 2011/02/25 17:14:44]

This is the wrong function to use.  We need to use ICU functions to find logical
boundaries as some languages don't have whitespace.

[MAD, 2011/02/25 20:09:13]

Will do in the next iteration, when I move the code to a common place where it
will be available to other platforms.

+    size_t end_of_word = 0;
+    // We don't need the last word, since we ignored duplicate titles above.

[sky, 2011/02/25 17:02:07]

Maybe I'm just dense, but this comment doesn't make sense to me.

[MAD, 2011/02/25 20:09:13]

Sorry, maybe my wording is wrong (unintended pun :-)...
What I mean is that we don't have to add an entry in the prefixes map for the
concatenation of all the words, since a match on the whole title is ignored
anyway... I reworded the comment...

+    for (size_t word_index = 0; word_index < words.size() - 1; ++word_index) {

[sky, 2011/02/25 17:02:07]

If words is empty, this is never stops. This might be possible if the title only
contained white space. Either way, you should handle that case here.

[MAD, 2011/02/25 20:09:13]

Hum, I thought I did handle that case by exiting the loop early for empty
titles, and I was under the false impression that I would get empty strings in
the words vector for contiguous white spaces, I see my mistake now, good catch!

Fixed...

+      if (!words[word_index].empty()) {
+        end_of_word = tab_data.title.find(words[word_index], end_of_word) +

[sky, 2011/02/25 17:02:07]

It seems horribly inefficient to have to use find again when presumably
SplitStringAlongWhitespace knows all this already.

[MAD, 2011/02/25 20:09:13]

Yeah, I know, I was debating whether I should just rewrite my own splitter
function, and decided to leave it like this... Now that I need to change it for
an ICU regexp for word breaks, I'll try to avoid the find if I can...

TODO...

+                      words[word_index].size();
+        prefixes[tab_data.title.substr(0, end_of_word)].push_back(tab_index);
+      }
+    }
+  }
+
+  // Now we parse the map to find common prefixes and set the largest per tab.
+  std::vector<size_t> prefix_lengths(tab_count(), 0);
+  base::hash_map<string16, std::vector<size_t> >::iterator iter =

[sky, 2011/02/25 17:02:07]

Move the iter into the for declaration so that its clear the iterator is only
used in the for loop.

[MAD, 2011/02/25 20:09:13]

Done...

+      prefixes.begin();
+  for (; iter != prefixes.end(); ++iter) {
+    if (iter->second.size() > 1) {  // Need more than one with same prefix.
+      size_t prefix_length = iter->first.size();
+      for (size_t index = 0; index < iter->second.size(); ++index){
+        if (prefix_lengths[iter->second[index]] < prefix_length) {
+          prefix_lengths[iter->second[index]] = prefix_length;
+        }
+      }
+    }
+  }
+
+  // And finally, reset the tab data for the tabs that changed.
+  for (int tab_index = 0; tab_index < tab_count(); ++tab_index) {
+    TabRendererData data = tab_data_[tab_index].tab->data();
+    if ((size_t)data.common_prefix_length != prefix_lengths[tab_index]) {

[sky, 2011/02/25 17:02:07]

static_cast

[MAD, 2011/02/25 20:09:13]

Oops... I meant to fix that and forgot... Sorry about that...
But I don't need it anymore, now that I changed the type of common_prefix_length
in the next file...

+      data.common_prefix_length = prefix_lengths[tab_index];
+      tab_data_[tab_index].tab->SetData(data);

[sky, 2011/02/25 17:02:07]

I think you need a SchedulePaint here.

[MAD, 2011/02/25 20:09:13]

I was wondering about that, but the SetData implementation actually does it...
Should I add it anyway?

[sky, 2011/02/25 21:04:44]

It does? It probably should, but I don't think it currently does.

[MAD, 2011/02/25 21:49:24]

Actually, it indirectly does, via Layout, which does a SchedulePaint when we
have a layout manager. Should I had one here still?

[sky, 2011/02/25 22:59:27]

BaseTab doesn't have a LayoutManager though.

+    }
+  }
+}
+
 int BaseTabStrip::TabIndexOfTab(BaseTab* tab) const {
   for (int i = 0; i < tab_count(); ++i) {
     if (base_tab_at_tab_index(i) == tab)