Removes --enable-stacked-tab-strip flag (Stacked Tabs)

Removes --enable-stacked-tab-strip flag (Stacked Tabs).

 There is no intention to ship and support this flag and it is disabled 
by default. Stacked tabs are enabled on touch platforms (without the flag).

BUG=332077
TEST=None
R=sky@chromium.org

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

[varkha, 2014-05-20 20:35:11]

Can you please take a look? This has been discussed in the bug and there is not
too much resistance to this change.

[tfarina, 2014-05-20 20:45:04]

Could you say in the CL description, why are you removing it?

Is it enabled by default now? I have no idea.

[Peter Kasting, 2014-05-20 20:48:15]

You also need to update chrome/browser/ui/tabs/tab_strip_layout_type.h not to
refer to command-line options; but see comments below.

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

https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:75: return
TAB_STRIP_LAYOUT_SHRINK;
While you're touching this code, it looks like there are some other changes that
could be made.

(1) The comment at the top seems confusing, as the code is really about non-Ash,
not Ash.
(2) I don't see why the |adjust_layout| parameter is necessary.
(3) The end of the function could be written more simply as

  return (prefs->GetInteger(prefs::kTabStripLayoutType) ==
      TAB_STRIP_LAYOUT_STACKED) ?
      TAB_STRIP_LAYOUT_STACKED : TAB_STRIP_LAYOUT_SHRINK;

But frankly, this suggests that instead of using an integral pref and enumerated
states everywhere, we should do something like switch everything to a bool named
"stacked_tabs" or something.  And I don't see why this can't all be handled in
the tabstrip, instead of having to affect the BrowserTabStripController.

sky wrote all this stuff, so you should really send this feedback to him, not
me.

[varkha, 2014-05-21 15:03:25]

+sky@. Can you please take a look at the previous comment?

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

https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:75: return
TAB_STRIP_LAYOUT_SHRINK;
On 2014/05/20 20:48:15, Peter Kasting wrote:
> While you're touching this code, it looks like there are some other changes
that
> could be made.
> 
> (1) The comment at the top seems confusing, as the code is really about
non-Ash,
> not Ash.
> (2) I don't see why the |adjust_layout| parameter is necessary.
> (3) The end of the function could be written more simply as
> 
>   return (prefs->GetInteger(prefs::kTabStripLayoutType) ==
>       TAB_STRIP_LAYOUT_STACKED) ?
>       TAB_STRIP_LAYOUT_STACKED : TAB_STRIP_LAYOUT_SHRINK;
> 
> But frankly, this suggests that instead of using an integral pref and
enumerated
> states everywhere, we should do something like switch everything to a bool
named
> "stacked_tabs" or something.  And I don't see why this can't all be handled in
> the tabstrip, instead of having to affect the BrowserTabStripController.
> 
> sky wrote all this stuff, so you should really send this feedback to him, not
> me.

+sky@, should I maybe refactor this to boolean state in this CL?

[sky, 2014-05-21 16:30:08]

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

https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:75: return
TAB_STRIP_LAYOUT_SHRINK;
On 2014/05/21 15:03:25, varkha wrote:
> On 2014/05/20 20:48:15, Peter Kasting wrote:
> > While you're touching this code, it looks like there are some other changes
> that
> > could be made.
> > 
> > (1) The comment at the top seems confusing, as the code is really about
> non-Ash,
> > not Ash.

Agreed.

> > (2) I don't see why the |adjust_layout| parameter is necessary.
> > (3) The end of the function could be written more simply as
> > 
> >   return (prefs->GetInteger(prefs::kTabStripLayoutType) ==
> >       TAB_STRIP_LAYOUT_STACKED) ?
> >       TAB_STRIP_LAYOUT_STACKED : TAB_STRIP_LAYOUT_SHRINK;

There are really three states:
1 shrink
2 shrink and can switch to stacked
3 stacked

adjust_layout distinguishes 1 and 3.

> > But frankly, this suggests that instead of using an integral pref and
> enumerated
> > states everywhere, we should do something like switch everything to a bool
> named
> > "stacked_tabs" or something.  And I don't see why this can't all be handled
in
> > the tabstrip, instead of having to affect the BrowserTabStripController.

I've tried to minimize the dependencies of the tabstrip on chrome.
TabStripController handles those.

