Clip tabs in overflow mode.

Clip tabs in overflow mode.

The tabstrip now tries to ensure that if tabs would encroach on the New Tab
button area or be clipped by the edge of the strip, they're made invisible
instead.  This prevents glitchy-looking overflow, modulo some existing bugs (on
file) where the strip doesn't recalculate widths correctly.

This also hides the tab next to the New Tab button if it can be shown when not
selected, but might be hidden when it (or a prior tab) is selected.  This
prevents having this tab toggle in and out as the active tab changes.

BUG=62510
TEST=Spawn lots of tabs and shrink the window to a narrow width.  Once the tabs hit their min size they should start hiding at the right edge instead of drawing atop the New Tab button.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278028

[Peter Kasting, 2014-06-17 19:12:39]

Most of the changes to TabStrip::RemoveTabDelegate are just cleanup that got
left out of the cleanup patch.

[sky, 2014-06-17 19:41:18]

Sure would be nice to have some test coverage too...

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:740: if (stacked_layout_)
touch_layout_ is more correct here.

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:774: if (tab_index <=
GetMiniTabCount())
I would check tab->data().mini here. The conditional you have is the same, but
more expensive. Arguably it doesn't matter as you need the index below.

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:1594: void
TabStrip::SetTabVisibility() {
Can you still hit the crash you sent to me with your code? I don't see you
calling out to the model here, so it seems like it shouldn't be effected. Is
that right?

[Peter Kasting, 2014-06-17 20:20:20]

I'm not opposed to test coverage, but it's not immediately obvious to me how to
do it in a non-flaky way (i.e. not with an interactive UI test).  Do you have
ideas?

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:740: if (stacked_layout_)
On 2014/06/17 19:41:17, sky wrote:
> touch_layout_ is more correct here.

Why?  If stacked_layout_ is true, then either we're stacking tabs (touch_layout_
is non-NULL) or we don't have any overflow.  In either case, all tabs are
visible.  Thus using |stacked_layout_| short-circuits cases that would otherwise
have to run through the whole function.

Or am I mistaken?

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:774: if (tab_index <=
GetMiniTabCount())
On 2014/06/17 19:41:18, sky wrote:
> I would check tab->data().mini here. The conditional you have is the same, but
> more expensive. Arguably it doesn't matter as you need the index below.

Done.

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:1594: void
TabStrip::SetTabVisibility() {
On 2014/06/17 19:41:17, sky wrote:
> Can you still hit the crash you sent to me with your code?

I removed the DCHECK in ShouldTabBeVisible() that I was seeing, so as to not
trip this.

> I don't see you
> calling out to the model here, so it seems like it shouldn't be effected. Is
> that right?

With the DCHECK removed, I think the only call to the model is when
ShouldTabBeVisible() calls TabStripController::GetActiveIndex().

[sky, 2014-06-17 23:52:51]

The important thing you want to test is visibility of tabs as you
add/remove and change the width right? All that should be possible with a
unit test. And I believe there is an existing unit test you can extend.

  -Scott


On Tue, Jun 17, 2014 at 1:20 PM, <pkasting@chromium.org> wrote:

> I'm not opposed to test coverage, but it's not immediately obvious to me
> how to
> do it in a non-flaky way (i.e. not with an interactive UI test).  Do you
> have
> ideas?
>
>
>
> https://codereview.chromium.org/339923005/diff/1/chrome/
> browser/ui/views/tabs/tab_strip.cc
> File chrome/browser/ui/views/tabs/tab_strip.cc (right):
>
> https://codereview.chromium.org/339923005/diff/1/chrome/
> browser/ui/views/tabs/tab_strip.cc#newcode740
> chrome/browser/ui/views/tabs/tab_strip.cc:740: if (stacked_layout_)
> On 2014/06/17 19:41:17, sky wrote:
>
>> touch_layout_ is more correct here.
>>
>
> Why?  If stacked_layout_ is true, then either we're stacking tabs
> (touch_layout_ is non-NULL) or we don't have any overflow.  In either
> case, all tabs are visible.  Thus using |stacked_layout_| short-circuits
> cases that would otherwise have to run through the whole function.
>
> Or am I mistaken?
>
>
> https://codereview.chromium.org/339923005/diff/1/chrome/
> browser/ui/views/tabs/tab_strip.cc#newcode774
> chrome/browser/ui/views/tabs/tab_strip.cc:774: if (tab_index <=
> GetMiniTabCount())
> On 2014/06/17 19:41:18, sky wrote:
>
>> I would check tab->data().mini here. The conditional you have is the
>>
> same, but
>
>> more expensive. Arguably it doesn't matter as you need the index
>>
> below.
>
> Done.
>
>
> https://codereview.chromium.org/339923005/diff/1/chrome/
> browser/ui/views/tabs/tab_strip.cc#newcode1594
> chrome/browser/ui/views/tabs/tab_strip.cc:1594: void
> TabStrip::SetTabVisibility() {
> On 2014/06/17 19:41:17, sky wrote:
>
>> Can you still hit the crash you sent to me with your code?
>>
>
> I removed the DCHECK in ShouldTabBeVisible() that I was seeing, so as to
> not trip this.
>
>
>  I don't see you
>> calling out to the model here, so it seems like it shouldn't be
>>
> effected. Is
>
>> that right?
>>
>
> With the DCHECK removed, I think the only call to the model is when
> ShouldTabBeVisible() calls TabStripController::GetActiveIndex().
>
> https://codereview.chromium.org/339923005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-06-17 23:53:13]

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/339923005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_strip.cc:740: if (stacked_layout_)
On 2014/06/17 20:20:20, Peter Kasting wrote:
> On 2014/06/17 19:41:17, sky wrote:
> > touch_layout_ is more correct here.
> 
> Why?  If stacked_layout_ is true, then either we're stacking tabs
(touch_layout_
> is non-NULL) or we don't have any overflow.  In either case, all tabs are
> visible.  Thus using |stacked_layout_| short-circuits cases that would
otherwise
> have to run through the whole function.
> 
> Or am I mistaken?

No, you're right.

[Peter Kasting, 2014-06-18 01:26:36]

Fixed the existing unittest failure and added a new test.  PTAL.

[sky, 2014-06-18 03:21:28]

Nice, LGTM

[Peter Kasting, 2014-06-18 04:49:03]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-06-18 04:51:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/339923005/60001

[commit-bot: I haz the power, 2014-06-18 10:56:38]

Change committed as 278028