Add a bare-bones extension shelf that displays extension items on OS X.

Add a bare-bones extension shelf that displays extension items on OS X.

This brings our extension support to about the level it has on linux.

One issue is that the toolstrips are webpages with a background image that just happens to look like the shelf they are on. But the background images are not updated on key->nonkey window changes, so the toolstrip backgrounds look slightly off in one of the two cases. If we decide to keep the shelf, we should fix this, but see the bug for erikkay's stance on this.

Also, the NTP is only loaded after all toolstrips have been loaded for some reason. That's what happens on the other platforms too, I believe.

The extension shelf uses the DownloadShelfView as background view for now.

Screenie: http://imgur.com/wSHgU.png

BUG=19073
TEST=Extensions that live in the shelf should show up. They should be clickable, resize correctly (e.g. the build status extension), and the shelf should interact in a sane way with the status bubble.

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

[Nico, 2009-09-04 02:14:40]

This is based on Pawel's work to make this work on Linux. Thanks, Pawel!

Erik, you're here for the extensions part.

Pink, you're here mainly for the "should we land this at all?" part. Extensions
are enabled by default these days – do we want something half-baked enabled by
default? On the other hand, it's enabled & half-baked on the other platforms
too. You can also look at the mac parts, or suggest someone else to look at it.

Pawel, Pam: Just FYI.

[pink (ping after 24hrs), 2009-09-04 17:08:06]

http://codereview.chromium.org/175025/diff/15015/15017
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/175025/diff/15015/15017#newcode74
Line 74: CGFloat verticalOffsetForStatusBubble;
member vars end in an underscore

http://codereview.chromium.org/175025/diff/15015/15018
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/175025/diff/15015/15018#newcode197
Line 197: resizeDelegate:self]);
line up colons

http://codereview.chromium.org/175025/diff/15015/15019
File chrome/browser/cocoa/extension_shelf_controller.h (right):

http://codereview.chromium.org/175025/diff/15015/15019#newcode13
Line 13: @interface ExtensionShelfController : NSViewController {
needs class-level comment explaining purpose, use, assumptions, etc.

http://codereview.chromium.org/175025/diff/15015/15019#newcode14
Line 14: 
@private

http://codereview.chromium.org/175025/diff/15015/15019#newcode27
Line 27: - (id)initWithBrowser:(Browser*)browser
public API calls need a comment.

http://codereview.chromium.org/175025/diff/15015/15019#newcode35
Line 35: - (void)wasInsertedIntoWindow;
the NSViewController doesn't get told at all when its view goes into a window?
That sucks.

http://codereview.chromium.org/175025/diff/15015/15020
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/15015/15020#newcode77
Line 77: class ExtensionShelfMac::Toolstrip {
needs a class-level comment describing its purpose, how it's to be used, etc etc

http://codereview.chromium.org/175025/diff/15015/15020#newcode85
Line 85: void AddToolstripToController(ExtensionShelfController* controller);
public API methods need a comment.

http://codereview.chromium.org/175025/diff/15015/15020#newcode109
Line 109: NSView* toolstripView = host_->view()->native_view();
while in C++ method code, use C++ naming. eg |toolstrip_view|

http://codereview.chromium.org/175025/diff/15015/15020#newcode113
Line 113: addObserver:controller
indent 4 additional spaces on continuation

http://codereview.chromium.org/175025/diff/15015/15020#newcode126
Line 126: object:host_->view()->native_view()];
are you guaranteed that this will be the same as |toolstripView| above? There's
nothing the extension can do to swap it out?

http://codereview.chromium.org/175025/diff/15015/15020#newcode230
Line 230: 100, [controller_ height] - kExtensionShelfPaddingTop);
100?

http://codereview.chromium.org/175025/diff/15015/15020#newcode300
Line 300: doneInitializing_ = FALSE;
Explain why this is necessary. Add a comment? Also, this should be |NO|, not
|FALSE|.

http://codereview.chromium.org/175025/diff/15015/15020#newcode309
Line 309: doneInitializing_ = TRUE;
= YES

http://codereview.chromium.org/175025/diff/15015/15021
File chrome/browser/cocoa/extension_view_mac.h (right):