> > sky wrote all this stuff, so you should really send this feedback to him,
not
> > me.
> 
> +sky@, should I maybe refactor this to boolean state in this CL?

Yes, a boolean value is fine.

[sky, 2014-05-21 16:30:29]

On 2014/05/21 16:30:08, sky wrote:
>
https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs...
> File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):
> 
>
https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs...
> chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:75: return
> TAB_STRIP_LAYOUT_SHRINK;
> On 2014/05/21 15:03:25, varkha wrote:
> > On 2014/05/20 20:48:15, Peter Kasting wrote:
> > > While you're touching this code, it looks like there are some other
changes
> > that
> > > could be made.
> > > 
> > > (1) The comment at the top seems confusing, as the code is really about
> > non-Ash,
> > > not Ash.
> 
> Agreed.
> 
> > > (2) I don't see why the |adjust_layout| parameter is necessary.
> > > (3) The end of the function could be written more simply as
> > > 
> > >   return (prefs->GetInteger(prefs::kTabStripLayoutType) ==
> > >       TAB_STRIP_LAYOUT_STACKED) ?
> > >       TAB_STRIP_LAYOUT_STACKED : TAB_STRIP_LAYOUT_SHRINK;
> 
> There are really three states:
> 1 shrink
> 2 shrink and can switch to stacked
> 3 stacked
> 
> adjust_layout distinguishes 1 and 3.

Sorry, that should be 1 and 2.

> 
> > > But frankly, this suggests that instead of using an integral pref and
> > enumerated
> > > states everywhere, we should do something like switch everything to a bool
> > named
> > > "stacked_tabs" or something.  And I don't see why this can't all be
handled
> in
> > > the tabstrip, instead of having to affect the BrowserTabStripController.
> 
> I've tried to minimize the dependencies of the tabstrip on chrome.
> TabStripController handles those.
> 
> > > sky wrote all this stuff, so you should really send this feedback to him,
> not
> > > me.
> > 
> > +sky@, should I maybe refactor this to boolean state in this CL?
> 
> Yes, a boolean value is fine.

[varkha, 2014-05-21 16:53:43]

sky@, I plan to:
1. Add a separate TabStrip::set_adjust_layout() that would be called only from
BrowserTabStripController::UpdateLayoutType().
2. Rename SetLayoutType to SetLayoutTypeStacked(bool) and eliminate
adjust_layout parameter from it (all cases where it is called with
|adjust_layout|=true are the cases where adjust_layout_ is already true so they
don't change it).
3. Rename TabStrip::layout_type_ to bool layout_type_stacked_.
4. Rename TabStrip::layout_type() to bool layout_type_stacked().
5. Rename DetermineTabStripLayout(...) to bool
DetermineTabStripLayoutStacked(...).

[varkha, 2014-05-21 16:57:44]

Also adjust_layout is always true on ash desktops and false otherwise (as set in
DetermineTabStripLayout).
Do we need to determine this in DetermineTabStripLayout or we could just set
adjust_layout_ to GetWidget()->GetNativeView()) == chrome::HOST_DESKTOP_TYPE_ASH
in TabStrip::Init (and remove the adjust_layout elsewhere)?

[sky, 2014-05-21 17:34:30]

On Wed, May 21, 2014 at 9:57 AM,  <varkha@chromium.org> wrote:
> Also adjust_layout is always true on ash desktops and false otherwise (as
> set in
> DetermineTabStripLayout).
> Do we need to determine this in DetermineTabStripLayout or we could just set
> adjust_layout_ to GetWidget()->GetNativeView()) ==
> chrome::HOST_DESKTOP_TYPE_ASH
> in TabStrip::Init (and remove the adjust_layout elsewhere)?

I think that'll work. In fact there is already
TabStrip::GetAdjustLayout, which pretty much does what you want.

  -Scott
>
> https://codereview.chromium.org/291093005/

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

[Peter Kasting, 2014-05-21 19:17:46]

I'm going to take myself off this and let sky handle the review from here on.

[varkha, 2014-05-21 21:11:13]

PTAL. I've tried to preserve the original flow since calling GetWidget() is not
yet OK in TabStrip::Init.

