Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(803)

Issue 291093005: Removes --enable-stacked-tab-strip flag (Stacked Tabs) (Closed)

Created:
6 years, 7 months ago by varkha
Modified:
6 years, 6 months ago
Reviewers:
Peter Kasting, sky
CC:
chromium-reviews, tfarina
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : Removes --enable-stacked-tab-strip flag (minor refactoring) #

Total comments: 8

Patch Set 3 : Removes --enable-stacked-tab-strip flag (more refactoring) #

Total comments: 5

Patch Set 4 : Removes --enable-stacked-tab-strip flag (more refactoring) #

Total comments: 14

Patch Set 5 : Removes --enable-stacked-tab-strip flag (moved pref logic to browser_view_prefs) #

Patch Set 6 : Removes --enable-stacked-tab-strip flag (moved pref logic to browser_view_prefs) #

Total comments: 9

Patch Set 7 : Removes --enable-stacked-tab-strip flag (nits) #

Patch Set 8 : Removes --enable-stacked-tab-strip flag (removed stale header) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+120 lines, -190 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/prefs/browser_prefs.cc View 1 2 3 4 5 6 7 3 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/ui/browser_view_prefs.h View 1 2 3 4 5 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_view_prefs.cc View 1 2 3 4 5 6 4 chunks +16 lines, -0 lines 0 comments Download
D chrome/browser/ui/tabs/tab_strip_layout_type.h View 1 2 1 chunk +0 lines, -21 lines 0 comments Download
M chrome/browser/ui/tabs/tab_strip_layout_type_prefs.h View 1 2 3 4 1 chunk +0 lines, -17 lines 0 comments Download
M chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc View 1 2 3 4 1 chunk +0 lines, -19 lines 0 comments Download
M chrome/browser/ui/views/tabs/browser_tab_strip_controller.h View 1 2 3 4 5 6 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc View 1 2 3 4 5 6 7 6 chunks +21 lines, -29 lines 0 comments Download
M chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/views/tabs/tab_strip.h View 1 2 3 4 5 6 6 chunks +25 lines, -19 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_strip.cc View 1 2 3 4 5 6 17 chunks +34 lines, -44 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_strip_controller.h View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/tabs/tab_strip_unittest.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 2 chunks +0 lines, -8 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 6 7 3 chunks +5 lines, -4 lines 0 comments Download

Messages

