Mac: Change content settings from showing tabs on top to showing a list on the side.

Mac: Change content settings from showing tabs on top to showing a list on the side.

xib changes:
* Set NSTabView's style to "Tabless" (but kept panes in an NSTabView, since it's a convenient method to handle multiple overlapping views in IB)
* Put NSTabView into a split view, put a table view in the other split pane of the split view, gave it class TabViewSwitcherTable
* changed autosize flags of all child views of the tab view items
* Made the NSTabView the tabView_ of the table view

BUG=45546
TEST=Content settings window should still work. If it's opened through an omnibox bubble, the right section should still be autoselected.

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

[vtl, 2010-06-03 22:15:49]

Can I get a screenshot?

[Nico, 2010-06-03 22:24:32]

On 2010/06/03 22:15:49, vtl wrote:
> Can I get a screenshot?

http://imgur.com/yMfQn&4nPMS (spacing obviously subject to tweaking; I'm hoping
one of the UI guys can help me there. But since that's all in IB, that should be
easy to do.)

[vtl, 2010-06-03 22:26:41]

http://codereview.chromium.org/2534001/diff/11001/12003
File chrome/browser/cocoa/tab_view_picker_table.h (right):

http://codereview.chromium.org/2534001/diff/11001/12003#newcode9
chrome/browser/cocoa/tab_view_picker_table.h:9: // TabViewPickerTable in
interface builder and connect the |tabView_| outlet
Interface Builder (with caps)

http://codereview.chromium.org/2534001/diff/11001/12003#newcode20
chrome/browser/cocoa/tab_view_picker_table.h:20: 
Clearly, this blank line was put here just to anger me.

http://codereview.chromium.org/2534001/diff/11001/12004
File chrome/browser/cocoa/tab_view_picker_table.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12004#newcode68
chrome/browser/cocoa/tab_view_picker_table.mm:68: -
(id)tableView:(NSTableView*)tableView
I'm of the opinion that you should insert spaces between "(id)" and
"tableView:..." so that things line up. This is permitted by the style guide.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode75
chrome/browser/cocoa/tab_view_picker_table.mm:75: - (void)        
tabView:(NSTabView*)tabView
Just like here.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode78
chrome/browser/cocoa/tab_view_picker_table.mm:78: NSInteger index =
This linebreak is clearly unneeded.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode80
chrome/browser/cocoa/tab_view_picker_table.mm:80: [self  
selectRowIndexes:[NSIndexSet indexSetWithIndex:index]
I'm not a huge fan of this spacing, but I don't have a good alternative.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode83
chrome/browser/cocoa/tab_view_picker_table.mm:83:
respondsToSelector:@selector(tabView:didSelectTabViewItem:)])
See below.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode90
chrome/browser/cocoa/tab_view_picker_table.mm:90:
respondsToSelector:@selector(tabView:shouldSelectTabViewItem:)])
I really hate how you broke this line. I'd prefer some extra indentation on the
second line.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode91
chrome/browser/cocoa/tab_view_picker_table.mm:91: return [oldTabViewDelegate_
tabView:tabView
braces

http://codereview.chromium.org/2534001/diff/11001/12004#newcode99
chrome/browser/cocoa/tab_view_picker_table.mm:99:
respondsToSelector:@selector(tabView:willSelectTabViewItem:)])
See above.

