Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476313003:
Mac: Improve tab strip layout in case of overflow (Closed)
|
Created:
6 years, 4 months ago by Andre Modified:
6 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionImprove tab strip layout in case of overflow, so that tabs will not draw on top or
behind the fullscreen and profile buttons.
Calculate the maximum number of tabs we can show and hide the rest, similar
to how it works on Linux and Windows.
Also changed so that only the one active tab has a bigger minimum width (and
draws the close button) instead of potentially multiple selected tabs. This also
matches Linux and Windows.
BUG=392137
Committed: https://crrev.com/d564db4ae0cd91a8981bb73e8d1e53fe5c1a066f
Cr-Commit-Position: refs/heads/master@{#293654}
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : Fixes for rsesek #Patch Set 4 : Fix unit test #Patch Set 5 : Rebase and resolve conflict #Patch Set 6 : Rebase conflict #Patch Set 7 : Rebase again #
Total comments: 2
Messages
Total messages: 40 (14 generated)
Patchset #2 (id:20001) has been deleted
rsesek@, please review. noms@, please review for avatar button. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (left): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.mm:59: const CGFloat kAvatarTabStripShrink = 18; This seems to be originally used as the width of the avatar button, but the code has evolved to compute the avatar button more dynamically and I think was incorrectly used. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.mm:63: const CGFloat kFullscreenIconWidth = 32; The fullscreen icon is actually 16pt wide. We were not computing its offset correctly anyway, after the fix we don't need this width anymore. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.mm:332: badgeXOffset -= kFullscreenIconWidth; Width is not enough, we also need to offset for its origin. fullScreenButtonOriginAdjustment is not good to use either, because it's the amount the button is adjusted from its original unknown origin. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.mm:358: rightIndent += -[window fullScreenButtonOriginAdjustment].x; fullScreenButtonOriginAdjustment is not the amount of space to the right of the button, it is the amount of adjustment from its original system defined position. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (left): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:986: availableSpace -= kLastMiniTabSpacing; availableSpaceForNonMini should be adjusted by this amount, not availableSpace. Although it ended up not mattering as this CL evolved. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:972: NSWidth([newTabButton_ frame]) + kNewTabButtonOffset - kTabOverlap; The new tab button also overlaps the right-most tab. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1024: kTabOverlap; I took this logic from TabStrip::GetDesiredTabWidths(). Would have been nice to share this code, but it ended up quite tricky.
andresantoso@chromium.org changed reviewers: + noms@chromium.org, rsesek@chromium.org
Robert, WDYT?
Nice cleanup! https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.mm:318: [[self window] standardWindowButton:NSWindowFullScreenButton]; This returns nil on 10.6, yes? (Rather than say, throwing an exception). https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1027: numberOfNonMiniTabs--; nit: favor pre-increment operator
https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser_window_controller_private.mm:318: [[self window] standardWindowButton:NSWindowFullScreenButton]; On 2014/09/02 18:17:49, rsesek wrote: > This returns nil on 10.6, yes? (Rather than say, throwing an exception). Yes it does. And turns out, it returns nil on Yosemite as well! I've updated the comments to reflect this. https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right): https://codereview.chromium.org/476313003/diff/40001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1027: numberOfNonMiniTabs--; On 2014/09/02 18:17:49, rsesek wrote: > nit: favor pre-increment operator Done.
LGTM
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/60001
lgtm :)
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...) ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/10...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/51501)
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/12...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/12...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/12...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by andresantoso@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andresantoso@chromium.org/476313003/14...
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as 184c5a4396554e9a55b8d57c73f0107002cdfa4e
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d564db4ae0cd91a8981bb73e8d1e53fe5c1a066f Cr-Commit-Position: refs/heads/master@{#293654}
Message was sent while issue was closed.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/476313003/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/476313003/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:384: if (![self isInAnyFullscreenMode] && fullScreenButton) { The comment references OS versions, but your code doesn't make any checks for OS version. You removed the previous check for IsOSLionOrLater(). Is this a mistake?
Message was sent while issue was closed.
https://codereview.chromium.org/476313003/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right): https://codereview.chromium.org/476313003/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser_window_controller_private.mm:384: if (![self isInAnyFullscreenMode] && fullScreenButton) { On 2014/09/11 01:20:54, erikchen wrote: > The comment references OS versions, but your code doesn't make any checks for OS > version. You removed the previous check for IsOSLionOrLater(). Is this a > mistake? It was intentional. In 10.6 and 10.10, fullScreenButton above will be nil because the button is not present. See comments in line 345. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
