Implement new page action API.

Implement new page action API.

Still no actual drawing code for badges, though.

BUG=24635
TEST=Load the sample test_page_action extension and click the page action to cycle through various states.

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

[Erik does not do reviews, 2009-10-14 22:25:30]

http://codereview.chromium.org/269079/diff/5001/6002
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/269079/diff/5001/6002#newcode120
Line 120: EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&tab_id));
I think it would be cleaner to do this in InitCommon instead and just make
tab_id an instance variable.

http://codereview.chromium.org/269079/diff/5001/6002#newcode146
Line 146: // setIcon can take a variant argument: either a canvas ImageData, or
an
this is disconnected from the variant logic.  move it closer.

http://codereview.chromium.org/269079/diff/5001/6002#newcode217
Line 217: // TODO(mpcomplete): implement badges for page actions.
This doesn't seem like the right place for the TODO.  Shouldn't it be where page
actions are displayed?  Looking at this code, it looks like it's implemented.

http://codereview.chromium.org/269079/diff/5001/6002#newcode238
Line 238: EXTENSION_FUNCTION_VALIDATE(color_value->GetInteger(i,
&color_array[i]));
do we also need to validate the range of the color values? (same with above)

http://codereview.chromium.org/269079/diff/5001/6002#newcode240
Line 240: //SkColor color = SkColorSetARGB(color_array[0], color_array[1],
color_array[2],
Why is this code commented out and not the others in the other two methods?  If
it needs to be disabled, just remove the commented out code and leave the todo
(or fix the 80 cols here and below)

http://codereview.chromium.org/269079/diff/5001/6002#newcode261
Line 261: // TODO(mpcomplete): implement badges for page actions.
same note about TODO

http://codereview.chromium.org/269079/diff/5001/6006
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/269079/diff/5001/6006#newcode735
Line 735: // Set the image.
nit: newline above comment

http://codereview.chromium.org/269079/diff/5001/6006#newcode754
Line 754: // The pixbuf might not be loaded yet.
nit: newline above comment

http://codereview.chromium.org/269079/diff/5001/6009
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/269079/diff/5001/6009#newcode274
Line 274: // to invalidate page actions when done.
I'm probably being dense, but I don't understand the last part of this comment. 
It seems to imply a use-case I'm missing.

http://codereview.chromium.org/269079/diff/5001/6010
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/269079/diff/5001/6010#newcode1255
Line 1255: // Set the image.
nit: newline above comment

http://codereview.chromium.org/269079/diff/5001/6010#newcode1259
Line 1259: // The image index (if not within bounds) will be set to the first
image.
nit: move this comment before the previous line

http://codereview.chromium.org/269079/diff/5001/6013
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/269079/diff/5001/6013#newcode413
Line 413: static v8::Handle<v8::Value> SetBrowserActionIcon(const v8::Arguments&
args) {
rename this and update the comment to indicate that it's more generic now. 
Perhaps SetExtensionActionIcon?

http://codereview.chromium.org/269079/diff/5001/6015
File chrome/test/data/extensions/samples/test_page_action/background.html
(right):

http://codereview.chromium.org/269079/diff/5001/6015#newcode7
Line 7: //chrome.pageActions.enableForTab("", {tabId: tabId, url: p.url});
remove commented out code

http://codereview.chromium.org/269079/diff/5001/6015#newcode35
Line 35: // We only have 2 icons, but cycle through 3 icons to test the
nit: move previous line below this comment

http://codereview.chromium.org/269079/diff/5001/6015#newcode46
Line 46: chrome.pageAction.setIcon({imageData: draw(i*2, i*4), tabId:
lastTabId});
nit: spaces around *

http://codereview.chromium.org/269079/diff/5001/6015#newcode64
Line 64: <canvas id="canvas" width="20" height="20"></canvas>
I think this should this be 19x19 (also the logic in draw())

[Matt Perry, 2009-10-14 22:44:58]

http://codereview.chromium.org/269079/diff/5001/6002
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/269079/diff/5001/6002#newcode120
Line 120: EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&tab_id));
On 2009/10/14 22:25:30, Erik Kay wrote:
> I think it would be cleaner to do this in InitCommon instead and just make
> tab_id an instance variable.

Unfortunately there isn't a common way of getting the tab_id. Sometimes it's the
only param, sometimes it's a property of the first param.

http://codereview.chromium.org/269079/diff/5001/6002#newcode146
Line 146: // setIcon can take a variant argument: either a canvas ImageData, or
an
On 2009/10/14 22:25:30, Erik Kay wrote:
> this is disconnected from the variant logic.  move it closer.

Done.

