Adds popups to browser actions, completing the feature....

Adds popups to browser actions, completing the feature.
Adds functionality to choose between two different InfoBubbleView types: white background and gradient background.

Screenshot: http://andybons.com/chrome/news_popup.png

BUG=23881
TEST=Install a browser action extension that has a popup, click and observe the popup being shown. Initial unit test added, but disabled.

Committed: r32589

[Bons, 2009-11-19 02:15:08]

http://codereview.chromium.org/402077/diff/1/10
File chrome/browser/cocoa/extensions/extension_popup_controller.h (right):

http://codereview.chromium.org/402077/diff/1/10#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
add an #ifdef guard

[pink (ping after 24hrs), 2009-11-19 03:57:49]

a good start.

http://codereview.chromium.org/402077/diff/1/6
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/402077/diff/1/6#newcode425
Line 425: [ExtensionPopupController showURL:action->popup_url()
see other comments, but this call should be changed to autorelease, and thus
this will need to retain.

http://codereview.chromium.org/402077/diff/1/10
File chrome/browser/cocoa/extensions/extension_popup_controller.h (right):

http://codereview.chromium.org/402077/diff/1/10#newcode16
Line 16: @interface ExtensionPopupController : NSWindowController {
class needs a class-level comment explaining what it's used for and how it's
used.

http://codereview.chromium.org/402077/diff/1/10#newcode16
Line 16: @interface ExtensionPopupController : NSWindowController {
this class needs a unit test, even if it's just a simple one.

http://codereview.chromium.org/402077/diff/1/10#newcode18
Line 18: NSView* extensionView_;
weak ref? if so, document it. if not, why not a scoped_nsobject?

http://codereview.chromium.org/402077/diff/1/10#newcode31
Line 31: - (ExtensionHost*)host;
all public methods need a comment.

http://codereview.chromium.org/402077/diff/1/10#newcode33
Line 33: - (id)initWithHost:(ExtensionHost*)host
is parentWindow optional? what coordinate system should anchoredAt be in? Can
host be null?

http://codereview.chromium.org/402077/diff/1/10#newcode38
Line 38: + (ExtensionPopupController*)showURL:(GURL)url
similar questions to above.

http://codereview.chromium.org/402077/diff/1/11
File chrome/browser/cocoa/extensions/extension_popup_controller.mm (right):

http://codereview.chromium.org/402077/diff/1/11#newcode62
Line 62: initWithContentRect:rect
is this the same rect that's declared above, and uninitialized?

http://codereview.chromium.org/402077/diff/1/11#newcode99
Line 99: DCHECK(manager);
i'd add browser to this dcheck and if() since you deref it below blindly.

http://codereview.chromium.org/402077/diff/1/11#newcode111
Line 111: return popup;
this should be autoreleased to match the cocoa naming rules. It doesn't say
"create" or "copy" in the name so it must return an autoreleased object.

http://codereview.chromium.org/402077/diff/1/11#newcode160
Line 160: - (void)dealloc {
move this up right next to init so they're together

[Aaron Boodman, 2009-11-19 06:09:49]

Defer to pink on the code. Screen cap lgtm.

http://codereview.chromium.org/402077/diff/1/2
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/402077/diff/1/2#newcode558
Line 558: type == ViewType::EXTENSION_POPUP);
Why weren't we hitting this before?

[Erik does not do reviews, 2009-11-19 20:27:34]

overall looks good.  I'll defer to pink on the cocoa code.

[Bons, 2009-11-20 00:40:38]

http://codereview.chromium.org/402077/diff/1/6
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/402077/diff/1/6#newcode425
Line 425: [ExtensionPopupController showURL:action->popup_url()
On 2009/11/19 03:57:49, pink wrote:
> see other comments, but this call should be changed to autorelease, and thus
> this will need to retain.

Done.

http://codereview.chromium.org/402077/diff/1/10
File chrome/browser/cocoa/extensions/extension_popup_controller.h (right):

http://codereview.chromium.org/402077/diff/1/10#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/11/19 02:15:08, Andrew Bonventre (chromium) wrote:
> add an #ifdef guard

Done.

http://codereview.chromium.org/402077/diff/1/10#newcode16
Line 16: @interface ExtensionPopupController : NSWindowController {
On 2009/11/19 03:57:49, pink wrote:
> class needs a class-level comment explaining what it's used for and how it's
> used.

Done.

http://codereview.chromium.org/402077/diff/1/10#newcode16
Line 16: @interface ExtensionPopupController : NSWindowController {
On 2009/11/19 03:57:49, pink wrote:
> this class needs a unit test, even if it's just a simple one.

Done, but half-assed. I will be filing a bug about making the requirements for
this type of unittest more easily mocked once this is committed.

http://codereview.chromium.org/402077/diff/1/10#newcode18
Line 18: NSView* extensionView_;
On 2009/11/19 03:57:49, pink wrote:
> weak ref? if so, document it. if not, why not a scoped_nsobject?

Done.

http://codereview.chromium.org/402077/diff/1/10#newcode31
Line 31: - (ExtensionHost*)host;
On 2009/11/19 03:57:49, pink wrote:
> all public methods need a comment.

Done.

http://codereview.chromium.org/402077/diff/1/10#newcode33
Line 33: - (id)initWithHost:(ExtensionHost*)host
On 2009/11/19 03:57:49, pink wrote:
> is parentWindow optional? what coordinate system should anchoredAt be in? Can
> host be null?

Made private since it should never be called externally.

http://codereview.chromium.org/402077/diff/1/10#newcode38
Line 38: + (ExtensionPopupController*)showURL:(GURL)url
On 2009/11/19 03:57:49, pink wrote:
> similar questions to above.

Done.

http://codereview.chromium.org/402077/diff/1/11
File chrome/browser/cocoa/extensions/extension_popup_controller.mm (right):

http://codereview.chromium.org/402077/diff/1/11#newcode62
Line 62: initWithContentRect:rect
On 2009/11/19 03:57:49, pink wrote:
> is this the same rect that's declared above, and uninitialized?

Done.

http://codereview.chromium.org/402077/diff/1/11#newcode99
Line 99: DCHECK(manager);
On 2009/11/19 03:57:49, pink wrote:
> i'd add browser to this dcheck and if() since you deref it below blindly.

Done.

http://codereview.chromium.org/402077/diff/1/11#newcode111
Line 111: return popup;
On 2009/11/19 03:57:49, pink wrote:
> this should be autoreleased to match the cocoa naming rules. It doesn't say
> "create" or "copy" in the name so it must return an autoreleased object.

Done.

http://codereview.chromium.org/402077/diff/1/11#newcode160
Line 160: - (void)dealloc {
On 2009/11/19 03:57:49, pink wrote:
> move this up right next to init so they're together

Done.

[pink (ping after 24hrs), 2009-11-20 01:09:41]

sorry for not noticing the window controller ownership the first time.

http://codereview.chromium.org/402077/diff/5001/5005
File chrome/browser/cocoa/extensions/browser_actions_controller.h (right):

http://codereview.chromium.org/402077/diff/5001/5005#newcode48
Line 48: scoped_nsobject<ExtensionPopupController> popupController_;
NSWindowController shouldn't be put into a scoped_nsobject. Just mark this as a
weak reference as the window will clean itself up when closed.

http://codereview.chromium.org/402077/diff/5001/5006
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/402077/diff/5001/5006#newcode415
Line 415: [sender extension]->id())];
is it every possible for this to be invalid utf8, and thus create a nil
NSString? If so, this will throw an exception and hose the browser. Can you
break this up so you can check for nil before looking for the key?

http://codereview.chromium.org/402077/diff/5001/5010
File chrome/browser/cocoa/extensions/extension_popup_controller.h (right):

http://codereview.chromium.org/402077/diff/5001/5010#newcode20
Line 20: // user has clicked on a browser action button. It instantiates the
extension
Can there be more than one of these at the same time?

http://codereview.chromium.org/402077/diff/5001/5010#newcode24
Line 24: // The native extension view retrieved from the extension host. Weak.
@private, indented one space

http://codereview.chromium.org/402077/diff/5001/5010#newcode46
Line 46: // |anchoredAt| is expected to be relative to the screen at the bottom
center of
instead of saying "relative to the screen" maybe say that it's in the screen's
coordinate system?

http://codereview.chromium.org/402077/diff/5001/5011
File chrome/browser/cocoa/extensions/extension_popup_controller.mm (right):

http://codereview.chromium.org/402077/diff/5001/5011#newcode26
Line 26: // Callers should be using the public static method for initialization.
Mention that this takes ownership of |host|.

http://codereview.chromium.org/402077/diff/5001/5011#newcode74
Line 74: if (!self)
just |return self|. If it's nil, returning self will return nil. No need for the
if case.

http://codereview.chromium.org/402077/diff/5001/5011#newcode80
Line 80: - (void)dealloc {
you also need to implement -windowWillClose: to call [self autorelease] and
release controller at that point. That's the right way to destroy a
NSWindowController. Don't put this into an scoped_nsobject.

http://codereview.chromium.org/402077/diff/5001/5011#newcode81
Line 81: [super dealloc];
must be called at the end of dealloc, not first.

http://codereview.chromium.org/402077/diff/5001/5011#newcode92
Line 92: 
omit blank line.

http://codereview.chromium.org/402077/diff/5001/5011#newcode120
Line 120: ExtensionPopupController* popup = [[[ExtensionPopupController alloc]
takes ownership of |host|

http://codereview.chromium.org/402077/diff/5001/5011#newcode124
Line 124: arrowLocation:arrowLocation] autorelease];
shouldn't be autoreleasing this (despite my comment in the last round), so I'd
rename this method to "createAndShowUrl...." to match the guidelines.

http://codereview.chromium.org/402077/diff/5001/5012
File chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm
(right):

http://codereview.chromium.org/402077/diff/5001/5012#newcode73
Line 73: profile_->ShutdownExtensionProfile();
call CocoaTest::TearDown() at the end.

http://codereview.chromium.org/402077/diff/5001/5012#newcode84
Line 84: // and extensions for easy testing need to be implemented.
put bug # in TODO.

[Bons, 2009-11-20 01:38:56]

http://codereview.chromium.org/402077/diff/5001/5005
File chrome/browser/cocoa/extensions/browser_actions_controller.h (right):

http://codereview.chromium.org/402077/diff/5001/5005#newcode48
Line 48: scoped_nsobject<ExtensionPopupController> popupController_;
On 2009/11/20 01:09:42, pink wrote:
> NSWindowController shouldn't be put into a scoped_nsobject. Just mark this as
a
> weak reference as the window will clean itself up when closed.

Done.

http://codereview.chromium.org/402077/diff/5001/5006
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/402077/diff/5001/5006#newcode415
Line 415: [sender extension]->id())];
On 2009/11/20 01:09:42, pink wrote:
> is it every possible for this to be invalid utf8, and thus create a nil
> NSString? If so, this will throw an exception and hose the browser. Can you
> break this up so you can check for nil before looking for the key?

Done.

http://codereview.chromium.org/402077/diff/5001/5010
File chrome/browser/cocoa/extensions/extension_popup_controller.h (right):

http://codereview.chromium.org/402077/diff/5001/5010#newcode20
Line 20: // user has clicked on a browser action button. It instantiates the
extension
On 2009/11/20 01:09:42, pink wrote:
> Can there be more than one of these at the same time?

Done.

http://codereview.chromium.org/402077/diff/5001/5010#newcode24
Line 24: // The native extension view retrieved from the extension host. Weak.
On 2009/11/20 01:09:42, pink wrote:
> @private, indented one space

Done.

http://codereview.chromium.org/402077/diff/5001/5010#newcode46
Line 46: // |anchoredAt| is expected to be relative to the screen at the bottom
center of
On 2009/11/20 01:09:42, pink wrote:
> instead of saying "relative to the screen" maybe say that it's in the screen's
> coordinate system?

Done.

http://codereview.chromium.org/402077/diff/5001/5011
File chrome/browser/cocoa/extensions/extension_popup_controller.mm (right):

http://codereview.chromium.org/402077/diff/5001/5011#newcode26
Line 26: // Callers should be using the public static method for initialization.
On 2009/11/20 01:09:42, pink wrote:
> Mention that this takes ownership of |host|.

Done.

http://codereview.chromium.org/402077/diff/5001/5011#newcode74
Line 74: if (!self)
On 2009/11/20 01:09:42, pink wrote:
> just |return self|. If it's nil, returning self will return nil. No need for
the
> if case.

Done.

http://codereview.chromium.org/402077/diff/5001/5011#newcode80
Line 80: - (void)dealloc {
On 2009/11/20 01:09:42, pink wrote:
> you also need to implement -windowWillClose: to call [self autorelease] and
> release controller at that point. That's the right way to destroy a
> NSWindowController. Don't put this into an scoped_nsobject.

Done.

http://codereview.chromium.org/402077/diff/5001/5011#newcode81
Line 81: [super dealloc];
On 2009/11/20 01:09:42, pink wrote:
> must be called at the end of dealloc, not first.

Done.

http://codereview.chromium.org/402077/diff/5001/5011#newcode92
Line 92: 
On 2009/11/20 01:09:42, pink wrote:
> omit blank line.

Done.

http://codereview.chromium.org/402077/diff/5001/5011#newcode120
Line 120: ExtensionPopupController* popup = [[[ExtensionPopupController alloc]
On 2009/11/20 01:09:42, pink wrote:
> takes ownership of |host|

Done.

http://codereview.chromium.org/402077/diff/5001/5011#newcode124
Line 124: arrowLocation:arrowLocation] autorelease];
On 2009/11/20 01:09:42, pink wrote:
> shouldn't be autoreleasing this (despite my comment in the last round), so I'd
> rename this method to "createAndShowUrl...." to match the guidelines.

Done.

http://codereview.chromium.org/402077/diff/5001/5012
File chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm
(right):

http://codereview.chromium.org/402077/diff/5001/5012#newcode73
Line 73: profile_->ShutdownExtensionProfile();
On 2009/11/20 01:09:42, pink wrote:
> call CocoaTest::TearDown() at the end.

Done.

http://codereview.chromium.org/402077/diff/5001/5012#newcode84
Line 84: // and extensions for easy testing need to be implemented.
On 2009/11/20 01:09:42, pink wrote:
> put bug # in TODO.

Done.

[pink (ping after 24hrs), 2009-11-20 01:55:55]

LGTM with nits.

http://codereview.chromium.org/402077/diff/5013/5018
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/402077/diff/5013/5018#newcode322
Line 322: if (popupController_)
no need to check for nil, the runtime does that for you.

http://codereview.chromium.org/402077/diff/5013/5023
File chrome/browser/cocoa/extensions/extension_popup_controller.mm (right):

http://codereview.chromium.org/402077/diff/5013/5023#newcode121
Line 121: // Takes ownership of |host|.
Also comment that this will autorelease itself when the window closes so don't
use an autorelease here (so someone doesn't read it and think it's a leak).

http://codereview.chromium.org/402077/diff/5013/5023#newcode149
Line 149: windowOrigin.x -= (NSWidth(frame) - kBubbleArrowXOffset -
do you need the outer layer of parens?

http://codereview.chromium.org/402077/diff/5013/5023#newcode151
Line 151: windowOrigin.y -= (NSHeight(frame) - (kBubbleArrowHeight / 2.0));
same question

[Bons, 2009-11-20 02:01:47]

http://codereview.chromium.org/402077/diff/5013/5018
File chrome/browser/cocoa/extensions/browser_actions_controller.mm (right):

http://codereview.chromium.org/402077/diff/5013/5018#newcode322
Line 322: if (popupController_)
On 2009/11/20 01:55:55, pink wrote:
> no need to check for nil, the runtime does that for you.

Done.

http://codereview.chromium.org/402077/diff/5013/5023
File chrome/browser/cocoa/extensions/extension_popup_controller.mm (right):

http://codereview.chromium.org/402077/diff/5013/5023#newcode121
Line 121: // Takes ownership of |host|.
On 2009/11/20 01:55:55, pink wrote:
> Also comment that this will autorelease itself when the window closes so don't
> use an autorelease here (so someone doesn't read it and think it's a leak).

Done.

http://codereview.chromium.org/402077/diff/5013/5023#newcode149
Line 149: windowOrigin.x -= (NSWidth(frame) - kBubbleArrowXOffset -
On 2009/11/20 01:55:55, pink wrote:
> do you need the outer layer of parens?

Done.

http://codereview.chromium.org/402077/diff/5013/5023#newcode151
Line 151: windowOrigin.y -= (NSHeight(frame) - (kBubbleArrowHeight / 2.0));
On 2009/11/20 01:55:55, pink wrote:
> same question

Done.