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

Elides the beginning of tab titles that have common prefixes. 

BUG=69304
TEST=Make sure the tab titles are displayed as spec'd, and that there isn't any performance issues.
 

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77860

[MAD, 2011-02-25 04:15:43]

I think you are the one that most recently (and more often) touched the code I
modified here, but feel free to redirect this code review to whoever you think
would be more appropriate...

Thanks!

BYE
MAD

[MAD, 2011-02-25 14:47:02]

On 2011/02/25 04:15:43, MAD wrote:
> I think you are the one that most recently (and more often) touched the code I
> modified here, but feel free to redirect this code review to whoever you think
> would be more appropriate...
> 
> Thanks!
> 
> BYE
> MAD

Note that the Linux try that failed in patch set 4, we retried and succeeded
(but didn't update the result here, I thought it would)
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/14120

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:268: kOsAll,
Your patch only effects windows and chromeos.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:432: // tab we saw them in first,
once we put this other index in the
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.'

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
DCHECK(tab_data_[tab_index].tab != NULL);
Shouldn't you skip over mini tabs?

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:442:
DCHECK(tab_data_[tab_index].tab->data().title ==
This code is tricky to read, how about a comment here saying the title is a
duplicate.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:454: base::hash_map<string16,
std::vector<size_t> > prefixes;
Shouldn't this be std::vector<int> ?

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:468: // We don't need the last
word, since we ignored duplicate titles above.
Maybe I'm just dense, but this comment doesn't make sense to me.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:469: for (size_t word_index = 0;
word_index < words.size() - 1; ++word_index) {
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.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:471: end_of_word =
tab_data.title.find(words[word_index], end_of_word) +
It seems horribly inefficient to have to use find again when presumably
SplitStringAlongWhitespace knows all this already.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:480: base::hash_map<string16,
std::vector<size_t> >::iterator iter =
Move the iter into the for declaration so that its clear the iterator is only
used in the for loop.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:496: if
((size_t)data.common_prefix_length != prefix_lengths[tab_index]) {
static_cast

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:498:
tab_data_[tab_index].tab->SetData(data);
I think you need a SchedulePaint here.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.h (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.h:199: virtual void
UpdateCommonTitlePrefix();
Why is this virtual?

Also, this class is only used on windows and chromeos. We're going to want this
feature on other platforms too (sadly). This function should be extracted into a
place that can be used on other platforms. I'm thinking you create some class
like PrefixMatcher or PrefixFinder or TabTitlePrefixBlahBlahBlah (you get the
idea) in chrome/browser/ui. The class has a single static method,
CalculatePrefixes and it takes a std::vector<string16> and calculates the
std::vector<size_t> of prefixes. This also has the benefit that you can write a
nice unit test for it:)

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_renderer_data.h:50: int32 common_prefix_length;
size_t

[sky, 2011-02-25 17:03:03]

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.h (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.h:199: virtual void
UpdateCommonTitlePrefix();
On 2011/02/25 17:02:07, sky wrote:
> Why is this virtual?
> 
> Also, this class is only used on windows and chromeos. We're going to want
this
> feature on other platforms too (sadly). This function should be extracted into
a
> place that can be used on other platforms. I'm thinking you create some class
> like PrefixMatcher or PrefixFinder or TabTitlePrefixBlahBlahBlah (you get the
> idea) in chrome/browser/ui. The class has a single static method,
> CalculatePrefixes and it takes a std::vector<string16> and calculates the
> std::vector<size_t> of prefixes. This also has the benefit that you can write
a
> nice unit test for it:)

One additional comment. Don't take my comment to mean you have to figure out the
other platforms now. Just that you should prepare for them:)

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:466:
base::SplitStringAlongWhitespace(tab_data.title, &words);
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]

Addressed most comments except the one about using ICU and the one about moving
the code to a common place...

I'll do that in the next iteration so that the reviewers can see the first
iteration's changes compared the code where it was initially written to make it
easer for diffs...

Next iteration will move the code to a common place and use ICU for word
breaking (and also try to avoid needing to search the words after they got
split)...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:268: kOsAll,
On 2011/02/25 17:02:07, sky wrote:
> Your patch only effects windows and chromeos.

Ho, I didn't realize that... Changed to:
kOsWin | kOsCrOS,  // Until we have a solution for non-views platforms.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:432: // tab we saw them in first,
once we put this other index in the
On 2011/02/25 17:02:07, sky wrote:
> 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.'

Thanks... Done...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
DCHECK(tab_data_[tab_index].tab != NULL);
On 2011/02/25 17:02:07, sky wrote:
> Shouldn't you skip over mini tabs?
Good point, it would save some processing time since we ignore them later
anyway... Done...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:442:
DCHECK(tab_data_[tab_index].tab->data().title ==
On 2011/02/25 17:02:07, sky wrote:
> This code is tricky to read, how about a comment here saying the title is a
> duplicate.
Done...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:454: base::hash_map<string16,
std::vector<size_t> > prefixes;
On 2011/02/25 17:02:07, sky wrote:
> Shouldn't this be std::vector<int> ?

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:466:
base::SplitStringAlongWhitespace(tab_data.title, &words);
On 2011/02/25 17:14:44, Peter Kasting wrote:
> This is the wrong function to use.  We need to use ICU functions to find
logical
> boundaries as some languages don't have whitespace.

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:468: // We don't need the last
word, since we ignored duplicate titles above.
On 2011/02/25 17:02:07, sky wrote:
> Maybe I'm just dense, but this comment doesn't make sense to me.

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...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:469: for (size_t word_index = 0;
word_index < words.size() - 1; ++word_index) {
On 2011/02/25 17:02:07, sky wrote:
> 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.

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...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:471: end_of_word =
tab_data.title.find(words[word_index], end_of_word) +
On 2011/02/25 17:02:07, sky wrote:
> It seems horribly inefficient to have to use find again when presumably
> SplitStringAlongWhitespace knows all this already.

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...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:480: base::hash_map<string16,
std::vector<size_t> >::iterator iter =
On 2011/02/25 17:02:07, sky wrote:
> Move the iter into the for declaration so that its clear the iterator is only
> used in the for loop.

Done...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:496: if
((size_t)data.common_prefix_length != prefix_lengths[tab_index]) {
On 2011/02/25 17:02:07, sky wrote:
> static_cast

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...

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:498:
tab_data_[tab_index].tab->SetData(data);
On 2011/02/25 17:02:07, sky wrote:
> I think you need a SchedulePaint here.

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.h (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.h:199: virtual void
UpdateCommonTitlePrefix();
On 2011/02/25 17:02:07, sky wrote:
> Why is this virtual?
> 
> Also, this class is only used on windows and chromeos. We're going to want
this
> feature on other platforms too (sadly). This function should be extracted into
a
> place that can be used on other platforms. I'm thinking you create some class
> like PrefixMatcher or PrefixFinder or TabTitlePrefixBlahBlahBlah (you get the
> idea) in chrome/browser/ui. The class has a single static method,
> CalculatePrefixes and it takes a std::vector<string16> and calculates the
> std::vector<size_t> of prefixes. This also has the benefit that you can write
a
> nice unit test for it:)

Good point... I looked for a test infrastructure for tabs and couldn't find
one... I'll follow your advice here... Thanks!

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/tab_renderer_data.h (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/tab_renderer_data.h:50: int32 common_prefix_length;
On 2011/02/25 17:02:07, sky wrote:
> size_t

D'Ho!... Done... :-)

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

I only have comments on the rendering.

By next iteration do you mean you want to land this first, then move?

  -Scott

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
DCHECK(tab_data_[tab_index].tab != NULL);
On 2011/02/25 20:09:13, MAD wrote:
> On 2011/02/25 17:02:07, sky wrote:
> > Shouldn't you skip over mini tabs?
> Good point, it would save some processing time since we ignore them later
> anyway... Done...
> 

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:498:
tab_data_[tab_index].tab->SetData(data);
On 2011/02/25 20:09:13, MAD wrote:
> On 2011/02/25 17:02:07, sky wrote:
> > I think you need a SchedulePaint here.
> 
> I was wondering about that, but the SetData implementation actually does it...
> Should I add it anyway?

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

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:449: font_->GetExpectedTextWidth(10) <
title_bounds.width() &&
I thought this became more important once the strings got smaller. Although, is
there a reason why we shouldn't always do this?

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:451: title.replace(0,
data().common_prefix_length, ASCIIToUTF16("..."));
I think you should use the same string as in text_elider for this (kEllipsis).

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

By next iteration, I meant another round of code review, I don't intend to
submit the code like this... :-)

And I don't think I'll have time to finish this today (I'm in EST and about to
leave home now), and I'm on vacation next week. So it'll have to wait until the
week after, but I was trying to get as much done as I could before I leave, and
we'll complete the review when I'm back. I hope it's not too much trouble...

BTW, now that I see there is ui/base/text folder (for the text_elider you
pointed me to), I wonder if I should put the new TitlePrefixMatcher there
instead of under browser/ui, what do you think?

It's kind of tab/browser independent so... Would make sense... Right?

Thanks again!

BYE
MAD
P.S.: I changed the text_elider files so that the kEllipsis is now externalized
under the ui namespace as opposed to local to the unnamed namespace... But I
won't upload the new version just yet, since I'm in the middle of getting the
isolated TitlePrefixMatcher working correctly.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
DCHECK(tab_data_[tab_index].tab != NULL);
On 2011/02/25 21:04:44, sky wrote:
> On 2011/02/25 20:09:13, MAD wrote:
> > On 2011/02/25 17:02:07, sky wrote:
> > > Shouldn't you skip over mini tabs?
> > Good point, it would save some processing time since we ignore them later
> > anyway... Done...
> > 
> 
> I wasn't so much concerned about that, but rather if you consider the title
here
> you might incorrectly think a title was duplicated.

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

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:498:
tab_data_[tab_index].tab->SetData(data);
On 2011/02/25 21:04:44, sky wrote:
> On 2011/02/25 20:09:13, MAD wrote:
> > On 2011/02/25 17:02:07, sky wrote:
> > > I think you need a SchedulePaint here.
> > 
> > I was wondering about that, but the SetData implementation actually does
it...
> > Should I add it anyway?
> 
> It does? It probably should, but I don't think it currently does.

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

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:449: font_->GetExpectedTextWidth(10) <
title_bounds.width() &&
On 2011/02/25 21:04:44, sky wrote:
> I thought this became more important once the strings got smaller. Although,
is
> there a reason why we shouldn't always do this?

Because we may end up with something like ...x... which isn't very useful...

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:451: title.replace(0,
data().common_prefix_length, ASCIIToUTF16("..."));
On 2011/02/25 21:04:44, sky wrote:
> I think you should use the same string as in text_elider for this (kEllipsis).

Ho, thanks, I didn't know about that...
Done...

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

According to Brett's email (Base is "done"...) we should put code 'in the most
specific place that makes sense.' So, that implies chrome/browser/ui, not ui. I
ran this by Ben too and he sayd chrome/browser/ui is the best place too.

Enjoy your vacation and let me know when I should take another look.

  -Scott

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
DCHECK(tab_data_[tab_index].tab != NULL);
On 2011/02/25 21:49:24, MAD wrote:
> On 2011/02/25 21:04:44, sky wrote:
> > On 2011/02/25 20:09:13, MAD wrote:
> > > On 2011/02/25 17:02:07, sky wrote:
> > > > Shouldn't you skip over mini tabs?
> > > Good point, it would save some processing time since we ignore them later
> > > anyway... Done...
> > > 
> > 
> > I wasn't so much concerned about that, but rather if you consider the title
> here
> > you might incorrectly think a title was duplicated.
> 
> Yes, that too... :-)
> Fixed now, right?

Yes, thanks.

http://codereview.chromium.org/6579050/diff/12001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:498:
tab_data_[tab_index].tab->SetData(data);
On 2011/02/25 21:49:24, MAD wrote:
> On 2011/02/25 21:04:44, sky wrote:
> > On 2011/02/25 20:09:13, MAD wrote:
> > > On 2011/02/25 17:02:07, sky wrote:
> > > > I think you need a SchedulePaint here.
> > > 
> > > I was wondering about that, but the SetData implementation actually does
> it...
> > > Should I add it anyway?
> > 
> > It does? It probably should, but I don't think it currently does.
> 
> Actually, it indirectly does, via Layout, which does a SchedulePaint when we
> have a layout manager. Should I had one here still?

BaseTab doesn't have a LayoutManager though.

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:449: font_->GetExpectedTextWidth(10) <
title_bounds.width() &&
On 2011/02/25 21:49:24, MAD wrote:
> On 2011/02/25 21:04:44, sky wrote:
> > I thought this became more important once the strings got smaller. Although,
> is
> > there a reason why we shouldn't always do this?
> 
> Because we may end up with something like ...x... which isn't very useful...

Good point.
Different suggestion then. For folks with lots of tabs all this work is useless.
Would be nice to have a check that avoids doing anything if all the tabs are
small.

[Peter Kasting, 2011-02-25 23:49:44]

BTW I'm ignoring this while it's still so much a WIP, since sky is a fine
reviewer, but if you want me to take a look when it's more done, I'd be happy
to.

If possible, I'd love to avoid adding a flag for this, and just do some personal
testing and then go ahead and land by default, and see what feedback we get.

[MAD, 2011-03-10 19:03:44]

OK, I think I'm done now (at least for views build)... Moved up the logic to a
new file under browser/ui, now use the ICU based BreakIterator, and added unit
tests.

Please take another look...

Though you may decide to wait for the try bots to complete successfully...

Thanks!

BYE
MAD

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:449: font_->GetExpectedTextWidth(10) <
title_bounds.width() &&
On 2011/02/25 22:59:27, sky wrote:
> On 2011/02/25 21:49:24, MAD wrote:
> > On 2011/02/25 21:04:44, sky wrote:
> > > I thought this became more important once the strings got smaller.
Although,
> > is
> > > there a reason why we shouldn't always do this?
> > 
> > Because we may end up with something like ...x... which isn't very useful...
> 
> Good point.
> Different suggestion then. For folks with lots of tabs all this work is
useless.
> Would be nice to have a check that avoids doing anything if all the tabs are
> small.

That would be hard, since the title_bounds and font are not public. Would it be
worth making them public so that the tab_strip could use them to make this
decision?

[sky, 2011-03-11 00:00:45]

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:449: font_->GetExpectedTextWidth(10) <
title_bounds.width() &&
On 2011/03/10 19:03:44, MAD wrote:
> On 2011/02/25 22:59:27, sky wrote:
> > On 2011/02/25 21:49:24, MAD wrote:
> > > On 2011/02/25 21:04:44, sky wrote:
> > > > I thought this became more important once the strings got smaller.
> Although,
> > > is
> > > > there a reason why we shouldn't always do this?
> > > 
> > > Because we may end up with something like ...x... which isn't very
useful...
> > 
> > Good point.
> > Different suggestion then. For folks with lots of tabs all this work is
> useless.
> > Would be nice to have a check that avoids doing anything if all the tabs are
> > small.
> 
> That would be hard, since the title_bounds and font are not public. Would it
be
> worth making them public so that the tab_strip could use them to make this
> decision?

Do you even need that? For horizontal tabs (TabStrip) couldn't you only do if
the width of each tab is > some value? For vertical tabs (SideTabStrip) you
probably never want to do this. I think.

If you do this you would have to be careful to recalculate things on layout.
This is a pretty expensive computation. Is it worth trying to avoid it?

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
File chrome/browser/ui/title_prefix_matcher.cc (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.cc:12: const size_t kPreviouslySeenIndex
= 0xFFFFFFFF;
Document whta this is for.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
File chrome/browser/ui/title_prefix_matcher.h (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:13: namespace browser {
We generally don't use namespaces in cases like this.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:16: // so that it can find the length
of the common prefixes among all the titles.
Imagine if you didn't write this class and went to use it on the mac and/or
linux. Is there enough info here to tell you how to use it? For example,
TitleInfo::caller_value isn't described, nor do you document what prefix_length
is. Could you clean this up? You should also document what this is intended to
be used for.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:21: : title(title), prefix_length(0),
caller_value(caller_value) {}
Since you wrapper, each param should be on its own line.
How a DCHECK(title) too.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:22: TitleInfo(const TitleInfo& other)
How come you wrote copy constructor/assignment operator. For this class aren't
the compiler generated ones what you want?

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:42:
DISALLOW_COPY_AND_ASSIGN(TitlePrefixMatcher);
after private. But you probably want DISALLOW_IMPLICIT_CONSTRUCTORS.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:17: #include
"chrome/common/chrome_switches.h"
Do we know this include anymore? In fact I suspect you can nuke nearly all of
the includes you added.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
tab_data_[tab_index].tab->SetData(data);
Didn't we want a SchedulePaint here?

[sky, 2011-03-11 00:01:37]

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:436:
tab_data_[tab_index].tab->SetData(data);
On 2011/03/11 00:00:45, sky wrote:
> Didn't we want a SchedulePaint here?

Never mind. I see it in BaseTab.

[MAD, 2011-03-11 03:01:37]

OK, I think I addressed all your comments in the most recent upload, except for
checking if we need to compute the prefixes at the tab strip level if the tab
width is too small...

I answered in the comment, and I will try something out, but I wanted to give
you the fixes to all other comments in the mean time...

Thanks a lot!

BYE
MAD

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (right):

http://codereview.chromium.org/6579050/diff/17002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:449: font_->GetExpectedTextWidth(10) <
title_bounds.width() &&
Hum... I'm not sure what would be a good "some value"... But I could try
something...

I guess I could keep an extra boolean identifying if I need to recompute the
prefixes or not, and only compute them when I need to and when I have enough
room for them... What do you think?

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
File chrome/browser/ui/title_prefix_matcher.cc (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.cc:12: const size_t kPreviouslySeenIndex
= 0xFFFFFFFF;
On 2011/03/11 00:00:45, sky wrote:
> Document whta this is for.

Done.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
File chrome/browser/ui/title_prefix_matcher.h (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:13: namespace browser {
On 2011/03/11 00:00:45, sky wrote:
> We generally don't use namespaces in cases like this.

Done.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:16: // so that it can find the length
of the common prefixes among all the titles.
On 2011/03/11 00:00:45, sky wrote:
> Imagine if you didn't write this class and went to use it on the mac and/or
> linux. Is there enough info here to tell you how to use it? For example,
> TitleInfo::caller_value isn't described, nor do you document what
prefix_length
> is. Could you clean this up? You should also document what this is intended to
> be used for.

Done.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:21: : title(title), prefix_length(0),
caller_value(caller_value) {}
On 2011/03/11 00:00:45, sky wrote:
> Since you wrapper, each param should be on its own line.
> How a DCHECK(title) too.

Done.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:22: TitleInfo(const TitleInfo& other)
On 2011/03/11 00:00:45, sky wrote:
> How come you wrote copy constructor/assignment operator. For this class aren't
> the compiler generated ones what you want?

Done.
This was needed in a previous iteration when the title was a const& and I was
allocating the TitleInfo... When I changed it to not allocating the TitleInfo,
it needed these, but I could find a way to make it work with const string16&, so
I changed it to a pointer and forgot to remove these... Thanks for mentioning
it... :-)

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:42:
DISALLOW_COPY_AND_ASSIGN(TitlePrefixMatcher);
On 2011/03/11 00:00:45, sky wrote:
> after private. But you probably want DISALLOW_IMPLICIT_CONSTRUCTORS.

Done.

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab_strip.cc (right):

http://codereview.chromium.org/6579050/diff/32001/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab_strip.cc:17: #include
"chrome/common/chrome_switches.h"
On 2011/03/11 00:00:45, sky wrote:
> Do we know this include anymore? In fact I suspect you can nuke nearly all of
> the includes you added.

D'ho...ne.

Sorry about that...

[sky, 2011-03-11 16:32:15]

I'm giving you the LGTM for now.

As this computation is expensive, and gets even more so with the more tabs you
have I think we should not do the calculation if possible. So, ya, the boolean
sounds good. That can be done in a separate patch though. I say land this for
now.

  -Scott

http://codereview.chromium.org/6579050/diff/38003/chrome/browser/ui/title_pre...
File chrome/browser/ui/title_prefix_matcher.h (right):

http://codereview.chromium.org/6579050/diff/38003/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:41: private:
nit: newline between 40 and 41.

[MAD, 2011-03-11 19:17:56]

Thanks! :-)

Will do...

BYE
MAD

[MAD, 2011-03-11 19:24:41]

http://codereview.chromium.org/6579050/diff/38003/chrome/browser/ui/title_pre...
File chrome/browser/ui/title_prefix_matcher.h (right):

http://codereview.chromium.org/6579050/diff/38003/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:41: private:
On 2011/03/11 16:32:15, sky wrote:
> nit: newline between 40 and 41.

Done.