http://codereview.chromium.org/269079/diff/5001/6002#newcode217
Line 217: // TODO(mpcomplete): implement badges for page actions.
On 2009/10/14 22:25:30, Erik Kay wrote:
> This doesn't seem like the right place for the TODO.  Shouldn't it be where
page
> actions are displayed?  Looking at this code, it looks like it's implemented.

Done.

http://codereview.chromium.org/269079/diff/5001/6002#newcode238
Line 238: EXTENSION_FUNCTION_VALIDATE(color_value->GetInteger(i,
&color_array[i]));
On 2009/10/14 22:25:30, Erik Kay wrote:
> do we also need to validate the range of the color values? (same with above)

I don't think it's necessary. The validation exists to prevent against rogue
renderers. An invalid color value won't do much damage here.

http://codereview.chromium.org/269079/diff/5001/6002#newcode240
Line 240: //SkColor color = SkColorSetARGB(color_array[0], color_array[1],
color_array[2],
On 2009/10/14 22:25:30, Erik Kay wrote:
> Why is this code commented out and not the others in the other two methods? 
If
> it needs to be disabled, just remove the commented out code and leave the todo
> (or fix the 80 cols here and below)

Done.

http://codereview.chromium.org/269079/diff/5001/6002#newcode261
Line 261: // TODO(mpcomplete): implement badges for page actions.
On 2009/10/14 22:25:30, Erik Kay wrote:
> same note about TODO

Done.

http://codereview.chromium.org/269079/diff/5001/6006
File chrome/browser/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/269079/diff/5001/6006#newcode735
Line 735: // Set the image.
On 2009/10/14 22:25:30, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/269079/diff/5001/6006#newcode754
Line 754: // The pixbuf might not be loaded yet.
On 2009/10/14 22:25:30, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/269079/diff/5001/6009
File chrome/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/269079/diff/5001/6009#newcode274
Line 274: // to invalidate page actions when done.
On 2009/10/14 22:25:30, Erik Kay wrote:
> I'm probably being dense, but I don't understand the last part of this
comment. 
> It seems to imply a use-case I'm missing.

Done.

http://codereview.chromium.org/269079/diff/5001/6010
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/269079/diff/5001/6010#newcode1255
Line 1255: // Set the image.
On 2009/10/14 22:25:30, Erik Kay wrote:
> nit: newline above comment

Done.

http://codereview.chromium.org/269079/diff/5001/6010#newcode1259
Line 1259: // The image index (if not within bounds) will be set to the first
image.
On 2009/10/14 22:25:30, Erik Kay wrote:
> nit: move this comment before the previous line

Why? It's referring to the 2 lines immediately below.

http://codereview.chromium.org/269079/diff/5001/6013
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/269079/diff/5001/6013#newcode413
Line 413: static v8::Handle<v8::Value> SetBrowserActionIcon(const v8::Arguments&
args) {
On 2009/10/14 22:25:30, Erik Kay wrote:
> rename this and update the comment to indicate that it's more generic now. 
> Perhaps SetExtensionActionIcon?

Done.

http://codereview.chromium.org/269079/diff/5001/6015
File chrome/test/data/extensions/samples/test_page_action/background.html
(right):

http://codereview.chromium.org/269079/diff/5001/6015#newcode7
Line 7: //chrome.pageActions.enableForTab("", {tabId: tabId, url: p.url});
On 2009/10/14 22:25:30, Erik Kay wrote:
> remove commented out code

Done.

http://codereview.chromium.org/269079/diff/5001/6015#newcode35
Line 35: // We only have 2 icons, but cycle through 3 icons to test the
On 2009/10/14 22:25:30, Erik Kay wrote:
> nit: move previous line below this comment

Done.

http://codereview.chromium.org/269079/diff/5001/6015#newcode46
Line 46: chrome.pageAction.setIcon({imageData: draw(i*2, i*4), tabId:
lastTabId});
On 2009/10/14 22:25:30, Erik Kay wrote:
> nit: spaces around *

That would require a line wrap. I think it's more readable this way.

http://codereview.chromium.org/269079/diff/5001/6015#newcode64
Line 64: <canvas id="canvas" width="20" height="20"></canvas>
On 2009/10/14 22:25:30, Erik Kay wrote:
> I think this should this be 19x19 (also the logic in draw())

Done.

[Erik does not do reviews, 2009-10-14 23:40:46]

http://codereview.chromium.org/269079/diff/5001/6002
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/269079/diff/5001/6002#newcode120
Line 120: EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&tab_id));
On 2009/10/14 22:44:58, Matt Perry wrote:
> On 2009/10/14 22:25:30, Erik Kay wrote:
> > I think it would be cleaner to do this in InitCommon instead and just make
> > tab_id an instance variable.
> 
> Unfortunately there isn't a common way of getting the tab_id. Sometimes it's
the
> only param, sometimes it's a property of the first param.
> 

