Implement Browser Actions extensions....

Implement Browser Actions extensions.

Browser Actions are like Page Actions, except they appear next to the Omnibox and are always visible. For details see http://code.google.com/p/chromium/wiki/BrowserActions.

Added a simple browser action sample that adds a Print button to the chrome toolbar (which brings up the Print dialog for the current page).

Removed |type| from PageActions, which is currently ignored and was already removed from the docs.

Each extension can only have 1 browser_action. Each browser action can specify more than one icon, but only the first is used. And no API has been added yet (besides the event definition).

BUG=22099
TEST=Install the sample browser action, navigate to google.com, press the print button. A print dialog should come up.

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

[Finnur, 2009-09-25 14:47:34]

This looks a bit scarier than it is, but a lot of files just have simple changes
because the PageAction class changed name.

I'm not married to the name of the class (ContextualAction), and would be happy
to go through the code of the patch once more and change it if you have a better
suggestion.

Also - Currently, we share the parsing code with Page Actions, which I think is
the right thing to do (even though it is not consistent with the API definition
as stated). If we want to change the manifest format we should probably change
it for PageAction as well.

[Erik does not do reviews, 2009-09-25 17:10:48]

I'm unhappy about the 1/many split between browser actions and page actions.  To
my mind, we should have the same behavior for both page actions and browser
actions.  They take up the same real estate, so allowing many in either has the
roughly the same effect (not 100% since page actions can hide).  Let's see if we
can come to a decision on this one way or the other before you land this patch.

