[SiteChip] Remove 50% width cap for site chip

[SiteChip] Remove 50% width cap for site chip

R=pkasting@chromium.org
BUG=331373
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249497

[Greg Billock, 2014-01-17 00:27:43]

On 2014/01/17 00:25:37, Greg Billock wrote:

Visually, this typically will just show more site chip for long names.

The window width clamps to the minimum omnibox width, so the minimum window
width is just quite wide for sites with really long domain names.

If you shrink the window, then go to a site with a really long name, the toolbar
overhangs the window, looking like the toolbar is chopped off. I think this is
what we want. When you resize the window, it pops out to min-width.

[Peter Kasting, 2014-01-17 01:40:06]

This patch seems wrong.  We still want to cap at 50% when doing so does not
require eliding the TLD+1 portion.

[Greg Billock, 2014-01-17 15:59:24]

On 2014/01/17 01:40:06, Peter Kasting wrote:
> This patch seems wrong.  We still want to cap at 50% when doing so does not
> require eliding the TLD+1 portion.

We'll need a custom elider to accomplish the full goal. The toolbar view is
about right, though, which is the main goal here.

[Peter Kasting, 2014-01-17 22:17:46]

On 2014/01/17 15:59:24, Greg Billock wrote:
> The toolbar view is
> about right, though, which is the main goal here.

I don't understand what this is saying.  How does this make the toolbar view
"about right"?  This patch seems pretty much not related to how we ultimately
want eliding to work.

Can you just write a patch that completely implements the desired behavior from
the bug, and I can review it all at once?

[Greg Billock, 2014-01-18 01:01:53]

On 2014/01/17 22:17:46, Peter Kasting wrote:
> On 2014/01/17 15:59:24, Greg Billock wrote:
> > The toolbar view is
> > about right, though, which is the main goal here.
> 
> I don't understand what this is saying.  How does this make the toolbar view
> "about right"?  This patch seems pretty much not related to how we ultimately
> want eliding to work.

Well, sure, but isn't that basically echoing what I just said?

The difference for the toolbar is that it'll need to call the site chip with the
50% width point so that if eliding is possible, the elider has a target. So
other than feeding the TBD elider, it's what we want.

> Can you just write a patch that completely implements the desired behavior
from
> the bug, and I can review it all at once?

That's fine, too.

[Greg Billock, 2014-01-22 01:09:00]

On 2014/01/18 01:01:53, Greg Billock wrote:
> On 2014/01/17 22:17:46, Peter Kasting wrote:
> > On 2014/01/17 15:59:24, Greg Billock wrote:
> > > The toolbar view is
> > > about right, though, which is the main goal here.
> > 
> > I don't understand what this is saying.  How does this make the toolbar view
> > "about right"?  This patch seems pretty much not related to how we
ultimately
> > want eliding to work.
> 
> Well, sure, but isn't that basically echoing what I just said?
> 
> The difference for the toolbar is that it'll need to call the site chip with
the
> 50% width point so that if eliding is possible, the elider has a target. So
> other than feeding the TBD elider, it's what we want.
> 
> > Can you just write a patch that completely implements the desired behavior
> from
> > the bug, and I can review it all at once?
> 
> That's fine, too.

The TextElider has about 90% of what we need, so I have a separate patch out to
get us the rest of the way there. Once that's in, this should be pretty simple.

[Greg Billock, 2014-02-04 19:08:20]

On 2014/01/22 01:09:00, Greg Billock wrote:
> On 2014/01/18 01:01:53, Greg Billock wrote:
> > On 2014/01/17 22:17:46, Peter Kasting wrote:
> > > On 2014/01/17 15:59:24, Greg Billock wrote:
> > > > The toolbar view is
> > > > about right, though, which is the main goal here.
> > > 
> > > I don't understand what this is saying.  How does this make the toolbar
view
> > > "about right"?  This patch seems pretty much not related to how we
> ultimately
> > > want eliding to work.
> > 
> > Well, sure, but isn't that basically echoing what I just said?
> > 
> > The difference for the toolbar is that it'll need to call the site chip with
> the
> > 50% width point so that if eliding is possible, the elider has a target. So
> > other than feeding the TBD elider, it's what we want.
> > 
> > > Can you just write a patch that completely implements the desired behavior
> > from
> > > the bug, and I can review it all at once?
> > 
> > That's fine, too.
> 
> The TextElider has about 90% of what we need, so I have a separate patch out
to
> get us the rest of the way there. Once that's in, this should be pretty
simple.

OK, this is ready for a look. We'll lift the algorithm for use in the
location-bar site chip.

[Peter Kasting, 2014-02-05 00:42:48]

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/site_chip_view.cc (right):

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/site_chip_view.cc:464: base::string16
elided_host = ElideHost(
What is ElideHost() a reference to?

Nit: I'd just inline this into the next statement

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_view.cc:586: int site_chip_width =
site_chip_view_->ElideDomainTarget(available_width/2);
Nit: Spaces around operators

[Greg Billock, 2014-02-05 18:32:04]

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/site_chip_view.cc (right):

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/site_chip_view.cc:464: base::string16
elided_host = ElideHost(
On 2014/02/05 00:42:49, Peter Kasting wrote:
> What is ElideHost() a reference to?

This is the new functionality I've placed in the text elider (now in elide_url).

> Nit: I'd just inline this into the next statement

sounds good.

[Peter Kasting, 2014-02-05 21:08:08]

LGTM

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/site_chip_view.cc (right):

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/site_chip_view.cc:464: base::string16
elided_host = ElideHost(
On 2014/02/05 18:32:05, Greg Billock wrote:
> On 2014/02/05 00:42:49, Peter Kasting wrote:
> > What is ElideHost() a reference to?
> 
> This is the new functionality I've placed in the text elider (now in
elide_url).

OK.  I don't know that I'm hugely keen on it being a global function, but
whatever.

Note: Reading ElideHost()'s header comment, something is confusing:

// ...leading-elide the domain name to fit the width.
// Example: http://sub.domain.com ---> ..."s...domain.com"

The description says "leading-elide", but the example does middle-eliding.

[Greg Billock, 2014-02-06 17:38:51]

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/site_chip_view.cc (right):

https://codereview.chromium.org/141333004/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/site_chip_view.cc:464: base::string16
elided_host = ElideHost(
On 2014/02/05 21:08:08, Peter Kasting wrote:
> On 2014/02/05 18:32:05, Greg Billock wrote:
> > On 2014/02/05 00:42:49, Peter Kasting wrote:
> > > What is ElideHost() a reference to?
> > 
> > This is the new functionality I've placed in the text elider (now in
> elide_url).
> 
> OK.  I don't know that I'm hugely keen on it being a global function, but
> whatever.

It shares a great deal of code with ElideUrl, as you'd expect.

> 
> Note: Reading ElideHost()'s header comment, something is confusing:
> 
> // ...leading-elide the domain name to fit the width.
> // Example: http://sub.domain.com ---> ..."s...domain.com"
> 
> The description says "leading-elide", but the example does middle-eliding.

Thanks. Yeah, that's just wrong. I'll fix it.

[Greg Billock, 2014-02-06 17:38:54]

The CQ bit was checked by gbillock@chromium.org

[commit-bot: I haz the power, 2014-02-06 17:42:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/141333004/240001

[commit-bot: I haz the power, 2014-02-06 21:04:44]

Change committed as 249497