Total messages: 36 (0 generated)
varkha
Can you please take a look? This has been discussed in the bug and there ...
6 years, 7 months ago (2014-05-20 20:35:11 UTC) #1
tfarina
Could you say in the CL description, why are you removing it? Is it enabled ...
6 years, 7 months ago (2014-05-20 20:45:04 UTC) #2
Peter Kasting
You also need to update chrome/browser/ui/tabs/tab_strip_layout_type.h not to refer to command-line options; but see comments ...
6 years, 7 months ago (2014-05-20 20:48:15 UTC) #3
varkha
+sky@. Can you please take a look at the previous comment? https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): ...
6 years, 7 months ago (2014-05-21 15:03:25 UTC) #4
sky
https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc#newcode75 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 ...
6 years, 7 months ago (2014-05-21 16:30:08 UTC) #5
sky
On 2014/05/21 16:30:08, sky wrote: > https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc > File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): > > https://codereview.chromium.org/291093005/diff/1/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc#newcode75 > ...
6 years, 7 months ago (2014-05-21 16:30:29 UTC) #6
varkha
sky@, I plan to: 1. Add a separate TabStrip::set_adjust_layout() that would be called only from ...
6 years, 7 months ago (2014-05-21 16:53:43 UTC) #7
varkha
Also adjust_layout is always true on ash desktops and false otherwise (as set in DetermineTabStripLayout). ...
6 years, 7 months ago (2014-05-21 16:57:44 UTC) #8
sky
On Wed, May 21, 2014 at 9:57 AM, <varkha@chromium.org> wrote: > Also adjust_layout is always ...
6 years, 7 months ago (2014-05-21 17:34:30 UTC) #9
Peter Kasting
I'm going to take myself off this and let sky handle the review from here ...
6 years, 7 months ago (2014-05-21 19:17:46 UTC) #10
varkha
PTAL. I've tried to preserve the original flow since calling GetWidget() is not yet OK ...
6 years, 7 months ago (2014-05-21 21:11:13 UTC) #11
sky
https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc#newcode72 chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:72: TAB_STRIP_LAYOUT_STACKED); Seems silly to keep the enum and not ...
6 years, 7 months ago (2014-05-21 22:57:19 UTC) #12
varkha
PTAL. https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/291093005/diff/60001/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc#newcode72 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 ...
6 years, 7 months ago (2014-05-23 17:43:23 UTC) #13
Peter Kasting
https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right): https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc#newcode17 chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: // the user uses a touch device. Don't use ...
6 years, 7 months ago (2014-05-23 17:48:45 UTC) #14
varkha
https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right): https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc#newcode17 chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: // the user uses a touch device. On 2014/05/23 ...
6 years, 7 months ago (2014-05-23 17:51:56 UTC) #15
Peter Kasting
On 2014/05/23 17:51:56, varkha wrote: > https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc > File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right): > > https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc#newcode17 > ...
6 years, 7 months ago (2014-05-23 17:53:08 UTC) #16
varkha
PTAL. https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc File chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc (right): https://codereview.chromium.org/291093005/diff/120001/chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc#newcode17 chrome/browser/ui/tabs/tab_strip_layout_type_prefs.cc:17: // the user uses a touch device. On ...
6 years, 7 months ago (2014-05-23 21:45:47 UTC) #17
Peter Kasting
I still don't understand why tab_strip_layout_type_prefs.* exist at all. Why can't all this just be ...
6 years, 7 months ago (2014-05-23 21:47:31 UTC) #18
varkha
On 2014/05/23 21:47:31, Peter Kasting wrote: > I still don't understand why tab_strip_layout_type_prefs.* exist at ...
6 years, 7 months ago (2014-05-23 22:08:03 UTC) #19
Peter Kasting
On 2014/05/23 22:08:03, varkha wrote: > On 2014/05/23 21:47:31, Peter Kasting wrote: > > I ...
6 years, 7 months ago (2014-05-23 22:19:38 UTC) #20
varkha
Well, I cannot move the registration and migration into BrowserTabStripController because of "-chrome/browser/ui/views" from chrome/browser's ...
6 years, 7 months ago (2014-05-23 22:26:30 UTC) #21
Peter Kasting
On 2014/05/23 22:26:30, varkha wrote: > Well, I cannot move the registration and migration into ...
6 years, 7 months ago (2014-05-23 22:38:43 UTC) #22
varkha
Moved it to chrome/browser/ui/browser_view_prefs.* to avoid including chrome/browser/ui/views from chrome/browser.
6 years, 7 months ago (2014-05-23 22:39:55 UTC) #23
varkha
Moved to chrome/browser/ui/browser_view_prefs.*, the name suggesting that this is views specific location for prefs registration. ...
6 years, 7 months ago (2014-05-23 22:50:55 UTC) #24
varkha
ping?
6 years, 6 months ago (2014-05-28 19:03:01 UTC) #25
Peter Kasting
LGTM, sky should be the final authority on location
6 years, 6 months ago (2014-05-28 19:12:50 UTC) #26
Peter Kasting
Forgot to send this trivial nit https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc File chrome/browser/ui/browser_view_prefs.cc (right): https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc#newcode26 chrome/browser/ui/browser_view_prefs.cc:26: // Old values: ...
6 years, 6 months ago (2014-05-28 19:13:05 UTC) #27
sky
LGTM https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc File chrome/browser/ui/browser_view_prefs.cc (right): https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc#newcode60 chrome/browser/ui/browser_view_prefs.cc:60: } Do you need to schedule a save ...
6 years, 6 months ago (2014-05-28 19:45:55 UTC) #28
varkha
Thanks for all the suggestions! https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc File chrome/browser/ui/browser_view_prefs.cc (right): https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc#newcode26 chrome/browser/ui/browser_view_prefs.cc:26: // Old values: 0 ...
6 years, 6 months ago (2014-05-28 22:15:55 UTC) #29
sky
SLGTM https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc File chrome/browser/ui/browser_view_prefs.cc (right): https://codereview.chromium.org/291093005/diff/240001/chrome/browser/ui/browser_view_prefs.cc#newcode60 chrome/browser/ui/browser_view_prefs.cc:60: } On 2014/05/28 22:15:56, varkha wrote: > On ...
6 years, 6 months ago (2014-05-28 23:12:30 UTC) #30
varkha
The CQ bit was checked by varkha@chromium.org
6 years, 6 months ago (2014-05-28 23:15:39 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/291093005/250001
6 years, 6 months ago (2014-05-28 23:17:19 UTC) #32
varkha
The CQ bit was unchecked by varkha@chromium.org
6 years, 6 months ago (2014-05-29 15:12:15 UTC) #33
varkha
The CQ bit was checked by varkha@chromium.org
6 years, 6 months ago (2014-05-29 15:17:14 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/varkha@chromium.org/291093005/260001
6 years, 6 months ago (2014-05-29 15:20:37 UTC) #35
commit-bot: I haz the power
6 years, 6 months ago (2014-05-29 19:28:19 UTC) #36
Message was sent while issue was closed.
Change committed as 273543

Powered by Google App Engine
This is Rietveld 408576698