Hide close buttons of inactive stacked tabs by default

Hide close buttons of inactive stacked tabs by default

Hide the close buttons of inactive tabs when the tabstrip
is in stacked mode (i.e., when touch is used to interact
with the tabstrip and the width of the tabs falls below
our minimum width threshold for touch). Introduce the flag
--disable-hide-inactive-stacked-tab-close-buttons
in order to disable this behaviour if needed.

BUG=455312
TEST=TabStripTest.TabCloseButtonVisibilityWhenStacked

Committed: https://crrev.com/5c0b95bd16c16a474ae3e5fd91c090f2e4851452
Cr-Commit-Position: refs/heads/master@{#320346}

[tdanderson, 2015-03-05 19:31:44]

tdanderson@chromium.org changed reviewers:
+ sky@chromium.org

[tdanderson, 2015-03-05 19:31:44]

Hi Scott, this has been given UX approval. Can you please take a look?

https://codereview.chromium.org/983853002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/983853002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:52558: -  <int value="412957264"
label="tab-close-buttons-hidden-with-touch"/>
Why was I required to have this entry for the flag I'm removing, but not for the
flag I'm adding?

[tdanderson, 2015-03-05 21:00:59]

On 2015/03/05 19:31:44, tdanderson wrote:
> Hi Scott, this has been given UX approval. Can you please take a look?
> 
>
https://codereview.chromium.org/983853002/diff/1/tools/metrics/histograms/his...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/983853002/diff/1/tools/metrics/histograms/his...
> tools/metrics/histograms/histograms.xml:52558: -  <int value="412957264"
> label="tab-close-buttons-hidden-with-touch"/>
> Why was I required to have this entry for the flag I'm removing, but not for
the
> flag I'm adding?

Sorry, looks like I need to take a look at the trybot failures first. Please
hold
off reviewing for now, I'll re-ping when the CL is ready.

[tdanderson, 2015-03-06 19:59:21]

Hi Scott, can you please take a look?

https://chromiumcodereview.appspot.com/983853002/diff/1/tools/metrics/histogr...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/983853002/diff/1/tools/metrics/histogr...
tools/metrics/histograms/histograms.xml:52558: -  <int value="412957264"
label="tab-close-buttons-hidden-with-touch"/>
On 2015/03/05 19:31:44, tdanderson wrote:
> Why was I required to have this entry for the flag I'm removing, but not for
the
> flag I'm adding?

Ah, I see that the trybots caught this (I had assumed it would have been a part
of the presubmit checks).

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:420: TEST_F(TabStripTest,
TabCloseButtonVisibilityWhenStacked) {
My test approach here feels like a bit of a hack, but this is the only way I
could find in order to trigger a relayout (and thus the new code path) for
stacked tab mode. I was unable to implement working tests that would let me
properly switch tabs and add tabs while in stacked mode. Can you offer any
suggestions here?

[sky, 2015-03-06 23:28:33]

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip.cc:978: if (!stacked_layout_ ||
tab->width() > Tab::GetTouchWidth())
Don't you just care about if touch_layout_ is non-null here?

[tdanderson, 2015-03-09 17:30:53]

Scott, can you PTAL?

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip.cc:978: if (!stacked_layout_ ||
tab->width() > Tab::GetTouchWidth())
On 2015/03/06 23:28:33, sky wrote:
> Don't you just care about if touch_layout_ is non-null here?

Yes, you're right - I can just do that instead since I'm removing the other
variants. Changed.

[sky, 2015-03-09 18:06:48]

LGTM

[tdanderson, 2015-03-09 18:16:02]

On 2015/03/09 18:06:48, sky wrote:
> LGTM

Thanks, but I'm concerned that I'm not properly testing this
feature (see my comment in Patch Set 2). WDYT?

[sky, 2015-03-09 19:55:59]

Going back to not lgtm until sorted.

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:420: TEST_F(TabStripTest,
TabCloseButtonVisibilityWhenStacked) {
On 2015/03/06 19:59:21, tdanderson wrote:
> My test approach here feels like a bit of a hack, but this is the only way I
> could find in order to trigger a relayout (and thus the new code path) for
> stacked tab mode. I was unable to implement working tests that would let me
> properly switch tabs and add tabs while in stacked mode. Can you offer any
> suggestions here?

Your change means we need to make sure SetStackedLayout() invokes Layout() on
all the tabs. That isn't necessarily the case right now. You'll have to update
SetStackedLayout() to do this.

Can you elaborate one what problems you had in adding/removing? Maybe you're
tripping over the bug I mentioned TabStrip::ShouldHideCloseButtonForInactiveTabs
?

https://chromiumcodereview.appspot.com/983853002/diff/40001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/40001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip.cc:973: if (!touch_layout_)
I was wrong here. My understanding is you always want to hide the close buttons
if the user use a touch device on the tabstrip. You should key off
stacked_layout_ for that. touch_layout_ means we're actively in the stacked
mode, which is slightly different. (The names I have here are confusing, I need
to change them).

[tdanderson, 2015-03-11 18:01:39]

Hi Scott, can you please take a look at my comments below?

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/20001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:420: TEST_F(TabStripTest,
TabCloseButtonVisibilityWhenStacked) {
On 2015/03/09 19:55:59, sky wrote:
> On 2015/03/06 19:59:21, tdanderson wrote:
> > My test approach here feels like a bit of a hack, but this is the only way I
> > could find in order to trigger a relayout (and thus the new code path) for
> > stacked tab mode. I was unable to implement working tests that would let me
> > properly switch tabs and add tabs while in stacked mode. Can you offer any
> > suggestions here?
> 
> Your change means we need to make sure SetStackedLayout() invokes Layout() on
> all the tabs. That isn't necessarily the case right now. You'll have to update
> SetStackedLayout() to do this.
> 
> Can you elaborate one what problems you had in adding/removing? Maybe you're
> tripping over the bug I mentioned
TabStrip::ShouldHideCloseButtonForInactiveTabs
> ?

I've made comments about this in the latest patchset.

https://chromiumcodereview.appspot.com/983853002/diff/40001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/40001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip.cc:973: if (!touch_layout_)
On 2015/03/09 19:55:59, sky wrote:
> I was wrong here. My understanding is you always want to hide the close
buttons
> if the user use a touch device on the tabstrip. You should key off
> stacked_layout_ for that. touch_layout_ means we're actively in the stacked
> mode, which is slightly different. (The names I have here are confusing, I
need
> to change them).

Actually, you were right. Using |touch_layout_| is what we want to be doing. The
desired behavior is to only hide the tab close buttons of inactive tabs when
we're actively in stacked mode. My CL description was a bit misleading, so I
changed it.

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:452:
EXPECT_FALSE(tab3->showing_close_button_);
Everything works fine up to here. AddTab() triggers a relayout, and the inactive
tab buttons become hidden.

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:456:
tab_strip_->SelectTab(tab2);
Lines 458 and 459 fail. SelectTab() does not trigger a relayout, and the checks
still fail even if I try to force a relayout by adjusting the tabstrip width.

It also seems the implementation of FakeBaseTabStripController is not sufficient
to be able to properly test tab switching.

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:464: // Close a tab. Three
tabs remain, so touch layout should not be
Lines 467-469 also fail. CloseTab() does not trigger a relayout, and the checks
still fail even if I try to force a relayout by adjusting the tabstrip width.

[sky, 2015-03-11 18:20:14]

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:448: Tab* tab3 =
tab_strip_->tab_at(3);
I think you want an assertion here that you're in touch layout mode, otherwise
the following expectations aren't going to be applicable.

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:456:
tab_strip_->SelectTab(tab2);
On 2015/03/11 18:01:38, tdanderson wrote:
> Lines 458 and 459 fail. SelectTab() does not trigger a relayout, and the
checks
> still fail even if I try to force a relayout by adjusting the tabstrip width.
> 
> It also seems the implementation of FakeBaseTabStripController is not
sufficient
> to be able to properly test tab switching.

I suspect you just send what the problem is. You're change means we need to
force the old/new tabs to layout when the active tab changes. We don't
necessarily do that now.

[tdanderson, 2015-03-11 19:30:13]

Thanks for the feedback. Is the latest patchset what you had in mind?

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:448: Tab* tab3 =
tab_strip_->tab_at(3);
On 2015/03/11 18:20:14, sky wrote:
> I think you want an assertion here that you're in touch layout mode, otherwise
> the following expectations aren't going to be applicable.

Done.

https://chromiumcodereview.appspot.com/983853002/diff/60001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:456:
tab_strip_->SelectTab(tab2);
On 2015/03/11 18:20:14, sky wrote:
> On 2015/03/11 18:01:38, tdanderson wrote:
> > Lines 458 and 459 fail. SelectTab() does not trigger a relayout, and the
> checks
> > still fail even if I try to force a relayout by adjusting the tabstrip
width.
> > 
> > It also seems the implementation of FakeBaseTabStripController is not
> sufficient
> > to be able to properly test tab switching.
> 
> I suspect you just send what the problem is. You're change means we need to
> force the old/new tabs to layout when the active tab changes. We don't
> necessarily do that now.

Done.

[sky, 2015-03-11 19:55:16]

https://chromiumcodereview.appspot.com/983853002/diff/80001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/80001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:128: void
ForceLayoutOfAllTabs() {
You shouldn't have to do this in the test. TabStrip should make sure Layout()
happens at the right time.

[sky, 2015-03-11 20:26:45]

LGTM with a better comment.

[tdanderson, 2015-03-11 21:10:51]

tdanderson@chromium.org changed reviewers:
+ mpearson@chromium.org, sievers@chromium.org

[tdanderson, 2015-03-11 21:10:52]

mpearson@chromium.org: Can you please review changes in tools/ ?

sievers@chromium.org: Can you please review changes in content/ ?

https://chromiumcodereview.appspot.com/983853002/diff/80001/chrome/browser/ui...
File chrome/browser/ui/views/tabs/tab_strip_unittest.cc (right):

https://chromiumcodereview.appspot.com/983853002/diff/80001/chrome/browser/ui...
chrome/browser/ui/views/tabs/tab_strip_unittest.cc:128: void
ForceLayoutOfAllTabs() {
On 2015/03/11 19:55:16, sky wrote:
> You shouldn't have to do this in the test. TabStrip should make sure Layout()
> happens at the right time.

Changed to forcing a paint instead based on offline discussion. (The
paint is what triggers the necessary layout, but painting doesn't
take place in unit tests.)

[no sievers, 2015-03-11 21:28:03]

content lgtm

[sky, 2015-03-11 21:44:36]

SLGTM

[Mark P, 2015-03-11 22:25:10]

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:52695: -  <int value="412957264"
label="tab-close-buttons-hidden-with-touch"/>
Please do not delete entries from this enum.  They help for interpreting data.

[tdanderson, 2015-03-11 23:37:38]

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:52695: -  <int value="412957264"
label="tab-close-buttons-hidden-with-touch"/>
On 2015/03/11 22:25:10, Mark P wrote:
> Please do not delete entries from this enum.  They help for interpreting data.

Should I still keep this entry in histograms.xml even though I am removing the
tab-close-buttons-hidden-with-touch switch as part of this CL (see
content_switches.cc)?

[Mark P, 2015-03-11 23:42:46]

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:52695: -  <int value="412957264"
label="tab-close-buttons-hidden-with-touch"/>
On 2015/03/11 23:37:37, tdanderson wrote:
> On 2015/03/11 22:25:10, Mark P wrote:
> > Please do not delete entries from this enum.  They help for interpreting
data.
> 
> Should I still keep this entry in histograms.xml even though I am removing the
> tab-close-buttons-hidden-with-touch switch as part of this CL (see
> content_switches.cc)?

Yes, that's exactly what I mean.

[tdanderson, 2015-03-11 23:53:50]

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (left):

https://chromiumcodereview.appspot.com/983853002/diff/100001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:52695: -  <int value="412957264"
label="tab-close-buttons-hidden-with-touch"/>
On 2015/03/11 23:42:46, Mark P wrote:
> On 2015/03/11 23:37:37, tdanderson wrote:
> > On 2015/03/11 22:25:10, Mark P wrote:
> > > Please do not delete entries from this enum.  They help for interpreting
> data.
> > 
> > Should I still keep this entry in histograms.xml even though I am removing
the
> > tab-close-buttons-hidden-with-touch switch as part of this CL (see
> > content_switches.cc)?
> 
> Yes, that's exactly what I mean.

Done.

[Mark P, 2015-03-12 16:31:15]

histograms.xml lgtm

[tdanderson, 2015-03-12 17:46:15]

The CQ bit was checked by tdanderson@chromium.org

[tdanderson, 2015-03-12 17:46:16]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
sievers@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/983853002/#ps120001
(title: "histograms.xml changed")

[commit-bot: I haz the power, 2015-03-12 17:54:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/983853002/120001

[commit-bot: I haz the power, 2015-03-12 19:41:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-12 19:41:59]

Patchset 7 (id:??) landed as
https://crrev.com/5c0b95bd16c16a474ae3e5fd91c090f2e4851452
Cr-Commit-Position: refs/heads/master@{#320346}