http://codereview.chromium.org/243001/diff/7001/8002
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/243001/diff/7001/8002#newcode232
Line 232: for (ContextualActionMap::const_iterator i(browser_actions.begin());
It looks like other parts of the code doesn't support more than one browser
action for an extension.  If we enforce that, we should validate that this list
only has one item here.

http://codereview.chromium.org/243001/diff/7001/8007
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/243001/diff/7001/8007#newcode110
Line 110: for (ContextualActionMap::const_iterator i =
extension->page_actions().begin();
80 cols

http://codereview.chromium.org/243001/diff/7001/8005
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/243001/diff/7001/8005#newcode107
Line 107: std::vector<ContextualAction*> GetContextualActions(
This seems cumbersome to expose in this way.  Make this private and add
convenience functions GetPageActions() and GetBrowserActions().

http://codereview.chromium.org/243001/diff/7001/8027
File chrome/browser/views/browser_actions_panel.cc (right):

http://codereview.chromium.org/243001/diff/7001/8027#newcode27
Line 27: // BrowserActionImageView
a comment explaining what this does above and beyond ImageButton seems warranted

http://codereview.chromium.org/243001/diff/7001/8027#newcode45
Line 45: // The browser action this view represents.
add comment indicating that this isn't owned here

http://codereview.chromium.org/243001/diff/7001/8027#newcode53
Line 53: ImageLoadingTracker* tracker_;
the ownership model of this object is odd and deserves a comment

http://codereview.chromium.org/243001/diff/7001/8027#newcode56
Line 56: BrowserActionsPanel* panel_;
This is always just the same as GetParent isn't it?  Do we need to have the
instance variable as well?

http://codereview.chromium.org/243001/diff/7001/8027#newcode84
Line 84: tracker_->StopTrackingImageLoad();
add comment explaining that it deletes itself here?

http://codereview.chromium.org/243001/diff/7001/8027#newcode98
Line 98: SetImage(views::CustomButton::BS_PUSHED, image);
Hmm.  I wonder if we need to support alternate images for these states?

http://codereview.chromium.org/243001/diff/7001/8027#newcode111
Line 111: : profile_(profile), toolbar_(toolbar) {
nit: indent

http://codereview.chromium.org/243001/diff/7001/8027#newcode113
Line 113: registrar_.Add(this, NotificationType::EXTENSION_LOADED,
Don't you also need to look for EXTENSIONS_READY and handle the case when all
extensions haven't yet been loaded?  I'm wondering about startup race conditions
here.

http://codereview.chromium.org/243001/diff/7001/8027#newcode129
Line 129: if (!extension_service)
I don't think this check is a good idea here.  Looking at the two callsites, it
would be a very odd thing for the service to come back null at this point.

http://codereview.chromium.org/243001/diff/7001/8027#newcode136
Line 136: if (browser_action_views_.size() != browser_actions.size())
this seems wrong.  if these sizes ever wind up being equal, then the loop below
will do some very bad things.  I also don't see a code path where this should
happen.

http://codereview.chromium.org/243001/diff/7001/8027#newcode176
Line 176: (kIconSize + kIconHorizontalPadding),
This indenting is really odd.  Could you just pull out the calculation into
another variable first for clarity?

http://codereview.chromium.org/243001/diff/7001/8027#newcode185
Line 185: (kIconSize + kIconHorizontalPadding),
same here

http://codereview.chromium.org/243001/diff/7001/8027#newcode186
Line 186: (height() - kIconSize) / 2,
it's odd to use a constant here, but sz.height() below.  These shouldn't be out
of sync, so we should just use one.  maybe the same is true of sz.width()?

http://codereview.chromium.org/243001/diff/7001/8028
File chrome/browser/views/browser_actions_panel.h (right):

http://codereview.chromium.org/243001/diff/7001/8028#newcode28
Line 28: class BrowserActionsPanel : public views::View,
"Panel" seems like an odd name here.  I thought the connotation of this was that
it would be more of a standalone window/popup associated with the app rather
than a view embedded in another window.  I think other uses of it in the code
support this as well.  How about BrowserActionsView or BrowserActionsContainer?

http://codereview.chromium.org/243001/diff/7001/8028#newcode41
Line 41: void OnBrowserActionVisiblityChanged();
typo: s/Visiblity/Visibility/

http://codereview.chromium.org/243001/diff/7001/8011
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/243001/diff/7001/8011#newcode1264
Line 1264: const ContextualActionState* state =
contents->GetPageActionState(page_action_);
80 cols

http://codereview.chromium.org/243001/diff/7001/8017
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/243001/diff/7001/8017#newcode110
Line 110: i != page_actions_.end(); ++i)
replace these with STLDeleteValues

http://codereview.chromium.org/243001/diff/7001/8017#newcode159
Line 159: ContextualActionMap::const_iterator it = page_actions_.find(id);
avoid the duplicate logic by setting a local ContextualActionMap* in the if/else
and then using that variable in common code

http://codereview.chromium.org/243001/diff/7001/8018
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/243001/diff/7001/8018#newcode297
Line 297: // Helper method that loads a PageAction object from a
update comment - I think there are other examples of this, so maybe do a search

http://codereview.chromium.org/243001/diff/7001/8020
File chrome/common/page_action.h (right):

http://codereview.chromium.org/243001/diff/7001/8020#newcode51
Line 51: // The id for the extension this ContextualAction belongs to (as
defined in the
80 cols

[Finnur, 2009-09-25 22:34:38]

I have updated the changelist. Please have another look.

As discussed offline - we've decided to go with one browser action per
extension. We can tweak it with a follow-up patch if we come to a different
conclusion.

http://codereview.chromium.org/243001/diff/7001/8002
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/243001/diff/7001/8002#newcode232
Line 232: for (ContextualActionMap::const_iterator i(browser_actions.begin());
I have changed extension.h to have a ContextualAction* instead of a map of
ContextualActions. That changes this and a few other places.

On 2009/09/25 17:10:48, Erik Kay wrote:
> It looks like other parts of the code doesn't support more than one browser
> action for an extension.  If we enforce that, we should validate that this
list
> only has one item here.

http://codereview.chromium.org/243001/diff/7001/8007
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/243001/diff/7001/8007#newcode110
Line 110: for (ContextualActionMap::const_iterator i =
extension->page_actions().begin();
On 2009/09/25 17:10:48, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/243001/diff/7001/8005
File chrome/browser/extensions/extensions_service.h (right):

http://codereview.chromium.org/243001/diff/7001/8005#newcode107
Line 107: std::vector<ContextualAction*> GetContextualActions(
I agree. Done.

http://codereview.chromium.org/243001/diff/7001/8027
File chrome/browser/views/browser_actions_panel.cc (right):

http://codereview.chromium.org/243001/diff/7001/8027#newcode27
Line 27: // BrowserActionImageView
On 2009/09/25 17:10:48, Erik Kay wrote:
> a comment explaining what this does above and beyond ImageButton seems
warranted

Done.

http://codereview.chromium.org/243001/diff/7001/8027#newcode45
Line 45: // The browser action this view represents.
On 2009/09/25 17:10:48, Erik Kay wrote:
> add comment indicating that this isn't owned here

Done.

http://codereview.chromium.org/243001/diff/7001/8027#newcode53
Line 53: ImageLoadingTracker* tracker_;
On 2009/09/25 17:10:48, Erik Kay wrote:
> the ownership model of this object is odd and deserves a comment

Done.

http://codereview.chromium.org/243001/diff/7001/8027#newcode56
Line 56: BrowserActionsPanel* panel_;
Well, yes. Although GetParent() returns a View*, which we'd need to convert so
we can call the functions added by the superclass. I thought this was simpler,
but don't have a strong opinion. If you prefer to cast it, I can change it. Just
let me know.

On 2009/09/25 17:10:48, Erik Kay wrote:
> This is always just the same as GetParent isn't it?  Do we need to have the
> instance variable as well?

http://codereview.chromium.org/243001/diff/7001/8027#newcode84
Line 84: tracker_->StopTrackingImageLoad();
On 2009/09/25 17:10:48, Erik Kay wrote:
> add comment explaining that it deletes itself here?

Yeah, I added an explicit call = NULL here and a comment, to make it explicit.

http://codereview.chromium.org/243001/diff/7001/8027#newcode98
Line 98: SetImage(views::CustomButton::BS_PUSHED, image);
I think we should probably do that for both this and PageActions. Not in this
changelist though. :)

On 2009/09/25 17:10:48, Erik Kay wrote:
> Hmm.  I wonder if we need to support alternate images for these states?

http://codereview.chromium.org/243001/diff/7001/8027#newcode111
Line 111: : profile_(profile), toolbar_(toolbar) {
On 2009/09/25 17:10:48, Erik Kay wrote:
> nit: indent

Done.

http://codereview.chromium.org/243001/diff/7001/8027#newcode113
Line 113: registrar_.Add(this, NotificationType::EXTENSION_LOADED,
I refresh the list of browser actions at start. Sometimes my browser actions
have already loaded and sometimes not. When they haven't, they appear as
EXTENSION_LOADED events come in so I still catch them. Or am I misunderstanding
something?

On 2009/09/25 17:10:48, Erik Kay wrote:
> Don't you also need to look for EXTENSIONS_READY and handle the case when all
> extensions haven't yet been loaded?  I'm wondering about startup race
conditions
> here.

http://codereview.chromium.org/243001/diff/7001/8027#newcode129
Line 129: if (!extension_service)
Added comment.

On 2009/09/25 17:10:48, Erik Kay wrote:
> I don't think this check is a good idea here.  Looking at the two callsites,
it
> would be a very odd thing for the service to come back null at this point.

http://codereview.chromium.org/243001/diff/7001/8027#newcode136
Line 136: if (browser_action_views_.size() != browser_actions.size())
Woops! Forgot the braces here. Good catch.

On 2009/09/25 17:10:48, Erik Kay wrote:
> this seems wrong.  if these sizes ever wind up being equal, then the loop
below
> will do some very bad things.  I also don't see a code path where this should
> happen.

http://codereview.chromium.org/243001/diff/7001/8027#newcode176
Line 176: (kIconSize + kIconHorizontalPadding),
On 2009/09/25 17:10:48, Erik Kay wrote:
> This indenting is really odd.  Could you just pull out the calculation into
> another variable first for clarity?

Done.

http://codereview.chromium.org/243001/diff/7001/8027#newcode185
Line 185: (kIconSize + kIconHorizontalPadding),
On 2009/09/25 17:10:48, Erik Kay wrote:
> same here

Done.

http://codereview.chromium.org/243001/diff/7001/8027#newcode186
Line 186: (height() - kIconSize) / 2,
Sure. Not sure what you mean by "maybe the same is true of sz.width()?" though. 

On 2009/09/25 17:10:48, Erik Kay wrote:
> it's odd to use a constant here, but sz.height() below.  These shouldn't be
out
> of sync, so we should just use one.  maybe the same is true of sz.width()?

http://codereview.chromium.org/243001/diff/7001/8028
File chrome/browser/views/browser_actions_panel.h (right):

http://codereview.chromium.org/243001/diff/7001/8028#newcode28
Line 28: class BrowserActionsPanel : public views::View,
Changed to BrowserActionsContainer

http://codereview.chromium.org/243001/diff/7001/8028#newcode41
Line 41: void OnBrowserActionVisiblityChanged();
No, this is actually visiblitty.

Just kidding. Fixed.

On 2009/09/25 17:10:48, Erik Kay wrote:
> typo: s/Visiblity/Visibility/

http://codereview.chromium.org/243001/diff/7001/8011
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/243001/diff/7001/8011#newcode1264
Line 1264: const ContextualActionState* state =
contents->GetPageActionState(page_action_);
On 2009/09/25 17:10:48, Erik Kay wrote:
> 80 cols

Done.

http://codereview.chromium.org/243001/diff/7001/8017
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/243001/diff/7001/8017#newcode110
Line 110: i != page_actions_.end(); ++i)
On 2009/09/25 17:10:48, Erik Kay wrote:
> replace these with STLDeleteValues

Oh, neat! Done.

http://codereview.chromium.org/243001/diff/7001/8017#newcode159
Line 159: ContextualActionMap::const_iterator it = page_actions_.find(id);
Good idea.

http://codereview.chromium.org/243001/diff/7001/8018
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/243001/diff/7001/8018#newcode297
Line 297: // Helper method that loads a PageAction object from a
There are other cases, but those cases I left intentionally that way because
they only apply to page actions (ie. in TabContents or in LocationBarView).

http://codereview.chromium.org/243001/diff/7001/8020
File chrome/common/page_action.h (right):

http://codereview.chromium.org/243001/diff/7001/8020#newcode51
Line 51: // The id for the extension this ContextualAction belongs to (as
defined in the
On 2009/09/25 17:10:48, Erik Kay wrote:
> 80 cols

Done.

[Erik does not do reviews, 2009-09-25 22:56:11]

LGTM

http://codereview.chromium.org/243001/diff/6078/5061
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/243001/diff/6078/5061#newcode111
Line 111: delete browser_action_;
just change this to a scoped_ptr

http://codereview.chromium.org/243001/diff/6078/5061#newcode156
Line 156: return browser_action_;
DCHECK(id.empty())