[sky, 2014-05-21 22:57:19]

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:72:
TAB_STRIP_LAYOUT_STACKED);
Seems silly to keep the enum and not use it. I believe Peter was suggesting you
nuke the enum entirely, which I'm ok with. Or at least move it here so that you
document the values that are written to prefs.

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

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:2643: bool TabStrip::GetAdjustLayout()
const {
I think you can remove this and just use adjust_layout_.

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.h:71: // Sets if the layout type is
stacked.
You also need to describe what these two values mean.

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.h:613: bool layout_type_stacked_;
How about stacked_layout_?

[varkha, 2014-05-23 17:43:23]

PTAL.

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right):

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:72:
TAB_STRIP_LAYOUT_STACKED);
On 2014/05/21 22:57:19, sky wrote:
> Seems silly to keep the enum and not use it. I believe Peter was suggesting
you
> nuke the enum entirely, which I'm ok with. Or at least move it here so that
you
> document the values that are written to prefs.

Right, I left the enum as a way to document the values. I have moved the prefs
values documentation and accessors to tab_strip_layout_type_prefs as it seemed
like a logical place for those.

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

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.cc:2643: bool TabStrip::GetAdjustLayout()
const {
On 2014/05/21 22:57:19, sky wrote:
> I think you can remove this and just use adjust_layout_.

Done.

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.h:71: // Sets if the layout type is
stacked.
On 2014/05/21 22:57:19, sky wrote:
> You also need to describe what these two values mean.

Done.

https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/tabs/tab_strip.h:613: bool layout_type_stacked_;
On 2014/05/21 22:57:19, sky wrote:
> How about stacked_layout_?

Done.

[Peter Kasting, 2014-05-23 17:48:45]

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: //     the user uses a
touch device.
Don't use an integer pref for this, use a boolean pref.  Name the pref something
like kAllowStackedTabs.

Once this is done, all this documentation is unnecessary, and this whole file
should go away; BrowserTabStripController can just read/write the prefs
directly.

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:31:
prefs->SetInteger(prefs::kTabStripLayoutType, stacked_layout ? 0 : 1);
Isn't this backwards?  Using a bool pref with a good name would make this clear.

[varkha, 2014-05-23 17:51:56]

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: //     the user uses a
touch device.
On 2014/05/23 17:48:46, Peter Kasting wrote:
> Don't use an integer pref for this, use a boolean pref.  Name the pref
something
> like kAllowStackedTabs.
> 
> Once this is done, all this documentation is unnecessary, and this whole file
> should go away; BrowserTabStripController can just read/write the prefs
> directly.

That would be great but don't we want to preserve existing user settings?

[Peter Kasting, 2014-05-23 17:53:08]

On 2014/05/23 17:51:56, varkha wrote:
>
https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
> File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):
> 
>
https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
> chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: //     the user uses
a
> touch device.
> On 2014/05/23 17:48:46, Peter Kasting wrote:
> > Don't use an integer pref for this, use a boolean pref.  Name the pref
> something
> > like kAllowStackedTabs.
> > 
> > Once this is done, all this documentation is unnecessary, and this whole
file
> > should go away; BrowserTabStripController can just read/write the prefs
> > directly.
> 
> That would be great but don't we want to preserve existing user settings?

If you really think that's important, you can do a simple pref migration: Check
for the old pref, if it exists write the value to the new pref and then delete
the old pref.  We do this in many other cases.

[varkha, 2014-05-23 21:45:47]

PTAL.

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: //     the user uses a
touch device.
On 2014/05/23 17:48:46, Peter Kasting wrote:
> Don't use an integer pref for this, use a boolean pref.  Name the pref
something
> like kAllowStackedTabs.
> 
> Once this is done, all this documentation is unnecessary, and this whole file
> should go away; BrowserTabStripController can just read/write the prefs
> directly.

Done. The file is still there (unmodified) since it has a registration call.

https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:31:
prefs->SetInteger(prefs::kTabStripLayoutType, stacked_layout ? 0 : 1);
On 2014/05/23 17:48:46, Peter Kasting wrote:
> Isn't this backwards?  Using a bool pref with a good name would make this
clear.

Exactly! That's why getting rid of this in favour of a bool pref is a good idea.

[Peter Kasting, 2014-05-23 21:47:31]

I still don't understand why tab_strip_layout_type_prefs.* exist at all.  Why
can't all this just be moved into the BrowserTabStripController, which is the
only client of these prefs?