http://codereview.chromium.org/175025/diff/15015/15021#newcode5
Line 5: #ifndef CHROME_BROWSER_MAC_EXTENSION_VIEW_MAC_H_
parallels the directory structure. should be
CHROME_BROWSER_COCOA_EXTENSION_VIEW_MAC_H_

http://codereview.chromium.org/175025/diff/15015/15021#newcode17
Line 17: class ExtensionViewMac {
class-level comment

http://codereview.chromium.org/175025/diff/15015/15022
File chrome/browser/cocoa/extension_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/15015/15022#newcode29
Line 29: DCHECK(render_widget_host_view_);
you're inconsistent here in how you check or don't check for rwhv being NULL.
Since it has that default value, you should plan for it being NULL. You do in
the dtor, but this will crash if called from other locations.

[Nico, 2009-09-05 01:34:34]

Thanks for the review.

http://codereview.chromium.org/175025/diff/15015/15017
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/175025/diff/15015/15017#newcode74
Line 74: CGFloat verticalOffsetForStatusBubble;
On 2009/09/04 17:08:07, pink wrote:
> member vars end in an underscore

Done.

http://codereview.chromium.org/175025/diff/15015/15018
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/175025/diff/15015/15018#newcode197
Line 197: resizeDelegate:self]);
On 2009/09/04 17:08:07, pink wrote:
> line up colons

Done.

http://codereview.chromium.org/175025/diff/15015/15019
File chrome/browser/cocoa/extension_shelf_controller.h (right):

http://codereview.chromium.org/175025/diff/15015/15019#newcode13
Line 13: @interface ExtensionShelfController : NSViewController {
On 2009/09/04 17:08:07, pink wrote:
> needs class-level comment explaining purpose, use, assumptions, etc.

Done.

http://codereview.chromium.org/175025/diff/15015/15019#newcode14
Line 14: 
On 2009/09/04 17:08:07, pink wrote:
> @private

Done.

http://codereview.chromium.org/175025/diff/15015/15019#newcode27
Line 27: - (id)initWithBrowser:(Browser*)browser
On 2009/09/04 17:08:07, pink wrote:
> public API calls need a comment.

Done.

http://codereview.chromium.org/175025/diff/15015/15019#newcode35
Line 35: - (void)wasInsertedIntoWindow;
On 2009/09/04 17:08:07, pink wrote:
> the NSViewController doesn't get told at all when its view goes into a window?
> That sucks.

I looked a bit for a notification that I could use, but couldn't find one.

http://codereview.chromium.org/175025/diff/15015/15020
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/15015/15020#newcode85
Line 85: void AddToolstripToController(ExtensionShelfController* controller);
On 2009/09/04 17:08:07, pink wrote:
> public API methods need a comment.

Done.

http://codereview.chromium.org/175025/diff/15015/15020#newcode109
Line 109: NSView* toolstripView = host_->view()->native_view();
On 2009/09/04 17:08:07, pink wrote:
> while in C++ method code, use C++ naming. eg |toolstrip_view|

Done. I hope I didn't miss anything.

http://codereview.chromium.org/175025/diff/15015/15020#newcode113
Line 113: addObserver:controller
On 2009/09/04 17:08:07, pink wrote:
> indent 4 additional spaces on continuation

4 _additional_ spaces? I indented 2 additional, for a total of 4.

By the way, several files indent only two spaces in this case, which is why I
did it.

http://codereview.chromium.org/175025/diff/15015/15020#newcode126
Line 126: object:host_->view()->native_view()];
On 2009/09/04 17:08:07, pink wrote:
> are you guaranteed that this will be the same as |toolstripView| above?
There's
> nothing the extension can do to swap it out?

The extension is just a RenderWidgetHostViewMac's native_view(), whose view does
not change.

http://codereview.chromium.org/175025/diff/15015/15020#newcode230
Line 230: 100, [controller_ height] - kExtensionShelfPaddingTop);
On 2009/09/04 17:08:07, pink wrote:
> 100?

More or less arbitrary, the extension's background is tiled with this. Too small
and a lot of cpu is spent tiling the image, too large and it needs lots of
memory.

Hm, I guess I should add that to the comment.

http://codereview.chromium.org/175025/diff/15015/15020#newcode300
Line 300: doneInitializing_ = FALSE;
On 2009/09/04 17:08:07, pink wrote:
> Explain why this is necessary. Add a comment? Also, this should be |NO|, not
> |FALSE|.

