[Mac] Website settings: Implement custom tab appearance.

[Mac] Website settings: Implement custom tab appearance.


BUG=
TEST=

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

[Patrick Dubroy, 2012-07-26 02:07:04]

Hi Robert,

Please take a look. This CL adds a custom appearance to the tabs (Glen wants the
same look on all platforms). It still needs some polish, so I won't be
committing this as-is, but I wanted to get your high-level feedback on the
approach before I go any further.

Thanks.

[Robert Sesek, 2012-07-26 02:49:28]

High-level looks good. Can you attach a screen shot?

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:7: #import
<AppKit/NSCell.h>
Is this necessary? You import Cocoa.h in the .h, which should bring in AppKit.h.
The convention is to not import individual headers and to use the umbrella
header (AppKit.h) instead.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:80: @interface
CustomTabbedSegmentedCell : NSSegmentedCell {
naming: call this WebsiteSettingsTabSegmentedCell

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:90:
ResourceBundle& rb = ResourceBundle::GetSharedInstance();
This should be in an if ((self = [super init])) block

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:108: [context
saveGraphicsState];
There's a scoper in ui/gfx for this.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:119:
withTiledImage:tabBackgroundImage_.get()
nit: if aligning the colons mean that the next line is <2 spaces indented, the
convention is to treat it like a C++ continuation and to put each param on its
own line and indent each 4 spaces.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:134: NO,
For these BOOL params, can you do /*param_name=*/ NO?

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:225:
[segmentedControl_ setWidth:textSize.width + 2 * 28 forSegment:0];
Where's 28 come from?

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:572: [tabView_
selectTabViewItemAtIndex: [segmentedControl_ selectedSegment]];
nit: no space after :

[Patrick Dubroy, 2012-07-28 02:15:02]

I fixed up some visual aspects to be more like Glen's mocks. One of them was
that the bubble should be all white. Not sure how this jives with the fact that
all other bubbles are transparent, but it seems ok to land for now since this is
still behind a flag.

I'll send a screenshot by email.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:7: #import
<AppKit/NSCell.h>
On 2012/07/26 02:49:28, rsesek wrote:
> Is this necessary? You import Cocoa.h in the .h, which should bring in
AppKit.h.
> The convention is to not import individual headers and to use the umbrella
> header (AppKit.h) instead.

You're right. I could have sworn I added this only after a compile failure, but
I can't remember now.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:80: @interface
CustomTabbedSegmentedCell : NSSegmentedCell {
On 2012/07/26 02:49:28, rsesek wrote:
> naming: call this WebsiteSettingsTabSegmentedCell

Done.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:90:
ResourceBundle& rb = ResourceBundle::GetSharedInstance();
On 2012/07/26 02:49:28, rsesek wrote:
> This should be in an if ((self = [super init])) block

Done.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:108: [context
saveGraphicsState];
On 2012/07/26 02:49:28, rsesek wrote:
> There's a scoper in ui/gfx for this.

Not relevant anymore.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:119:
withTiledImage:tabBackgroundImage_.get()
On 2012/07/26 02:49:28, rsesek wrote:
> nit: if aligning the colons mean that the next line is <2 spaces indented, the
> convention is to treat it like a C++ continuation and to put each param on its
> own line and indent each 4 spaces.

Not relevant anymore.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:134: NO,
On 2012/07/26 02:49:28, rsesek wrote:
> For these BOOL params, can you do /*param_name=*/ NO?

Done.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:225:
[segmentedControl_ setWidth:textSize.width + 2 * 28 forSegment:0];
On 2012/07/26 02:49:28, rsesek wrote:
> Where's 28 come from?

Fixed.

http://codereview.chromium.org/10827029/diff/1/chrome/browser/ui/cocoa/websit...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:572: [tabView_
selectTabViewItemAtIndex: [segmentedControl_ selectedSegment]];
On 2012/07/26 02:49:28, rsesek wrote:
> nit: no space after :

Done.

[Robert Sesek, 2012-07-30 22:24:14]

On 2012/07/28 02:15:02, dubroy wrote:
> I fixed up some visual aspects to be more like Glen's mocks. One of them was
> that the bubble should be all white. Not sure how this jives with the fact
that
> all other bubbles are transparent, but it seems ok to land for now since this
is
> still behind a flag.

The bubble should be transparent. There was a UI decision in March that bubbles
(on all platforms, but only currently done on Mac) should be slightly
transparent and have a smaller border-radius.

If it were just setting the background inside the website settings file that'd
be one thing, but this is adding API surface to InfoBubbleWindow, which I don't
think should be done. Bubbles should be consistent across the app.

> I'll send a screenshot by email.

Thanks. Looks pretty nice.

[Patrick Dubroy, 2012-07-30 22:39:15]

On 2012/07/30 22:24:14, rsesek wrote:
> On 2012/07/28 02:15:02, dubroy wrote:
> > I fixed up some visual aspects to be more like Glen's mocks. One of them was
> > that the bubble should be all white. Not sure how this jives with the fact
> that
> > all other bubbles are transparent, but it seems ok to land for now since
this
> is
> > still behind a flag.
> 
> The bubble should be transparent. There was a UI decision in March that
bubbles
> (on all platforms, but only currently done on Mac) should be slightly
> transparent and have a smaller border-radius.
> 
> If it were just setting the background inside the website settings file that'd
> be one thing, but this is adding API surface to InfoBubbleWindow, which I
don't
> think should be done. Bubbles should be consistent across the app.

Ok, that makes sense. I'll double-check with Glen to make sure that's what he
intended.

Any other concerns, or is it ok to land once I fix up the assets?
 
> > I'll send a screenshot by email.
> 
> Thanks. Looks pretty nice.

[Patrick Dubroy, 2012-07-30 22:41:40]

On 2012/07/30 22:39:15, dubroy wrote:
> On 2012/07/30 22:24:14, rsesek wrote:
> > On 2012/07/28 02:15:02, dubroy wrote:
> > > I fixed up some visual aspects to be more like Glen's mocks. One of them
was
> > > that the bubble should be all white. Not sure how this jives with the fact
> > that
> > > all other bubbles are transparent, but it seems ok to land for now since
> this
> > is
> > > still behind a flag.
> > 
> > The bubble should be transparent. There was a UI decision in March that
> bubbles
> > (on all platforms, but only currently done on Mac) should be slightly
> > transparent and have a smaller border-radius.
> > 
> > If it were just setting the background inside the website settings file
that'd
> > be one thing, but this is adding API surface to InfoBubbleWindow, which I
> don't
> > think should be done. Bubbles should be consistent across the app.
> 
> Ok, that makes sense. I'll double-check with Glen to make sure that's what he
> intended.
> 
> Any other concerns, or is it ok to land once I fix up the assets?

I mean, fix up the assets and remove the changes to info_bubble_view.

> > > I'll send a screenshot by email.
> > 
> > Thanks. Looks pretty nice.

[Robert Sesek, 2012-07-31 00:11:38]

I keep forgetting that Rietveld doesn't send these comments when you reply to
the thread. Sorry!

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:108: if (self =
[super init]) {
nit: need extra () around this otherwise you'll likely get a warning from Clang

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:119: 
nit: no blank line

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:161: for (int i =
0; i < segment; ++i) {
segment is NSInteger

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:189: for (int i =
0; i < segmentCount; ++i) {
NSInteger

[Patrick Dubroy, 2012-08-09 14:31:12]

(Back from vacation)

I've gotten rid of the changes that set the background color of the bubble to
white. We'll use the same translucent white background all of the other bubbles.

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:108: if (self =
[super init]) {
On 2012/07/31 00:11:38, rsesek wrote:
> nit: need extra () around this otherwise you'll likely get a warning from
Clang

Done.

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:119: 
On 2012/07/31 00:11:38, rsesek wrote:
> nit: no blank line

Done.

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:161: for (int i =
0; i < segment; ++i) {
On 2012/07/31 00:11:38, rsesek wrote:
> segment is NSInteger

Done.

http://codereview.chromium.org/10827029/diff/7001/chrome/browser/ui/cocoa/web...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:189: for (int i =
0; i < segmentCount; ++i) {
On 2012/07/31 00:11:38, rsesek wrote:
> NSInteger

Done.

[Robert Sesek, 2012-08-09 14:44:00]

Welcome back!

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:105: @end
nit: blank line after

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:122: -
(void)drawWithFrame:(NSRect)cellFrame inView:(NSView *)controlView {
nit: no space before *

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:163: int xAdjust =
(segment == 0) ? kTabStripXPadding : 0;
nit no () around ternary

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:260:
[segmentedControl_ setCell:[[WebsiteSettingsTabSegmentedCell alloc] init]];
This will leak the cell.

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:263:
[segmentedControl_ setAction:@selector(tabSelected:)];
nit: blank line after

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:284: // additional
padding that is not part of the tab.
Why?

[Patrick Dubroy, 2012-08-09 15:33:19]

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
File chrome/browser/ui/cocoa/website_settings_bubble_controller.mm (right):

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:105: @end
On 2012/08/09 14:44:00, rsesek wrote:
> nit: blank line after

Done.

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:122: -
(void)drawWithFrame:(NSRect)cellFrame inView:(NSView *)controlView {
On 2012/08/09 14:44:00, rsesek wrote:
> nit: no space before *

Done.

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:163: int xAdjust =
(segment == 0) ? kTabStripXPadding : 0;
On 2012/08/09 14:44:00, rsesek wrote:
> nit no () around ternary

Done.

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:260:
[segmentedControl_ setCell:[[WebsiteSettingsTabSegmentedCell alloc] init]];
On 2012/08/09 14:44:00, rsesek wrote:
> This will leak the cell.

Fixed.

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:263:
[segmentedControl_ setAction:@selector(tabSelected:)];
On 2012/08/09 14:44:01, rsesek wrote:
> nit: blank line after

Done.

http://codereview.chromium.org/10827029/diff/13001/chrome/browser/ui/cocoa/we...
chrome/browser/ui/cocoa/website_settings_bubble_controller.mm:284: // additional
padding that is not part of the tab.
On 2012/08/09 14:44:01, rsesek wrote:
> Why?

Changed the comment to explain.

[Robert Sesek, 2012-08-09 15:39:43]

LGTM