Refactor implementation of BrowserActions, and add support for

Refactor BrowserActions, and add support for
tab-specific state.

Future changelists will move Page Actions over to
ExtensionAction2, then replace ExtensionAction and
ExtensionActionState with ExtensionAction2.

Also, fix a bug in setIcon({path:...}) where it
would work only the first time.

BUG=24669, 24472
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29997

[Aaron Boodman, 2009-10-22 07:39:50]

Sorry about the mega CL.

Note that this change is relative to http://codereview.chromium.org/293044/show,
though that is not checked in.

Also, I still need to update the linux port.

[Matt Perry, 2009-10-22 18:24:28]

mostly lgtm

http://codereview.chromium.org/306044/diff/53/1106
File chrome/browser/extensions/extension_browser_actions_api.cc (right):

http://codereview.chromium.org/306044/diff/53/1106#newcode66
Line 66: browser_action_->SetDefaultIcon(icon_index);
Since the browser action API was never finalized, can we just drop
setIcon(iconIndex)?

http://codereview.chromium.org/306044/diff/53/1112
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/306044/diff/53/1112#newcode209
Line 209: ExtensionActionState::PaintBadge(
nit: add a TODO

http://codereview.chromium.org/306044/diff/53/1112#newcode285
Line 285: if ((*iter)->button()->extension()->id() == extension->id()) {
couldn't you just compare the Extension pointers directly? or the
ExtensionAction2 ptrs?

http://codereview.chromium.org/306044/diff/53/1117
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/306044/diff/53/1117#newcode389
Line 389: const DictionaryValue* page_action, std::string* error) {
nit: rename to extension_action

http://codereview.chromium.org/306044/diff/53/1117#newcode428
Line 428: // TODO(EXTENSIONS_DEPRECATED): one icon.
you can remove this TODO, since there's nothing to do when we drop the old api

[asargent_no_longer_on_chrome, 2009-10-22 18:53:34]

lgtm

(I only focused on extension_browser_actions_api.{h,cc})

http://codereview.chromium.org/306044/diff/53/1106
File chrome/browser/extensions/extension_browser_actions_api.cc (right):

http://codereview.chromium.org/306044/diff/53/1106#newcode65
Line 65: // setIcon({iconIndex:...});
This comment was confusing to me for a moment; consider rewording slightly to
something like "setIcon used to only support an iconIndex parameter, and had no
support for tab-specific state"

http://codereview.chromium.org/306044/diff/53/1116
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/306044/diff/53/1116#newcode991
Line 991: "description": "Sets the icon for the browser action. The icon can be
specified either as the index of one of the icons that was pre-specified in the
manifest, or as the pixel data from a Canvas element. Either the path or the
imageData property must be specified.",
Looks like this description of setIcon is now contradictory - it first says
index or pixel data are only choices, then says path or pixel data are the only
choices.

[Aaron Boodman, 2009-10-22 20:27:12]

http://codereview.chromium.org/306044/diff/53/1106
File chrome/browser/extensions/extension_browser_actions_api.cc (right):

http://codereview.chromium.org/306044/diff/53/1106#newcode65
Line 65: // setIcon({iconIndex:...});
On 2009/10/22 18:53:34, Antony Sargent wrote:
> This comment was confusing to me for a moment; consider rewording slightly to
> something like "setIcon used to only support an iconIndex parameter, and had
no
> support for tab-specific state"

Per Matt's suggestion, I removed this functionality, so this request is no
longer applicable.

http://codereview.chromium.org/306044/diff/53/1106#newcode66
Line 66: browser_action_->SetDefaultIcon(icon_index);
On 2009/10/22 18:24:28, Matt Perry wrote:
> Since the browser action API was never finalized, can we just drop
> setIcon(iconIndex)?

Good idea. Done.

http://codereview.chromium.org/306044/diff/53/1112
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/306044/diff/53/1112#newcode285
Line 285: if ((*iter)->button()->extension()->id() == extension->id()) {
On 2009/10/22 18:24:28, Matt Perry wrote:
> couldn't you just compare the Extension pointers directly? or the
> ExtensionAction2 ptrs?

Done.

http://codereview.chromium.org/306044/diff/53/1116
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/306044/diff/53/1116#newcode991
Line 991: "description": "Sets the icon for the browser action. The icon can be
specified either as the index of one of the icons that was pre-specified in the
manifest, or as the pixel data from a Canvas element. Either the path or the
imageData property must be specified.",
On 2009/10/22 18:53:34, Antony Sargent wrote:
> Looks like this description of setIcon is now contradictory - it first says
> index or pixel data are only choices, then says path or pixel data are the
only
> choices. 

Nice catch, fixed. This also made me notice that we were missing the "path"
option from pageAction.setIcon(), though the code does support it, so I added
that.

http://codereview.chromium.org/306044/diff/53/1117
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/306044/diff/53/1117#newcode389
Line 389: const DictionaryValue* page_action, std::string* error) {
On 2009/10/22 18:24:28, Matt Perry wrote:
> nit: rename to extension_action

Done.

http://codereview.chromium.org/306044/diff/53/1117#newcode428
Line 428: // TODO(EXTENSIONS_DEPRECATED): one icon.
On 2009/10/22 18:24:28, Matt Perry wrote:
> you can remove this TODO, since there's nothing to do when we drop the old api

Done.

[asargent_no_longer_on_chrome, 2009-10-22 20:49:40]

http://codereview.chromium.org/306044/diff/7003/7015
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/306044/diff/7003/7015#newcode860
Line 860: "description": "Sets the icon for the page action. The icon can be
specified either the path to an image inside the extension, or as the pixel data
from a Canvas element. Either the path or the imageData property must be
specified.",
typo: "... can be specified either the path..."

http://codereview.chromium.org/306044/diff/7003/7015#newcode996
Line 996: "description": "Sets the icon for the page action. The icon can be
specified either the path to an image inside the extension, or as the pixel data
from a Canvas element. Either the path or the imageData property must be
specified.",
same typo

[Matt Perry, 2009-10-22 20:50:36]

LGTM

[Aaron Boodman, 2009-10-23 05:00:36]

+estade

Evan, can you please review the files that end in _gtk.* ?

[Evan Stade, 2009-10-23 17:39:09]

LGTM. Does this fix bug 24669?

[Aaron Boodman, 2009-10-23 17:49:33]

On 2009/10/23 17:39:09, Evan Stade wrote:
> Does this fix bug 24669?

Yes! Thanks for the pointer.