Actually, it shouldn't be necessary anymore. I created the ExtensionShelfMac()
here earlier, which ended up calling show:, which called something in the
browser controller, which in turn was then confused because it thought the
extension shelf is not yet done initializing.

But since the bridge is now built later, I removed this. Thanks.

http://codereview.chromium.org/175025/diff/15015/15020#newcode309
Line 309: doneInitializing_ = TRUE;
On 2009/09/04 17:08:07, pink wrote:
> = YES

Obsolete.

http://codereview.chromium.org/175025/diff/15015/15021
File chrome/browser/cocoa/extension_view_mac.h (right):

http://codereview.chromium.org/175025/diff/15015/15021#newcode5
Line 5: #ifndef CHROME_BROWSER_MAC_EXTENSION_VIEW_MAC_H_
On 2009/09/04 17:08:07, pink wrote:
> parallels the directory structure. should be
> CHROME_BROWSER_COCOA_EXTENSION_VIEW_MAC_H_

Done.

http://codereview.chromium.org/175025/diff/15015/15021#newcode17
Line 17: class ExtensionViewMac {
On 2009/09/04 17:08:07, pink wrote:
> class-level comment

Done.

http://codereview.chromium.org/175025/diff/15015/15022
File chrome/browser/cocoa/extension_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/15015/15022#newcode29
Line 29: DCHECK(render_widget_host_view_);
Added a comment in the header file that says that other methods must only be
called after Init().

[pink (ping after 24hrs), 2009-09-08 18:46:17]

http://codereview.chromium.org/175025/diff/14020/15047
File chrome/browser/cocoa/extension_shelf_controller.h (right):

http://codereview.chromium.org/175025/diff/14020/15047#newcode14
Line 14: // insert it's |view| into a superview and call
|wasInsertedIntoWindow|. After
s/it's/its

http://codereview.chromium.org/175025/diff/14020/15047#newcode17
Line 17: @interface ExtensionShelfController : NSViewController {
where's the unit test for this class?

http://codereview.chromium.org/175025/diff/14020/15048
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/14020/15048#newcode22
Line 22: class ExtensionShelfMac : public ExtensionShelfModelObserver {
it would be nice if this could be moved into a separate file so it could be
independently unit tested. There's a lot of machinery here.

http://codereview.chromium.org/175025/diff/14020/15048#newcode246
Line 246: 100, [controller_ height] - kExtensionShelfPaddingTop);
make the 100 an actual declared constant so it's clear that's what you're
discussing in the above comment. Right now there's no correlation between the
value and the comment.

http://codereview.chromium.org/175025/diff/14020/15048#newcode329
Line 329: // the view is in a superview to work. Hence, create the bridge object
not
s/not/no? 

In addition, you should mention this in the class comment for ExtensionShelfMac.
It's pretty important.

http://codereview.chromium.org/175025/diff/14020/15049
File chrome/browser/cocoa/extension_view_mac.h (right):

http://codereview.chromium.org/175025/diff/14020/15049#newcode20
Line 20: class ExtensionViewMac {
where's the unit test for this?

http://codereview.chromium.org/175025/diff/14020/15049#newcode27
Line 27: void Init();
is there a reason why you can't just do this work in the ctor? You're not
returning a failure result and it makes it impossible to forget to call it.

http://codereview.chromium.org/175025/diff/14020/15050
File chrome/browser/cocoa/extension_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/14020/15050#newcode34
Line 34: return extension_host_->render_view_host();
will extension_host always be non-null? Should you DCHECK in the ctor?

http://codereview.chromium.org/175025/diff/14020/15053
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/175025/diff/14020/15053#newcode69
Line 69: view_.reset(new ExtensionViewMac(this, browser));
is there no common API these can inherit from? It seems a little silly to have
to re-declare 3 similar objects.

http://codereview.chromium.org/175025/diff/14020/15056
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/14020/15056#newcode540
Line 540: if (renderWidgetHostView_->render_widget_host_) {
why bother adding these if there are no other code changes?

[Erik does not do reviews, 2009-09-09 00:04:04]

http://codereview.chromium.org/175025/diff/14020/15046
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/175025/diff/14020/15046#newcode481
Line 481: // |downloadShelfController_| may be nil.
extensionShelfController_ may also be nil

http://codereview.chromium.org/175025/diff/14020/15047
File chrome/browser/cocoa/extension_shelf_controller.h (right):

http://codereview.chromium.org/175025/diff/14020/15047#newcode21
Line 21: scoped_ptr<ExtensionShelfMac> bridge_;
just curious, why is it called "bridge_"?

http://codereview.chromium.org/175025/diff/14020/15048
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/14020/15048#newcode16
Line 16: 
I believe style is to remove the surrounding newlines here.

http://codereview.chromium.org/175025/diff/14020/15048#newcode330
Line 330: // sooner.
DCHECK(brige_.get() == NULL)

http://codereview.chromium.org/175025/diff/14020/15050
File chrome/browser/cocoa/extension_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/14020/15050#newcode42
Line 42: void ExtensionViewMac::UpdatePreferredWidth(int pref_width) {
If you look at the Windows implementation, we went through a lot of gyrations to
avoid drawing and updating the preferred width at all until we were sure that
the initial load of the toolstrip finished.  If you don't do that, you'll get a
lot of odd flickering and flashing on startup (in part because the CSS hasn't
been loaded yet).  If you're not going to do this work for the initial checkin,
you should definitely add a TODO to deal with it.

http://codereview.chromium.org/175025/diff/14020/15053
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/175025/diff/14020/15053#newcode69
Line 69: view_.reset(new ExtensionViewMac(this, browser));
agree with pink that we should add a common superclass/interface at this point.

[Nico, 2009-09-13 07:09:59]

I added a basic test for the controller. Other than that, I fear my comments
consist mostly of me saying "not now".

http://codereview.chromium.org/175025/diff/14020/15047
File chrome/browser/cocoa/extension_shelf_controller.h (right):

http://codereview.chromium.org/175025/diff/14020/15047#newcode14
Line 14: // insert it's |view| into a superview and call
|wasInsertedIntoWindow|. After
On 2009/09/08 18:46:17, pink wrote:
> s/it's/its

Done.

http://codereview.chromium.org/175025/diff/14020/15047#newcode17
Line 17: @interface ExtensionShelfController : NSViewController {
On 2009/09/08 18:46:17, pink wrote:
> where's the unit test for this class?

Why, in extension_shelf_controller_unittest.mm of course? ;-)

http://codereview.chromium.org/175025/diff/14020/15047#newcode21
Line 21: scoped_ptr<ExtensionShelfMac> bridge_;
On 2009/09/09 00:04:04, Erik Kay wrote:
> just curious, why is it called "bridge_"?

Because it bridges the chromium c++ world to the objc world – this name is used
in several of the mac ui classes.

http://codereview.chromium.org/175025/diff/14020/15048
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/14020/15048#newcode16
Line 16: 
On 2009/09/09 00:04:04, Erik Kay wrote:
> I believe style is to remove the surrounding newlines here.

The styleguide has those newlines in its code examples (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces ).
Also, a quick `ack -C 4 'namespace {' chrome/browser/cocoa/` shows that it's
also what is consistent with what is there.

http://codereview.chromium.org/175025/diff/14020/15048#newcode22
Line 22: class ExtensionShelfMac : public ExtensionShelfModelObserver {
On 2009/09/08 18:46:17, pink wrote:
> it would be nice if this could be moved into a separate file so it could be
> independently unit tested. There's a lot of machinery here.

Yes. I'm confident that some of the machinery can be shared across platforms
once it is clear how extensions are supposed to look. Until then, I think this
should stay similar to how the linux code looks, so when the extension guys
unify this code, it's easier for them.

(also see my reasoning later on)

http://codereview.chromium.org/175025/diff/14020/15048#newcode246
Line 246: 100, [controller_ height] - kExtensionShelfPaddingTop);
On 2009/09/08 18:46:17, pink wrote:
> make the 100 an actual declared constant so it's clear that's what you're
> discussing in the above comment. Right now there's no correlation between the
> value and the comment.

Done. Made it a local constant though, as it's used only here.

http://codereview.chromium.org/175025/diff/14020/15048#newcode329
Line 329: // the view is in a superview to work. Hence, create the bridge object
not
On 2009/09/08 18:46:17, pink wrote:
> s/not/no? 
> 
> In addition, you should mention this in the class comment for
ExtensionShelfMac.
> It's pretty important.

Done.

http://codereview.chromium.org/175025/diff/14020/15048#newcode330
Line 330: // sooner.
On 2009/09/09 00:04:04, Erik Kay wrote:
> DCHECK(brige_.get() == NULL)

Good point! Done.

http://codereview.chromium.org/175025/diff/14020/15049
File chrome/browser/cocoa/extension_view_mac.h (right):

http://codereview.chromium.org/175025/diff/14020/15049#newcode20
Line 20: class ExtensionViewMac {
On 2009/09/08 18:46:17, pink wrote:
> where's the unit test for this?

Not here. There's no good way to create an ExtensionHost for testing (the little
test code that is there for testing is windows-specific, and pretty
heavy-weight). Writing a test for this class would require lots of scaffolding,
and this scaffolding is probably better written by someone who has a better
grasp on the whole extension design.

http://codereview.chromium.org/175025/diff/14020/15049#newcode27
Line 27: void Init();
On 2009/09/08 18:46:17, pink wrote:
> is there a reason why you can't just do this work in the ctor? You're not
> returning a failure result and it makes it impossible to forget to call it.

It's here because of http://codereview.chromium.org/173506 – the linux guys did
it directly in the constructor but it looks like there was a problem with that?
Erik actually asked the same question on that review, but Pawel didn't answer. I
will remove that Init() method on both os x and linux in my "extract common
extension view interface" follow-up cl that i promised above (or will promise
further down).

http://codereview.chromium.org/175025/diff/14020/15050
File chrome/browser/cocoa/extension_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/14020/15050#newcode34
Line 34: return extension_host_->render_view_host();
On 2009/09/08 18:46:17, pink wrote:
> will extension_host always be non-null? Should you DCHECK in the ctor?

It is my understanding that it's always non-null. Added a DCHECK.

http://codereview.chromium.org/175025/diff/14020/15050#newcode42
Line 42: void ExtensionViewMac::UpdatePreferredWidth(int pref_width) {
I'm doing what gtk is doing here. Didn't see a lot of flickering either. Added a
todo anyway. (if it's bad to call this while the extension view is not yet
visible, why does the caller not call it only after the view is visible?)

http://codereview.chromium.org/175025/diff/14020/15053
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/175025/diff/14020/15053#newcode69
Line 69: view_.reset(new ExtensionViewMac(this, browser));
My thinking was that a common api would make sense, but it would also slow down
the speed the extension team can iterate on this. As far as I understand,
they're not sure how all this will look in the end, and making them change an
interface and 3 platforms sounds like it'd slow down experimentation. (right now
there are a few things

And when they're done experimenting, I'd rather have the extension team define
an interface instead of someone who is not very familiar with this code.

Since Erik agrees, these assumptions were wrong :-), so I can extract an
interface. But I'd prefer to do it in a separate CL, as this one is already
pretty big (I will extract the interface within a week of landing this CL.

http://codereview.chromium.org/175025/diff/14020/15056
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/175025/diff/14020/15056#newcode540
Line 540: if (renderWidgetHostView_->render_widget_host_) {
On 2009/09/08 18:46:17, pink wrote:
> why bother adding these if there are no other code changes?

Relicts from earlier changes I made in this file. Removed.

[Nico, 2009-09-15 18:12:46]

Ping.

[Erik does not do reviews, 2009-09-15 18:21:54]

extension code LGTM

http://codereview.chromium.org/175025/diff/14020/15053
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/175025/diff/14020/15053#newcode69
Line 69: view_.reset(new ExtensionViewMac(this, browser));
On 2009/09/13 07:09:59, Nico wrote:
> My thinking was that a common api would make sense, but it would also slow
down
> the speed the extension team can iterate on this. As far as I understand,
> they're not sure how all this will look in the end, and making them change an
> interface and 3 platforms sounds like it'd slow down experimentation. (right
now
> there are a few things
> 
> And when they're done experimenting, I'd rather have the extension team define
> an interface instead of someone who is not very familiar with this code.
> 
> Since Erik agrees, these assumptions were wrong :-), so I can extract an
> interface. But I'd prefer to do it in a separate CL, as this one is already
> pretty big (I will extract the interface within a week of landing this CL.

Why don't you add a TODO with a bug and assign it to me.

[pink (ping after 24hrs), 2009-09-15 19:03:43]

LGTM with nits.

http://codereview.chromium.org/175025/diff/15063/19005
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/15063/19005#newcode63
Line 63: Browser* browser_;
comment on other weak references here and below

http://codereview.chromium.org/175025/diff/15063/19005#newcode112
Line 112: ExtensionHost* host_;
weak reference should be commented.

http://codereview.chromium.org/175025/diff/15063/19005#newcode275
Line 275: CGFloat x = 0;
comment about how this lays out the tab strips horizontally and how the height
gets adjusted. I initially assumed they'd be vertical since we're adjusting the
height, but clearly that's not the case.

http://codereview.chromium.org/175025/diff/15063/19006
File chrome/browser/cocoa/extension_shelf_controller_unittest.mm (right):

http://codereview.chromium.org/175025/diff/15063/19006#newcode28
Line 28: scoped_nsobject<ExtensionShelfController> controller_;
this should go after the browser helper so it doesn't outlive it.

http://codereview.chromium.org/175025/diff/15063/19007
File chrome/browser/cocoa/extension_view_mac.h (right):

http://codereview.chromium.org/175025/diff/15063/19007#newcode54
Line 54: Browser* browser_;
assuming these are weak, they should be commented as such.

[Nico, 2009-09-15 23:15:22]

Thanks. Will land this today.

http://codereview.chromium.org/175025/diff/14020/15053
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/175025/diff/14020/15053#newcode69
Line 69: view_.reset(new ExtensionViewMac(this, browser));
On 2009/09/15 18:21:54, Erik Kay wrote:
> On 2009/09/13 07:09:59, Nico wrote:
> > My thinking was that a common api would make sense, but it would also slow
> down
> > the speed the extension team can iterate on this. As far as I understand,
> > they're not sure how all this will look in the end, and making them change
an
> > interface and 3 platforms sounds like it'd slow down experimentation. (right
> now
> > there are a few things
> > 
> > And when they're done experimenting, I'd rather have the extension team
define
> > an interface instead of someone who is not very familiar with this code.
> > 
> > Since Erik agrees, these assumptions were wrong :-), so I can extract an
> > interface. But I'd prefer to do it in a separate CL, as this one is already
> > pretty big (I will extract the interface within a week of landing this CL.
> 
> Why don't you add a TODO with a bug and assign it to me.
> 

Done.

http://codereview.chromium.org/175025/diff/15063/19005
File chrome/browser/cocoa/extension_shelf_controller.mm (right):

http://codereview.chromium.org/175025/diff/15063/19005#newcode63
Line 63: Browser* browser_;
On 2009/09/15 19:03:43, pink wrote:
> comment on other weak references here and below

Done.

http://codereview.chromium.org/175025/diff/15063/19005#newcode112
Line 112: ExtensionHost* host_;
On 2009/09/15 19:03:43, pink wrote:
> weak reference should be commented.

Done.

http://codereview.chromium.org/175025/diff/15063/19005#newcode275
Line 275: CGFloat x = 0;
On 2009/09/15 19:03:43, pink wrote:
> comment about how this lays out the tab strips horizontally and how the height
> gets adjusted. I initially assumed they'd be vertical since we're adjusting
the
> height, but clearly that's not the case.

Done.

http://codereview.chromium.org/175025/diff/15063/19006
File chrome/browser/cocoa/extension_shelf_controller_unittest.mm (right):

http://codereview.chromium.org/175025/diff/15063/19006#newcode28
Line 28: scoped_nsobject<ExtensionShelfController> controller_;
On 2009/09/15 19:03:43, pink wrote:
> this should go after the browser helper so it doesn't outlive it.

Oh, good point. Done.

http://codereview.chromium.org/175025/diff/15063/19007
File chrome/browser/cocoa/extension_view_mac.h (right):

http://codereview.chromium.org/175025/diff/15063/19007#newcode54
Line 54: Browser* browser_;
On 2009/09/15 19:03:43, pink wrote:
> assuming these are weak, they should be commented as such.

Done.