Improvements to tab title prefix eliding as per email discussions.

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!
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80492

[Peter Kasting, 2011-04-01 17:28:31]

LGTM

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:312: [cell
setDesiredCharactersToTruncateFromHead:length - 4];
Nit: Use constants for these and put the constants some place that can be shared
by all the implementations.

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/title_pref...
File chrome/browser/ui/title_prefix_matcher.cc (right):

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/title_pref...
chrome/browser/ui/title_prefix_matcher.cc:46:
existing_title.insert(std::make_pair(*title_infos->at(i).title, i));
Nit: While you're here, use (*title_infos)[i] to make clear that you don't need
the bounds-checking at() performs.  (Many places)

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/title_pref...
chrome/browser/ui/title_prefix_matcher.cc:99:
DCHECK(title_infos->at(iter->second[i]).url.host() ==
Nit: DCHECK_EQ()

[MAD, 2011-04-01 19:47:37]

Addressed all comments...

Could sail@ confirm the Mac changes are OK please?

Thanks!

BYE
MAD

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/cocoa/tabs...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/cocoa/tabs...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:312: [cell
setDesiredCharactersToTruncateFromHead:length - 4];
On 2011/04/01 17:28:31, Peter Kasting wrote:
> Nit: Use constants for these and put the constants some place that can be
shared
> by all the implementations.

Done.

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/title_pref...
File chrome/browser/ui/title_prefix_matcher.cc (right):

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/title_pref...
chrome/browser/ui/title_prefix_matcher.cc:46:
existing_title.insert(std::make_pair(*title_infos->at(i).title, i));
On 2011/04/01 17:28:31, Peter Kasting wrote:
> Nit: While you're here, use (*title_infos)[i] to make clear that you don't
need
> the bounds-checking at() performs.  (Many places)

Done.

http://codereview.chromium.org/6783015/diff/4001/chrome/browser/ui/title_pref...
chrome/browser/ui/title_prefix_matcher.cc:99:
DCHECK(title_infos->at(iter->second[i]).url.host() ==
On 2011/04/01 17:28:31, Peter Kasting wrote:
> Nit: DCHECK_EQ()

Done.

[sail, 2011-04-01 19:53:05]

LGTM
The Mac code looks good!

[Peter Kasting, 2011-04-01 20:19:20]

LGTM

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (left):

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:8: 
Nit: Don't remove this blank line

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

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:482: // We arbitrarily choose to
request enough room for 6 average chars.
Nit: These comments are more useful than the ones you currently have on these
constants' declarations in title_prefix_matcher.h (which mostly just restate the
constants' names).  Can you delete this comment and put something like it there?

[sky, 2011-04-04 15:48:17]

LGTM

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

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.cc:18: const int
TitlePrefixMatcher::kCommonCharsToShow = 4;
// static for both of these.

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.cc:101: size_t prefix_length =
iter->first.size() -
DCHECK_GE(iter->first.size(), first_title.info.url.host().size())

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

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:31: // We assume the title string will
be valid throughout the execution of
Add a destructor and move it to the .cc

[MAD, 2011-04-05 16:13:12]

Thanks...

Will re-sync, re-build, re-try and then submit, now that I addressed all latest
comments...

BYE
MAD

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

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.cc:18: const int
TitlePrefixMatcher::kCommonCharsToShow = 4;
On 2011/04/04 15:48:17, sky wrote:
> // static for both of these.

Done.

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

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/title_pre...
chrome/browser/ui/title_prefix_matcher.h:31: // We assume the title string will
be valid throughout the execution of
On 2011/04/04 15:48:17, sky wrote:
> Add a destructor and move it to the .cc

Done.

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/base_tab.cc (left):

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:8: 
On 2011/04/01 20:19:20, Peter Kasting wrote:
> Nit: Don't remove this blank line

D'Ho...ne. :-)

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

http://codereview.chromium.org/6783015/diff/10002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/base_tab.cc:482: // We arbitrarily choose to
request enough room for 6 average chars.
On 2011/04/01 20:19:20, Peter Kasting wrote:
> Nit: These comments are more useful than the ones you currently have on these
> constants' declarations in title_prefix_matcher.h (which mostly just restate
the
> constants' names).  Can you delete this comment and put something like it
there?
Good idea..
Done.

[commit-bot: I haz the power, 2011-04-05 18:48:35]

Try job failure for 6783015-13001 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...