[varkha, 2014-05-23 22:08:03]

On 2014/05/23 21:47:31, Peter Kasting wrote:
> I still don't understand why tab_strip_layout_type_prefs.* exist at all.  Why
> can't all this just be moved into the BrowserTabStripController, which is the
> only client of these prefs?

I was trying to get the registration done early in the application start-up
following the comment in DeprecatedGetPrefRegistry
(https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...).
I could move the registration and migration into static methods in
BrowserTabStripController, would that be better?

[Peter Kasting, 2014-05-23 22:19:38]

On 2014/05/23 22:08:03, varkha wrote:
> On 2014/05/23 21:47:31, Peter Kasting wrote:
> > I still don't understand why tab_strip_layout_type_prefs.* exist at all. 
Why
> > can't all this just be moved into the BrowserTabStripController, which is
the
> > only client of these prefs?
> 
> I was trying to get the registration done early in the application start-up
> following the comment in DeprecatedGetPrefRegistry
>
(https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/pref_se...).
> I could move the registration and migration into static methods in
> BrowserTabStripController, would that be better?

Yes, that's what I've been hoping for.

[varkha, 2014-05-23 22:26:30]

Well, I cannot move the registration and migration into
BrowserTabStripController because of "-chrome/browser/ui/views" from
chrome/browser's include_rule. But I could move it to
chrome/browser/ui/browser_view_prefs. Would that be right?

[Peter Kasting, 2014-05-23 22:38:43]

On 2014/05/23 22:26:30, varkha wrote:
> Well, I cannot move the registration and migration into
> BrowserTabStripController because of "-chrome/browser/ui/views" from
> chrome/browser's include_rule. But I could move it to
> chrome/browser/ui/browser_view_prefs. Would that be right?

