Experiment to hide close buttons of inactive tabs when using touch

Experiment to hide close buttons of inactive tabs when using touch

Add the flag --tab-close-buttons-hidden-with-touch, which
gives options for having the close buttons on inactive tabs
hidden when touch is used to interact with the tabstrip.
The flag gives four options:

* Always show the tab close buttons. This is the default
  and is the current behaviour.
* Always hide the tab close buttons.
* Hide the tab close buttons once the tabs become
  narrower than their standard width.
* Hide the tab close buttons once the tabs begin
  stacking.

BUG=455312
TEST=none

Committed: https://crrev.com/8edd6e43a27564d8fab055d29c2177b253541495
Cr-Commit-Position: refs/heads/master@{#315684}

[tdanderson, 2015-02-06 20:34:32]

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

[tdanderson, 2015-02-06 20:34:33]

Hi Scott, can you please take a first look at this CL + my inline comments?

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
chrome/browser/ui/tabs/tab_utils.cc:12: #include
"chrome/browser/ui/views/tabs/tab.h"
I added this so that I can access the static methods in Tab which give the
ideal/touch widths, but this is a violation of the include rules. Can you
suggest the best way to make the width information available?

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:34: virtual bool
TouchWasUsedAsLastInput() = 0;
It seems wrong to add something like this to TabController, but I couldn't think
of a better way to make this information available in Tab.

[sky, 2015-02-07 00:09:39]

https://codereview.chromium.org/893843007/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/893843007/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6452: +      <message
name="IDS_FLAGS_TAB_CLOSE_BUTTONS_HIDDEN_WITH_TOUCH_NAME" desc="">
Do we really need all these permutations? What are you going to use to make a
decision? Could this just be something you build locally and show ux rather than
landing?

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
chrome/browser/ui/tabs/tab_utils.cc:145: if (switch_value == "always")
This code is basically saying if touch_used_last (and one of these flags is set)
don't show the close button. That conditional can just as well be moved to views
and don't call this function. Which basically means no changes are necessary
here.

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:34: virtual bool
TouchWasUsedAsLastInput() = 0;
On 2015/02/06 20:34:33, tdanderson wrote:
> It seems wrong to add something like this to TabController, but I couldn't
think
> of a better way to make this information available in Tab.

Agreed. Maybe this should be ShouldShowCloseButton?

[tdanderson, 2015-02-10 18:52:17]

Scott, can you please take another look?

https://codereview.chromium.org/893843007/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/893843007/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6452: +      <message
name="IDS_FLAGS_TAB_CLOSE_BUTTONS_HIDDEN_WITH_TOUCH_NAME" desc="">
On 2015/02/07 00:09:39, sky wrote:
> Do we really need all these permutations? What are you going to use to make a
> decision? Could this just be something you build locally and show ux rather
than
> landing?

As discussed, there isn't an easy way for me to do this since I'm
in a different office. I checked with the PM and he said the most
convenient way for UX to see it would be for me to land behind
a flag.

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
chrome/browser/ui/tabs/tab_utils.cc:12: #include
"chrome/browser/ui/views/tabs/tab.h"
On 2015/02/06 20:34:33, tdanderson wrote:
> I added this so that I can access the static methods in Tab which give the
> ideal/touch widths, but this is a violation of the include rules. Can you
> suggest the best way to make the width information available?

No longer an issue since I've moved the new logic into Tab.

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/tabs/tab_u...
chrome/browser/ui/tabs/tab_utils.cc:145: if (switch_value == "always")
On 2015/02/07 00:09:39, sky wrote:
> This code is basically saying if touch_used_last (and one of these flags is
set)
> don't show the close button. That conditional can just as well be moved to
views
> and don't call this function. Which basically means no changes are necessary
> here.

Done.

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/views/tabs...
File chrome/browser/ui/views/tabs/tab_controller.h (right):

https://codereview.chromium.org/893843007/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/tab_controller.h:34: virtual bool
TouchWasUsedAsLastInput() = 0;
On 2015/02/07 00:09:39, sky wrote:
> On 2015/02/06 20:34:33, tdanderson wrote:
> > It seems wrong to add something like this to TabController, but I couldn't
> think
> > of a better way to make this information available in Tab.
> 
> Agreed. Maybe this should be ShouldShowCloseButton?

Done.

[sky, 2015-02-10 20:09:10]

LGTM

[tdanderson, 2015-02-10 21:54:10]

New patchsets have been uploaded after l-g-t-m from sky@chromium.org

[tdanderson, 2015-02-10 21:59:00]

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

[tdanderson, 2015-02-10 21:59:00]

sievers@: Can you PTAL at my changes in content/ ?

mpearson@: Can you PTAL at my changes in tools/ ?

[no sievers, 2015-02-10 22:07:56]

On 2015/02/10 21:59:00, tdanderson wrote:
> sievers@: Can you PTAL at my changes in content/ ?
> 
> 
lgtm

[Mark P, 2015-02-10 23:10:54]

histograms.xml lgtm

[tdanderson, 2015-02-10 23:24:04]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2015-02-10 23:25:15]

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

[commit-bot: I haz the power, 2015-02-11 00:19:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-11 00:20:54]

Patchset 3 (id:??) landed as
https://crrev.com/8edd6e43a27564d8fab055d29c2177b253541495
Cr-Commit-Position: refs/heads/master@{#315684}