Mac: Correctly clip tab title...

Mac: Correctly clip tab title

When lots of tabs were created tab titles would sometimes overflow onto the close buttons or onto other tabs.

I think this is a problem with NSView's autoresize code. It doesn't handle the case where a view has been resized to 0 width.

To fix this I'm changing the tab layout code not to depend on NSView's autoresize behavior. The old code would let NSView resize the title view then grow or shrink the title as needed. The new code always sets the title view's frame a specific distance from the super frame.

BUG=32268
TEST=Opened lots of tabs and resized the browser window. Verified that the title never overlapped the close button. Added a new unit test to verify the layout of the title view.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69342

[sail, 2010-12-14 00:01:09]

Updating reviewers list.

[viettrungluu, 2010-12-14 00:19:50]

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
File chrome/browser/ui/cocoa/tab_controller.h (right):

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.h:110: - (NSTextField*)titleView;
Maybe we should just use |@property|s for these?

(The Right Thing™ to do is probably to use |IBOutlet|s on |@property|s too, and
hook up the .xib to the setters, but I've never managed to convince people to do
this.)

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
File chrome/browser/ui/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:242: BOOL newShowIcon = [self
shouldShowIcon] ? YES : NO;
The |? YES : NO| here is just strange. Could you get rid of it as a drive-by?

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:244: [iconView_ setHidden:newShowIcon
? NO : YES];
Ditto. Should just be |!newShowIcon|.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:250: BOOL newShowCloseButton = [self
shouldShowCloseButton] ? YES : NO;
Etc.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:258: newTitleFrame.size.height =
oldTitleFrame.size.height;
NSMakeRect and a couple of uses of ?: would be more concise.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:269: NSMinX(newTitleFrame);
Presumably you may want to calculate the x-coordinate of newTitleFrame('s
origin) before the NSMakeRect. In any case, you shouldn't pass the
partially-defined newTitleFrame into NSMinX....

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:272: NSMinX(newTitleFrame);
"

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
File chrome/browser/ui/cocoa/tab_controller_unittest.mm (right):

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller_unittest.mm:65: static CGFloat
LeftMargin(NSRect superFrame, NSRect subFrame);
|static|s aren't necessary in an anonymous namespace.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller_unittest.mm:328: static CGFloat
LeftMargin(NSRect superFrame, NSRect subFrame) {
Just define these directly at the top.

[sail, 2010-12-14 01:59:53]

Addressed review comments.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
File chrome/browser/ui/cocoa/tab_controller.h (right):

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.h:110: - (NSTextField*)titleView;
On 2010/12/14 00:19:50, viettrungluu wrote:
> Maybe we should just use |@property|s for these?
> 
> (The Right Thing™ to do is probably to use |IBOutlet|s on |@property|s too,
and
> hook up the .xib to the setters, but I've never managed to convince people to
do
> this.)

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
File chrome/browser/ui/cocoa/tab_controller.mm (right):

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:242: BOOL newShowIcon = [self
shouldShowIcon] ? YES : NO;
On 2010/12/14 00:19:50, viettrungluu wrote:
> The |? YES : NO| here is just strange. Could you get rid of it as a drive-by?

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:244: [iconView_ setHidden:newShowIcon
? NO : YES];
On 2010/12/14 00:19:50, viettrungluu wrote:
> Ditto. Should just be |!newShowIcon|.

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:250: BOOL newShowCloseButton = [self
shouldShowCloseButton] ? YES : NO;
On 2010/12/14 00:19:50, viettrungluu wrote:
> Etc.

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:258: newTitleFrame.size.height =
oldTitleFrame.size.height;
On 2010/12/14 00:19:50, viettrungluu wrote:
> NSMakeRect and a couple of uses of ?: would be more concise.

Unfortunately I need a temporary variable here since the width depends on the
origin.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:269: NSMinX(newTitleFrame);
On 2010/12/14 00:19:50, viettrungluu wrote:
> Presumably you may want to calculate the x-coordinate of newTitleFrame('s
> origin) before the NSMakeRect. In any case, you shouldn't pass the
> partially-defined newTitleFrame into NSMinX....

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller.mm:272: NSMinX(newTitleFrame);
On 2010/12/14 00:19:50, viettrungluu wrote:
> "

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
File chrome/browser/ui/cocoa/tab_controller_unittest.mm (right):

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller_unittest.mm:65: static CGFloat
LeftMargin(NSRect superFrame, NSRect subFrame);
On 2010/12/14 00:19:50, viettrungluu wrote:
> |static|s aren't necessary in an anonymous namespace.

Cool! Didn't know that.

Done.

http://codereview.chromium.org/5703007/diff/1/chrome/browser/ui/cocoa/tab_con...
chrome/browser/ui/cocoa/tab_controller_unittest.mm:328: static CGFloat
LeftMargin(NSRect superFrame, NSRect subFrame) {
On 2010/12/14 00:19:50, viettrungluu wrote:
> Just define these directly at the top.

Done.

[viettrungluu, 2010-12-14 17:47:13]

LGTM