The real issue is that since this is all views-specific functionality, it all
belongs in a views-related location.  Scott, is there any sort of views-specific
pref registration phase?  I couldn't find one, but just doing this globally
seems lame.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:18: // Old values: 0 =
SHRINK (default),  1 = STACKED
This comment will be obvious once this constant is declared in the function
below (see next comment), and should be removed.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:19: const char
kTabStripLayoutType[] = "tab_strip_layout_type";
Declare constants as locally as possible, in this case in the function below.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:24: // This value is
device dependent, so it goes in local state.
This function doesn't necessarily know whether |registry| represents local
state, so this comment doesn't belong in this function.  I would just remove
this.  If it needs to be said, it should be said in a header or something.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:29: void
MigratePrefs(PrefService* pref) {
Nit: Call this "prefs" -- "pref" sounds like one pref.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:31: // boolean value
indicating stacked layout preference.
This comment is confusing and redundant with the code; remove it.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:34:
pref->SetBoolean(prefs::kTabStripStackedLayout, true);
Nit: Should we just do:

    pref->SetBoolean(prefs::kTabStripStackedLayout,
                     pref->GetInteger(kTabStripLayoutType) != 0);

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.h (right):

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.h:12: namespace
tab_strip_layout_type_prefs {
Why do we need another namespace?  We shouldn't have nested namespaces in
Chromium code in any case.

Frankly, we're trying to kill the chrome:: namespace as well, so I'd remove both
these namespaces.

[varkha, 2014-05-23 22:39:55]

Moved it to chrome/browser/ui/browser_view_prefs.* to avoid including
chrome/browser/ui/views from chrome/browser.

[varkha, 2014-05-23 22:50:55]

Moved to chrome/browser/ui/browser_view_prefs.*, the name suggesting that this
is views specific location for prefs registration.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:18: // Old values: 0 =
SHRINK (default),  1 = STACKED
On 2014/05/23 22:38:43, Peter Kasting wrote:
> This comment will be obvious once this constant is declared in the function
> below (see next comment), and should be removed.

See my next comment.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:19: const char
kTabStripLayoutType[] = "tab_strip_layout_type";
On 2014/05/23 22:38:43, Peter Kasting wrote:
> Declare constants as locally as possible, in this case in the function below.

There are two functions that use the constant.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:24: // This value is
device dependent, so it goes in local state.
On 2014/05/23 22:38:43, Peter Kasting wrote:
> This function doesn't necessarily know whether |registry| represents local
> state, so this comment doesn't belong in this function.  I would just remove
> this.  If it needs to be said, it should be said in a header or something.

Done (removed).

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:29: void
MigratePrefs(PrefService* pref) {
On 2014/05/23 22:38:43, Peter Kasting wrote:
> Nit: Call this "prefs" -- "pref" sounds like one pref.

Done.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:31: // boolean value
indicating stacked layout preference.
On 2014/05/23 22:38:43, Peter Kasting wrote:
> This comment is confusing and redundant with the code; remove it.

Moved to header and simplified.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:34:
pref->SetBoolean(prefs::kTabStripStackedLayout, true);
On 2014/05/23 22:38:43, Peter Kasting wrote:
> Nit: Should we just do:
> 
>     pref->SetBoolean(prefs::kTabStripStackedLayout,
>                      pref->GetInteger(kTabStripLayoutType) != 0);

Done.

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.h (right):

https://codereview.chromium.org/291093005/diff/180001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_strip_layout_type_prefs.h:12: namespace
tab_strip_layout_type_prefs {
On 2014/05/23 22:38:43, Peter Kasting wrote:
> Why do we need another namespace?  We shouldn't have nested namespaces in
> Chromium code in any case.
> 
> Frankly, we're trying to kill the chrome:: namespace as well, so I'd remove
both
> these namespaces.

Code moved so this namespace is not necessary.

[varkha, 2014-05-28 19:03:01]

ping?

[Peter Kasting, 2014-05-28 19:12:50]

LGTM, sky should be the final authority on location

[Peter Kasting, 2014-05-28 19:13:05]

Forgot to send this trivial nit

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:26: // Old values: 0 = SHRINK (default),
 1 = STACKED
Nit: Extra space

[sky, 2014-05-28 19:45:55]

LGTM

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:60: }
Do you need to schedule a save here?

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.h:66: // If |adjust_layout| is true the
layout type changes
layout type-> stacked layout. In fact you'll likely want to make this change in
other places.

https://codereview.chromium.org/291093005/diff/240001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/common/pref_name...
chrome/common/pref_names.cc:2104: // Boolean indicating  whether tabstrip uses
stacked layout (on touch devices).
nit: you've got two spaces after indicating.

[varkha, 2014-05-28 22:15:55]

Thanks for all the suggestions!

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:26: // Old values: 0 = SHRINK (default),
 1 = STACKED
On 2014/05/28 19:13:06, Peter Kasting wrote:
> Nit: Extra space

Done.

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:60: }
On 2014/05/28 19:45:56, sky wrote:
> Do you need to schedule a save here?

I looked at similar methods and none of them have that. Do you think this is
missing in all of them or in some other place? Do we really need to explicitly
sync something like this?

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.h (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.h:66: // If |adjust_layout| is true the
layout type changes
On 2014/05/28 19:45:56, sky wrote:
> layout type-> stacked layout. In fact you'll likely want to make this change
in
> other places.

Done.

https://codereview.chromium.org/291093005/diff/240001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/common/pref_name...
chrome/common/pref_names.cc:2104: // Boolean indicating  whether tabstrip uses
stacked layout (on touch devices).
On 2014/05/28 19:45:56, sky wrote:
> nit: you've got two spaces after indicating.

Done.

[sky, 2014-05-28 23:12:30]

SLGTM

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_view_prefs.cc (right):

https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/brows...
chrome/browser/ui/browser_view_prefs.cc:60: }
On 2014/05/28 22:15:56, varkha wrote:
> On 2014/05/28 19:45:56, sky wrote:
> > Do you need to schedule a save here?
> 
> I looked at similar methods and none of them have that. Do you think this is
> missing in all of them or in some other place? Do we really need to explicitly
> sync something like this?

It's probably that the prefs code has changes since I remember it. If you don't
need to explicitly tell prefs store its dirty, great!

[varkha, 2014-05-28 23:15:39]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-28 23:17:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/291093005/250001

[varkha, 2014-05-29 15:12:15]

The CQ bit was unchecked by varkha@chromium.org

[varkha, 2014-05-29 15:17:14]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2014-05-29 15:20:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/varkha@chromium.org/291093005/260001

[commit-bot: I haz the power, 2014-05-29 19:28:19]

Change committed as 273543