PageActions can now specify multiple icons and switch between them...

PageActions can now specify multiple icons and switch between them
using optional parameters to enableForTab.

BUG=http://crbug.com/11906
TEST=None


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

[Finnur, 2009-06-26 20:03:01]

Please take a look.

Changes since patch set 1: I added an updated page_action.crx to the changelist
to get the unit test working and added comment to extensions_service_unittest.cc
(plus a missing check I happened to notice).

You can ignore the change listed in extension_page_actions_module.cc, I already
submitted that earlier today.

[Aaron Boodman, 2009-06-29 21:42:57]

This is great, thanks for pushing on it and sorry for the delayed review.

One meta-comment (maybe for a subsequence CL?): I wonder if we can get away with
not specifying the list of images at all, and instead validating them lazily
with the utility process. Maybe Matt can comment on how hard/slow that would be?

http://codereview.chromium.org/149046/diff/23/32
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/149046/diff/23/32#newcode38
Line 38: action->GetString(keys::kTitleKey, &title);
You should still validate that if they exist, they are the right type, so:

if (HasString(kTitleKey)) EXTENSION_FUNCTION_VALIDATE(GetString(kTitleKey))

http://codereview.chromium.org/149046/diff/23/36
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/149046/diff/23/36#newcode263
Line 263: // Returns the page action state for this tab. The pair returns
contains
Add blank line before this comment.

http://codereview.chromium.org/149046/diff/23/36#newcode265
Line 265: std::pair<std::string, int> PageActionInfo(const PageAction*
page_action);
This method name should start with a verb. How about GetPageActionState() ?

http://codereview.chromium.org/149046/diff/23/36#newcode1047
Line 1047: std::map< const PageAction*, std::pair<std::string, int> >
I think it might be cleaner to introduce a struct called PageActionState. It
would encapsulate the tooltip and the icon state. The method could be
PageAction* GetPageActionState(const PageAction&) and would return NULL if the
page action is not enabled, which would also eliminate the need for
IsPageActionEnabled().

http://codereview.chromium.org/149046/diff/23/37
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/149046/diff/23/37#newcode135
Line 135: const char* Extension::kInvalidPageActionIconPathsListError =
It seems like kInvalidPageActionIconPathsListError is not necessary since we
already have kInvalidPageActionIconPathsError?

http://codereview.chromium.org/149046/diff/23/39
File chrome/common/page_action.h (right):

http://codereview.chromium.org/149046/diff/23/39#newcode38
Line 38: std::vector<FilePath> icon_paths() const { return icon_paths_; }
This is going to return a copy everytime we call it. Maybe make this a const
reference?

http://codereview.chromium.org/149046/diff/23/27
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/149046/diff/23/27#newcode363
Line 363: iconId: chrome.types.optPInt,
How about just making this a path, not an ID?

http://codereview.chromium.org/149046/diff/23/26
File chrome/test/data/extensions/samples/subscribe_page_action/background.html
(right):

http://codereview.chromium.org/149046/diff/23/26#newcode37
Line 37: title: "Click to subscribe...",
80 cols

[Matt Perry, 2009-06-29 22:14:56]

On 2009/06/29 21:42:57, Aaron Boodman wrote:
> One meta-comment (maybe for a subsequence CL?): I wonder if we can get away
with
> not specifying the list of images at all, and instead validating them lazily
> with the utility process. Maybe Matt can comment on how hard/slow that would
be?

I couldn't say offhand.  If the image loading is already async, it seems to me
inserting a transcode step in there shouldn't be too hard.  It would obviously
be slower, possibly noticeably depending on the image size.

But even if it was easy and fast, I don't know that it would be necessarily
better than specifying them in the manifest up-front.

[Finnur, 2009-07-01 20:28:32]

I've updated the patch. I believe I've addressed everything but the idea of not
having to specify the images in the manifest (to specify it instead in the API
call). 

I think that would be good for a future changelist, if we decide to do it.
Speaking of which, I'm with Matt -- it seems (at least on the face of it) like
we don't gain much... Or am I missing something?


On 2009/06/29 22:14:56, Matt Perry wrote:
> On 2009/06/29 21:42:57, Aaron Boodman wrote:
> > One meta-comment (maybe for a subsequence CL?): I wonder if we can get away
> with
> > not specifying the list of images at all, and instead validating them lazily
> > with the utility process. Maybe Matt can comment on how hard/slow that would
> be?
> 
> I couldn't say offhand.  If the image loading is already async, it seems to me
> inserting a transcode step in there shouldn't be too hard.  It would obviously
> be slower, possibly noticeably depending on the image size.
> 
> But even if it was easy and fast, I don't know that it would be necessarily
> better than specifying them in the manifest up-front.

[Aaron Boodman, 2009-07-01 21:17:49]

lgtm

On 2009/07/01 20:28:32, Finnur wrote:
> I think that would be good for a future changelist, if we decide to do it.
> Speaking of which, I'm with Matt -- it seems (at least on the face of it) like
> we don't gain much... Or am I missing something?

We gain increased awesomeness. It is a bit dorky to have to specify all the
states you might want to put this image into ahead of time. What if there is a
long list?

[Finnur, 2009-07-01 21:52:03]

Increased awesomeness and decreased dorkiness is something all coders should
strive for. 

Although... is Matt on board or has the image unpacker code reached perfection
already? :)

In the meantime, I'll check this in.

 On 2009/07/01 21:17:49, Aaron Boodman wrote:
> lgtm
> 
> On 2009/07/01 20:28:32, Finnur wrote:
> > I think that would be good for a future changelist, if we decide to do it.
> > Speaking of which, I'm with Matt -- it seems (at least on the face of it)
like
> > we don't gain much... Or am I missing something?
> 
> We gain increased awesomeness. It is a bit dorky to have to specify all the
> states you might want to put this image into ahead of time. What if there is a
> long list?

[Matt Perry, 2009-07-01 22:05:05]

On 2009/07/01 21:52:03, Finnur wrote:
> Increased awesomeness and decreased dorkiness is something all coders should
> strive for. 
> 
> Although... is Matt on board or has the image unpacker code reached perfection
> already? :)

It's technically feasible to do what Aaron suggests.  I don't know a whole lot
about the page actions API or what it would look like either way.  I'll trust
you guys to do the right thing.

[Finnur, 2009-07-01 22:06:31]

I thought he was soliciting your feedback because the unpacker would need to
change? Do you not rely on the manifest to know which images to unpack? That
information would no longer be present, if we change this.

On 2009/07/01 22:05:05, Matt Perry wrote:
> On 2009/07/01 21:52:03, Finnur wrote:
> > Increased awesomeness and decreased dorkiness is something all coders should
> > strive for. 
> > 
> > Although... is Matt on board or has the image unpacker code reached
perfection
> > already? :)
> 
> It's technically feasible to do what Aaron suggests.  I don't know a whole lot
> about the page actions API or what it would look like either way.  I'll trust
> you guys to do the right thing.