Port the Extension Install Bubble changes to Mac

Port the Extension Install Bubble changes to Mac.

This makes sure Mac is in sync with Views and GTK when it comes to describing the Extension Command shortcut to the user. 

BUG=121417
TEST=Install an extension with an Extension Command on Mac and watch the install bubble describing the shortcut.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=153658

[Nico, 2012-08-23 15:42:12]

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm
(right):

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm:56: if
((extension->browser_action()) || !extension->omnibox_keyword().empty() ||
Remove parens around first term

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm:59:
extension->browser_action_command() || extension->page_action_command()) {
This condition is unreadable. Is it the same on all platforms? If so, it should
probably be some method on Extension? should_show_install_bubble()?

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
(right):

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:120:
}
Same here: Is this if() the same in all UI implementations?

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:340:
}
This block is now long and nested. Please extract parts of it into new functions
(e.g. the whole block, or maybe you find a way to share some code between the
similar looking two branches. But I'm asking mostly to keep
)calculateWindowHeight readable, not because I'm worried about code reuse)

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:459:
type_ == extension_installed_bubble::kOmniboxKeyword) {
This again looks like something that should be shared across ports.

[Finnur, 2012-08-24 12:55:26]

Nico, please take another look.

Elliot, can you give owners approval for browser/ui/GTK.
Scott, can you give owners approval for browser/ui/views

Yoyo, can you review:
chrome/browser/extensions/api/commands/command_service.cc
chrome/common/extensions/extension.h

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm
(right):

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm:56: if
((extension->browser_action()) || !extension->omnibox_keyword().empty() ||
Done. I also removed browser_action_command() altogether, because
browser_action() implies browser_action_command() -- page_action(), however,
does not imply page_action_command().

On 2012/08/23 15:42:12, Nico wrote:
> Remove parens around first term

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm:59:
extension->browser_action_command() || extension->page_action_command()) {
Mac is the only platform that decided to show the Extension Install as an
Infobar, the other platforms show the bubble. However, I've added
verbose_install_message(), which matches what you intended, I think.

On 2012/08/23 15:42:12, Nico wrote:
> This condition is unreadable. Is it the same on all platforms? If so, it
should
> probably be some method on Extension? should_show_install_bubble()?

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
(right):

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:340:
}
Done.

On 2012/08/23 15:42:12, Nico wrote:
> This block is now long and nested. Please extract parts of it into new
functions
> (e.g. the whole block, or maybe you find a way to share some code between the
> similar looking two branches. But I'm asking mostly to keep
> )calculateWindowHeight readable, not because I'm worried about code reuse)

[Aaron Boodman, 2012-08-24 13:48:57]

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm
(right):

http://codereview.chromium.org/10883008/diff/3001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_bridge.mm:59:
extension->browser_action_command() || extension->page_action_command()) {
On 2012/08/24 12:55:26, Finnur wrote:
> Mac is the only platform that decided to show the Extension Install as an
> Infobar, the other platforms show the bubble. However, I've added
> verbose_install_message(), which matches what you intended, I think.
> 
> On 2012/08/23 15:42:12, Nico wrote:
> > This condition is unreadable. Is it the same on all platforms? If so, it
> should
> > probably be some method on Extension? should_show_install_bubble()?
> 

If it helps at all, we want to remove the infobar on mac: crbug.com/109940.

[Yoyo Zhou, 2012-08-24 15:56:21]

extension stuff LGTM but for 1 comment

http://codereview.chromium.org/10883008/diff/12010/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10883008/diff/12010/chrome/common/extensions/e...
chrome/common/extensions/extension.h:612: bool verbose_install_message() const {
I would rename this to something like is_install_message_verbose to be
consistent with other bool accessors in this class.

[Elliot Glaysher, 2012-08-24 17:06:30]

gtk lgtm

[Finnur, 2012-08-27 14:14:05]

Nico: Ping

[Nico, 2012-08-27 19:08:40]

Thanks, I like this much better. LGTM with comments addressed.

> Ping.

I was out on friday. You can use who/reviewerlogin/calendar to check if
reviewers are away.

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.h
(right):

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.h:113:
- (NSString*)getResourceIdForExtensionAction;
nit: This returns a string, not an Id, so it shouldn't be named "getresourceid".
Also, cocoa getters don't start with "get" (the function names below are wrong
too)). Maybe "infoText"?

Also, this isn't used by tests, so it can be declared in a private category in
the .mm file.

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm
(right):

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller.mm:297:
} else {
CHECK_EQ(extension_installed_bubble::kBrowserAction, type_);

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
File
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm
(right):

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm:192:
EXPECT_EQ(msg2Frame.origin.y,
NSMinY(msg2Frame)

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm:193:
msg3Frame.origin.y + msg3Frame.size.height +
NSMaxY(msg3Frame)

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm:197:
EXPECT_EQ(msg1Frame.origin.y,
same

http://codereview.chromium.org/10883008/diff/5008/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/extension_installed_bubble_controller_unittest.mm:198:
msg2Frame.origin.y + msg2Frame.size.height +
same