http://codereview.chromium.org/2534001/diff/11001/12005
File chrome/browser/cocoa/tab_view_picker_table_unittest.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12005#newcode14
chrome/browser/cocoa/tab_view_picker_table_unittest.mm:14: @interface Ping :
NSObject {
I worry about pollution of the global symbol namespace.

http://codereview.chromium.org/2534001/diff/11001/12007
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/2534001/diff/11001/12007#newcode738
chrome/chrome_tests.gypi:738: 'browser/cocoa/tab_view_picker_table_unittest.mm',
alphabetical order

[Robert Sesek, 2010-06-03 22:33:19]

http://codereview.chromium.org/2534001/diff/11001/12002
File chrome/browser/cocoa/content_settings_dialog_controller.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12002#newcode57
chrome/browser/cocoa/content_settings_dialog_controller.mm:57: // autoupdated in
IB. However, in chrome, the subviews shouldn't have
Chrome

http://codereview.chromium.org/2534001/diff/11001/12003
File chrome/browser/cocoa/tab_view_picker_table.h (right):

http://codereview.chromium.org/2534001/diff/11001/12003#newcode8
chrome/browser/cocoa/tab_view_picker_table.h:8: // NSTabViewItems of an
NSTabView. To use this, just put create a
put|create

http://codereview.chromium.org/2534001/diff/11001/12003#newcode15
chrome/browser/cocoa/tab_view_picker_table.h:15: IBOutlet NSTabView* tabView_; 
// Visible for testing.
I don't think this should be part of the public interface. Create a UnitTesting
category and add an accessor.

http://codereview.chromium.org/2534001/diff/11001/12004
File chrome/browser/cocoa/tab_view_picker_table.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12004#newcode9
chrome/browser/cocoa/tab_view_picker_table.mm:9: @interface NSTabView(debug)
Remove

http://codereview.chromium.org/2534001/diff/11001/12004#newcode13
chrome/browser/cocoa/tab_view_picker_table.mm:13: @interface
NSTabViewItem(debug)
Remove

http://codereview.chromium.org/2534001/diff/11001/12004#newcode20
chrome/browser/cocoa/tab_view_picker_table.mm:20: if((self = [super
initWithFrame:frame])) {
Space after if

http://codereview.chromium.org/2534001/diff/11001/12004#newcode28
chrome/browser/cocoa/tab_view_picker_table.mm:28: if((self = [super
initWithCoder:coder])) {
Space after if

http://codereview.chromium.org/2534001/diff/11001/12004#newcode51
chrome/browser/cocoa/tab_view_picker_table.mm:51: [self  
selectRowIndexes:[NSIndexSet indexSetWithIndex:index]
I'd take colon misalignment over this, I think

http://codereview.chromium.org/2534001/diff/11001/12005
File chrome/browser/cocoa/tab_view_picker_table_unittest.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12005#newcode92
chrome/browser/cocoa/tab_view_picker_table_unittest.mm:92: NSLog(@"%d %@",
[view_ selectedRow], [view_ selectedRowIndexes]);
Remove

[viettrungluu, 2010-06-03 23:08:03]

Man, that vtl guy is a terrible reviewer.

On 2010/06/03 22:33:19, rsesek wrote:
> http://codereview.chromium.org/2534001/diff/11001/12002
> File chrome/browser/cocoa/content_settings_dialog_controller.mm (right):
> 
> http://codereview.chromium.org/2534001/diff/11001/12002#newcode57
> chrome/browser/cocoa/content_settings_dialog_controller.mm:57: // autoupdated
in
> IB. However, in chrome, the subviews shouldn't have
> Chrome
> 
> http://codereview.chromium.org/2534001/diff/11001/12003
> File chrome/browser/cocoa/tab_view_picker_table.h (right):
> 
> http://codereview.chromium.org/2534001/diff/11001/12003#newcode8
> chrome/browser/cocoa/tab_view_picker_table.h:8: // NSTabViewItems of an
> NSTabView. To use this, just put create a
> put|create
> 
> http://codereview.chromium.org/2534001/diff/11001/12003#newcode15
> chrome/browser/cocoa/tab_view_picker_table.h:15: IBOutlet NSTabView* tabView_;

> // Visible for testing.
> I don't think this should be part of the public interface. Create a
UnitTesting
> category and add an accessor.
> 
> http://codereview.chromium.org/2534001/diff/11001/12004
> File chrome/browser/cocoa/tab_view_picker_table.mm (right):
> 
> http://codereview.chromium.org/2534001/diff/11001/12004#newcode9
> chrome/browser/cocoa/tab_view_picker_table.mm:9: @interface NSTabView(debug)
> Remove
> 
> http://codereview.chromium.org/2534001/diff/11001/12004#newcode13
> chrome/browser/cocoa/tab_view_picker_table.mm:13: @interface
> NSTabViewItem(debug)
> Remove
> 
> http://codereview.chromium.org/2534001/diff/11001/12004#newcode20
> chrome/browser/cocoa/tab_view_picker_table.mm:20: if((self = [super
> initWithFrame:frame])) {
> Space after if
> 
> http://codereview.chromium.org/2534001/diff/11001/12004#newcode28
> chrome/browser/cocoa/tab_view_picker_table.mm:28: if((self = [super
> initWithCoder:coder])) {
> Space after if
> 
> http://codereview.chromium.org/2534001/diff/11001/12004#newcode51
> chrome/browser/cocoa/tab_view_picker_table.mm:51: [self  
> selectRowIndexes:[NSIndexSet indexSetWithIndex:index]
> I'd take colon misalignment over this, I think
> 
> http://codereview.chromium.org/2534001/diff/11001/12005
> File chrome/browser/cocoa/tab_view_picker_table_unittest.mm (right):
> 
> http://codereview.chromium.org/2534001/diff/11001/12005#newcode92
> chrome/browser/cocoa/tab_view_picker_table_unittest.mm:92: NSLog(@"%d %@",
> [view_ selectedRow], [view_ selectedRowIndexes]);
> Remove

[Nico, 2010-06-04 17:46:26]

Thanks, Gentlemen.

http://codereview.chromium.org/2534001/diff/11001/12003
File chrome/browser/cocoa/tab_view_picker_table.h (right):

http://codereview.chromium.org/2534001/diff/11001/12003#newcode8
chrome/browser/cocoa/tab_view_picker_table.h:8: // NSTabViewItems of an
NSTabView. To use this, just put create a
On 2010/06/03 22:33:20, rsesek wrote:
> put|create

Done.

http://codereview.chromium.org/2534001/diff/11001/12003#newcode9
chrome/browser/cocoa/tab_view_picker_table.h:9: // TabViewPickerTable in
interface builder and connect the |tabView_| outlet
On 2010/06/03 22:26:41, vtl wrote:
> Interface Builder (with caps)

dnoe

http://codereview.chromium.org/2534001/diff/11001/12003#newcode15
chrome/browser/cocoa/tab_view_picker_table.h:15: IBOutlet NSTabView* tabView_; 
// Visible for testing.
On 2010/06/03 22:33:20, rsesek wrote:
> I don't think this should be part of the public interface. Create a
UnitTesting
> category and add an accessor.

But I need to set it, not access it. And the setter needs to be called before
awakeFromNib is called. Providing a setter function seems worse, given these
circumstances

http://codereview.chromium.org/2534001/diff/11001/12003#newcode20
chrome/browser/cocoa/tab_view_picker_table.h:20: 
On 2010/06/03 22:26:41, vtl wrote:
> Clearly, this blank line was put here just to anger me.

Clearly. Did it work?

http://codereview.chromium.org/2534001/diff/11001/12004
File chrome/browser/cocoa/tab_view_picker_table.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12004#newcode9
chrome/browser/cocoa/tab_view_picker_table.mm:9: @interface NSTabView(debug)
On 2010/06/03 22:33:20, rsesek wrote:
> Remove

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode13
chrome/browser/cocoa/tab_view_picker_table.mm:13: @interface
NSTabViewItem(debug)
On 2010/06/03 22:33:20, rsesek wrote:
> Remove

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode20
chrome/browser/cocoa/tab_view_picker_table.mm:20: if((self = [super
initWithFrame:frame])) {
On 2010/06/03 22:33:20, rsesek wrote:
> Space after if

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode28
chrome/browser/cocoa/tab_view_picker_table.mm:28: if((self = [super
initWithCoder:coder])) {
On 2010/06/03 22:33:20, rsesek wrote:
> Space after if

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode51
chrome/browser/cocoa/tab_view_picker_table.mm:51: [self  
selectRowIndexes:[NSIndexSet indexSetWithIndex:index]
On 2010/06/03 22:33:20, rsesek wrote:
> I'd take colon misalignment over this, I think

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode68
chrome/browser/cocoa/tab_view_picker_table.mm:68: -
(id)tableView:(NSTableView*)tableView
On 2010/06/03 22:26:41, vtl wrote:
> I'm of the opinion that you should insert spaces between "(id)" and
> "tableView:..." so that things line up. This is permitted by the style guide.

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode78
chrome/browser/cocoa/tab_view_picker_table.mm:78: NSInteger index =
On 2010/06/03 22:26:41, vtl wrote:
> This linebreak is clearly unneeded.

The line's at 81 chars if I remove it.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode80
chrome/browser/cocoa/tab_view_picker_table.mm:80: [self  
selectRowIndexes:[NSIndexSet indexSetWithIndex:index]
On 2010/06/03 22:26:41, vtl wrote:
> I'm not a huge fan of this spacing, but I don't have a good alternative.

Me neither. Applied rsesek's suggesion, which I'm not a huge fan of either, but
that way at least he's happy.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode83
chrome/browser/cocoa/tab_view_picker_table.mm:83:
respondsToSelector:@selector(tabView:didSelectTabViewItem:)])
On 2010/06/03 22:26:41, vtl wrote:
> See below.

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode90
chrome/browser/cocoa/tab_view_picker_table.mm:90:
respondsToSelector:@selector(tabView:shouldSelectTabViewItem:)])
On 2010/06/03 22:26:41, vtl wrote:
> I really hate how you broke this line. I'd prefer some extra indentation on
the
> second line.

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode91
chrome/browser/cocoa/tab_view_picker_table.mm:91: return [oldTabViewDelegate_
tabView:tabView
On 2010/06/03 22:26:41, vtl wrote:
> braces

Done.

http://codereview.chromium.org/2534001/diff/11001/12004#newcode99
chrome/browser/cocoa/tab_view_picker_table.mm:99:
respondsToSelector:@selector(tabView:willSelectTabViewItem:)])
On 2010/06/03 22:26:41, vtl wrote:
> See above.

Done.

http://codereview.chromium.org/2534001/diff/11001/12005
File chrome/browser/cocoa/tab_view_picker_table_unittest.mm (right):

http://codereview.chromium.org/2534001/diff/11001/12005#newcode14
chrome/browser/cocoa/tab_view_picker_table_unittest.mm:14: @interface Ping :
NSObject {
On 2010/06/03 22:26:41, vtl wrote:
> I worry about pollution of the global symbol namespace.

Renamed the class to something less polluting. Sadly, objc classes can't be put
into namespaces.

http://codereview.chromium.org/2534001/diff/11001/12005#newcode92
chrome/browser/cocoa/tab_view_picker_table_unittest.mm:92: NSLog(@"%d %@",
[view_ selectedRow], [view_ selectedRowIndexes]);
On 2010/06/03 22:33:20, rsesek wrote:
> Remove

Done.

[viettrungluu, 2010-06-04 18:16:30]

On 2010/06/04 17:46:26, Nico wrote:
> Thanks, Gentlemen.

s/men/man/. (I assume you're referring to rsesek, since you're clearly not
talking about me.)

> On 2010/06/03 22:26:41, vtl wrote:
> > Clearly, this blank line was put here just to anger me.
> 
> Clearly. Did it work?

I was enraged in fact. But I'll take it out on you when you least expect it.

> On 2010/06/03 22:26:41, vtl wrote:
> > This linebreak is clearly unneeded.
> 
> The line's at 81 chars if I remove it.

My monitor must be warped.

> On 2010/06/03 22:26:41, vtl wrote:
> > I'm not a huge fan of this spacing, but I don't have a good alternative.
> 
> Me neither. Applied rsesek's suggesion, which I'm not a huge fan of either,
but
> that way at least he's happy.

He's so easy to please.

LGTM.

[Robert Sesek, 2010-06-04 18:34:49]

LGTM

http://codereview.chromium.org/2534001/diff/11001/12003
File chrome/browser/cocoa/tab_view_picker_table.h (right):

http://codereview.chromium.org/2534001/diff/11001/12003#newcode15
chrome/browser/cocoa/tab_view_picker_table.h:15: IBOutlet NSTabView* tabView_; 
// Visible for testing.
On 2010/06/04 17:46:26, Nico wrote:
> On 2010/06/03 22:33:20, rsesek wrote:
> > I don't think this should be part of the public interface. Create a
> UnitTesting
> > category and add an accessor.
> 
> But I need to set it, not access it. And the setter needs to be called before
> awakeFromNib is called. Providing a setter function seems worse, given these
> circumstances

Fair enough.