ah, bummer

http://codereview.chromium.org/269079/diff/6018/5014
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/269079/diff/6018/5014#newcode410
Line 410: // A special request for setting the browser action icon. This
function
s/browser/extension/

[Matt Perry, 2009-10-15 00:13:35]

Also adjusted the comment on GetOrCreatePageActionState

http://codereview.chromium.org/269079/diff/6018/5014
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/269079/diff/6018/5014#newcode410
Line 410: // A special request for setting the browser action icon. This
function
On 2009/10/14 23:40:47, Erik Kay wrote:
> s/browser/extension/

Done.

[Erik does not do reviews, 2009-10-15 00:20:07]

LGTM

[Aaron Boodman, 2009-10-15 06:01:21]

FYI...

estade recently refactored the badge-drawing code out (it may not be checked in
yet) so that should be easy to add in.

Also, the badge drawing doesn't support text color yet. When you add that, could
you add it to browser actions, too?

http://codereview.chromium.org/269079/diff/7005/7019
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/269079/diff/7005/7019#newcode348
Line 348: apiFunctions["pageAction.setIcon"].handleRequest = function(details) {
If bind() is defined here, you could be clever and define a setIconHandler(name,
details) { ... } and then set the handlers to bind(extensionActionHandler, null,
"browserAction.setIcon").

http://codereview.chromium.org/269079/diff/7005/7020
File chrome/test/data/extensions/samples/test_page_action/background.html
(right):

http://codereview.chromium.org/269079/diff/7005/7020#newcode66
Line 66: 
Nit: extra newline.

[Aaron Boodman, 2009-10-15 06:06:29]

Crap, one more thing. Update the static docs. Build test shell and run
src/chrome/common/extensions/build/build.bat

- a

On Wed, Oct 14, 2009 at 11:03 PM, Aaron Boodman <aa@chromium.org> wrote:
> Oh and ... tests? You can probably copy the tests I just added for
> browser actions.
>
> - a
>
> On Wed, Oct 14, 2009 at 11:01 PM, =A0<aa@chromium.org> wrote:
>> FYI...
>>
>> estade recently refactored the badge-drawing code out (it may not be che=
cked
>> in
>> yet) so that should be easy to add in.
>>
>> Also, the badge drawing doesn't support text color yet. When you add tha=
t,
>> could
>> you add it to browser actions, too?
>>
>>
>> http://codereview.chromium.org/269079/diff/7005/7019
>> File chrome/renderer/resources/extension_process_bindings.js (right):
>>
>> http://codereview.chromium.org/269079/diff/7005/7019#newcode348
>> Line 348: apiFunctions["pageAction.setIcon"].handleRequest =3D
>> function(details) {
>> If bind() is defined here, you could be clever and define a
>> setIconHandler(name, details) { ... } and then set the handlers to
>> bind(extensionActionHandler, null, "browserAction.setIcon").
>>
>> http://codereview.chromium.org/269079/diff/7005/7020
>> File
>> chrome/test/data/extensions/samples/test_page_action/background.html
>> (right):
>>
>> http://codereview.chromium.org/269079/diff/7005/7020#newcode66
>> Line 66:
>> Nit: extra newline.
>>
>> http://codereview.chromium.org/269079
>>
>

[Aaron Boodman, 2009-10-15 06:07:54]

Oh and ... tests? You can probably copy the tests I just added for
browser actions.

- a

On Wed, Oct 14, 2009 at 11:01 PM,  <aa@chromium.org> wrote:
> FYI...
>
> estade recently refactored the badge-drawing code out (it may not be checked
> in
> yet) so that should be easy to add in.
>
> Also, the badge drawing doesn't support text color yet. When you add that,
> could
> you add it to browser actions, too?
>
>
> http://codereview.chromium.org/269079/diff/7005/7019
> File chrome/renderer/resources/extension_process_bindings.js (right):
>
> http://codereview.chromium.org/269079/diff/7005/7019#newcode348
> Line 348: apiFunctions["pageAction.setIcon"].handleRequest =
> function(details) {
> If bind() is defined here, you could be clever and define a
> setIconHandler(name, details) { ... } and then set the handlers to
> bind(extensionActionHandler, null, "browserAction.setIcon").
>
> http://codereview.chromium.org/269079/diff/7005/7020
> File
> chrome/test/data/extensions/samples/test_page_action/background.html
> (right):
>
> http://codereview.chromium.org/269079/diff/7005/7020#newcode66
> Line 66:
> Nit: extra newline.
>
> http://codereview.chromium.org/269079
>

[Matt Perry, 2009-10-15 21:17:26]

Added tests, and updated the browser_action apitest to the new manifest format.