Update browserAction.setIcon and pageAction.setIcon for hidpi

Update browserAction.setIcon and pageAction.setIcon for hidpi

added support to defining icon as dictionary of paths/imageDatas with keys 19, 38.
(new optional details parameters details.imageDataSet, details.pathSet)
Dictionary values define icon's imageSkia representations for 1x and 2x scale respectively.

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

[tbarzic, 2012-08-16 21:39:27]

Please, take a look..

[Jeffrey Yasskin, 2012-08-17 23:20:28]

Aaron should review the API changes, but here's what I see.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:58:
void VerifyIconState(const Extension* extension,
Please use descriptive assertion names, so people reading the test don't always
have to come back to this definition to understand things. Sometimes you may
need to use 2 functions instead of 1 to give them each coherent meanings.

When a helper function EXPECT-fails, name it starting with "Expect".

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:74:
EXPECT_GT(action_icon_bitmap.getGenerationID(), *last_action_icon_id);
Please include a test that checks for icon equality, not just icon inequality.
BitmapsAreEqual() in ui/gfx/skia_util.h might be helpful.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:48: //
Size as a string that will be used to retrieve represantation value from
sp: represantation -> representation

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:102:
void SetDefaultsFromValue(base::DictionaryValue* dict,
Ew @ modifying the dict. I think you can avoid this by making icon_value a const
base::DictionaryValue*?

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:172:
bool UpdateIconWithRepresentation(const char* size_key,
This function doesn't seem to be pulling its weight. The behavior of both
pulling a sized image out of a dictionary and adding an ImageSkiaRep don't feel
like they logically belong together, and the function's only called once, so
you'd actually save code lines by inlining it.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_test_util.h (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util.h:41: uint32 GetIconID(int
index);
This function needs a comment.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_test_util_gtk.cc (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util_gtk.cc:57: // gfx::Image will
unref |pixbuf| when it goes away. Pair that unref here so
"will unref" is a bit lower-level than I'd expect. Perhaps, "gfx::Image takes
ownership of a reference."

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util_gtk.cc:60: return
gfx::Image(pixbuf).AsBitmap().getGenerationID();
Isn't this going to return a new ID each time it's called, rather than returning
a consistent ID for a given image?
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/src/core/...

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:424: gfx::Image
BrowserActionButton::GetIconForTest() {
Probably return the same type as icon(), since I don't see a need to cache the
other formats in tests.

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
File chrome/common/extensions/api/browser_action.json (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:26: "description": "Pixel data
for an image. Must be an ImageData object (for example, from a
<code>canvas</code> element)."
How does Canvas.getImageDataHD() fit into all of this?

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:95: "description": "Data for
icons of sizes 19 and 38. UI will decide which icon to used depending on screens
scale factor.",
"screen's"

Also, "scale factor" isn't very descriptive. Maybe "pixel density"? Is there
other documentation on this that you should be imitating?

Are 1x and 2x the only sizes external developers will need to support for a
while? I have a vague memory that Windows 8 wants 1.4 and 1.8, and it might make
sense to allude to that here, even if we're not using the other dictionary
entries.

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:118: "description": "Relative
path to an image representation in the extension to show in the browser action
at 2x scale."
"at 2x scale" won't make much sense when it shows up in
http://developer.chrome.com/extensions/browserAction.html#method-setIcon. Can
you say something about high-dpi screens here?

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/extensions/...
File chrome/renderer/extensions/set_icon_natives.cc (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/extensions/...
chrome/renderer/extensions/set_icon_natives.cc:73: *bitmap_set_value = new
DictionaryValue();
This will be a memory leak if the function returns false.

In other codebases, I'd return a scoped_ptr<DictionaryValue>, and use NULL as
the failure value. I'm not sure if that's common practice here.

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/set_icon.js (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/set_icon.js:11: console.error("Could not
load " + actionType + " icon '" +
I see "prefer single quotes" in the JS style guide:
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/set_icon.js:22: var imageData =
canvas_context.getImageData(0, 0, canvas.width,
Should this be getImageData or getImageDataHD? Are there any cases where drawing
an image onto a canvas of the same logical size will cause scaling in both
directions, which could cause display artifacts?

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/set_icon.js:63: } else if ("imageData" in
details) {
Does it make sense to detect the type of the 'imageData' argument, and do the
right thing depending on whether it's an ImageData or a dictionary of
size->ImageData?

http://codereview.chromium.org/10855154/diff/4011/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/browser_action/no_icon/background.html
(right):

http://codereview.chromium.org/10855154/diff/4011/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/browser_action/no_icon/background.html:10:
<canvas id="canvas" width="37" height="33">
Why these dimensions?

[tbarzic, 2012-08-21 00:22:07]

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:58:
void VerifyIconState(const Extension* extension,
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Please use descriptive assertion names, so people reading the test don't
always
> have to come back to this definition to understand things. Sometimes you may
> need to use 2 functions instead of 1 to give them each coherent meanings.
> 
> When a helper function EXPECT-fails, name it starting with "Expect".

ok, got rid of this function..

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:74:
EXPECT_GT(action_icon_bitmap.getGenerationID(), *last_action_icon_id);
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Please include a test that checks for icon equality, not just icon inequality.
> BitmapsAreEqual() in ui/gfx/skia_util.h might be helpful.

yeah, I've stumbled upon that function on Friday, but I haven't had time to
changed the test ;)

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:48: //
Size as a string that will be used to retrieve represantation value from
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> sp: represantation -> representation

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:102:
void SetDefaultsFromValue(base::DictionaryValue* dict,
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Ew @ modifying the dict. I think you can avoid this by making icon_value a
const
> base::DictionaryValue*?

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:172:
bool UpdateIconWithRepresentation(const char* size_key,
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> This function doesn't seem to be pulling its weight. The behavior of both
> pulling a sized image out of a dictionary and adding an ImageSkiaRep don't
feel
> like they logically belong together, and the function's only called once, so
> you'd actually save code lines by inlining it.

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_test_util.h (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util.h:41: uint32 GetIconID(int
index);
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> This function needs a comment.

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_action_test_util_gtk.cc (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util_gtk.cc:57: // gfx::Image will
unref |pixbuf| when it goes away. Pair that unref here so
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> "will unref" is a bit lower-level than I'd expect. Perhaps, "gfx::Image takes
> ownership of a reference."

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/extensions/b...
chrome/browser/extensions/browser_action_test_util_gtk.cc:60: return
gfx::Image(pixbuf).AsBitmap().getGenerationID();
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Isn't this going to return a new ID each time it's called, rather than
returning
> a consistent ID for a given image?
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/src/core/...

Yeah, you're probably right, but doesn't matter after changes to the test..

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_action_view.cc:424: gfx::Image
BrowserActionButton::GetIconForTest() {
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Probably return the same type as icon(), since I don't see a need to cache the
> other formats in tests.

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
File chrome/common/extensions/api/browser_action.json (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:95: "description": "Data for
icons of sizes 19 and 38. UI will decide which icon to used depending on screens
scale factor.",
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> "screen's"
> 
> Also, "scale factor" isn't very descriptive. Maybe "pixel density"? Is there
> other documentation on this that you should be imitating?
> 
> Are 1x and 2x the only sizes external developers will need to support for a
> while? I have a vague memory that Windows 8 wants 1.4 and 1.8, and it might
make
> sense to allude to that here, even if we're not using the other dictionary
> entries.

I guess this is a bit better, but I feel there's still room for improvement..

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:118: "description": "Relative
path to an image representation in the extension to show in the browser action
at 2x scale."
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> "at 2x scale" won't make much sense when it shows up in
> http://developer.chrome.com/extensions/browserAction.html#method-setIcon. Can
> you say something about high-dpi screens here?

removed description from here.

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/extensions/...
File chrome/renderer/extensions/set_icon_natives.cc (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/extensions/...
chrome/renderer/extensions/set_icon_natives.cc:73: *bitmap_set_value = new
DictionaryValue();
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> This will be a memory leak if the function returns false.
> 
> In other codebases, I'd return a scoped_ptr<DictionaryValue>, and use NULL as
> the failure value. I'm not sure if that's common practice here.

Done.

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/set_icon.js (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/set_icon.js:11: console.error("Could not
load " + actionType + " icon '" +
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> I see "prefer single quotes" in the JS style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...

yeah, I was a bit too lazy to change those..

http://codereview.chromium.org/10855154/diff/4011/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/set_icon.js:63: } else if ("imageData" in
details) {
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Does it make sense to detect the type of the 'imageData' argument, and do the
> right thing depending on whether it's an ImageData or a dictionary of
> size->ImageData?

yeah, it would.. I just had to remember how to declare this in api json
definition :)

http://codereview.chromium.org/10855154/diff/4011/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/browser_action/no_icon/background.html
(right):

http://codereview.chromium.org/10855154/diff/4011/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/browser_action/no_icon/background.html:10:
<canvas id="canvas" width="37" height="33">
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> Why these dimensions?

no particular reason, they're pretty random

[tbarzic, 2012-08-21 00:24:24]

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
File chrome/common/extensions/api/browser_action.json (right):

http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:26: "description": "Pixel data
for an image. Must be an ImageData object (for example, from a
<code>canvas</code> element)."
On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> How does Canvas.getImageDataHD() fit into all of this?

I don't think we support Canvas.getImageDataHD (see
http://code.google.com/p/chromium/issues/detail?id=127852#c29)

+rbyers in case I'm missing something

[Rick Byers, 2012-08-21 01:13:06]

On 2012/08/21 00:24:24, tbarzic wrote:
>
http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
> File chrome/common/extensions/api/browser_action.json (right):
> 
>
http://codereview.chromium.org/10855154/diff/4011/chrome/common/extensions/ap...
> chrome/common/extensions/api/browser_action.json:26: "description": "Pixel
data
> for an image. Must be an ImageData object (for example, from a
> <code>canvas</code> element)."
> On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> > How does Canvas.getImageDataHD() fit into all of this?
> 
> I don't think we support Canvas.getImageDataHD (see
> http://code.google.com/p/chromium/issues/detail?id=127852#c29)
> 
> +rbyers in case I'm missing something

Yep, we think it's better to require apps to opt into to a high density backing
store explicitly rather than automatically give it one transparently - so
there's no need for separate 'HD' versions of the backing store APIs. 
senorblanco@  is the expert here though if you need details.

[Jeffrey Yasskin, 2012-08-21 20:10:38]

http://codereview.chromium.org/10855154/diff/7041/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

http://codereview.chromium.org/10855154/diff/7041/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:133:
uint32_t action_icon_last_id = action_icon.AsBitmap().getGenerationID();
It might be nice to have one sanity-check here, of the form:
EXPECT_EQ(action_icon_last_id,
          extension->browser_action()->GetIcon(0).
          ToBitmap()->getGenerationID());
Otherwise it's possible that GetIcon returns an entirely new bitmap, which would
cause your assertions below to fail to test anything.

I'm suggesting ToBitmap because of the discussion at http://crbug.com/142930,
and because I don't think you need AsBitmap()'s empty behavior in this case.

http://codereview.chromium.org/10855154/diff/7041/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10855154/diff/7041/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:455:
icon.AddRepresentation(gfx::ImageSkiaRep(bitmap,kIconSizes[i].scale));
Nit: Include a space after commas.

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
File chrome/common/extensions/api/browser_action.json (right):

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:26: "description": "Pixel data
for an image. Must be an ImageData object (for example, from a
<code>canvas</code> element)."
On 2012/08/21 00:24:24, tbarzic wrote:
> On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> > How does Canvas.getImageDataHD() fit into all of this?
> 
> I don't think we support Canvas.getImageDataHD (see
> http://code.google.com/p/chromium/issues/detail?id=127852#c29)
> 
> +rbyers in case I'm missing something

Ok, we should document that here. Would you like to do that in this CL, or
should I try it in a subsequent one?

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:100: "description": "Either
ImageData object or dictionary {size -> ImageData} representing icon to be set.
If the icon is specified as a dictionary, the actual image to be used is chosen
depending on screen's pixel density. If number of image pixels that fit into one
screen space unit equals scale, then image with size scale * 19 will be
selected. Initially only scales 1 and 2 will be supported. At least one image
should be specified. Note that 'details.imageData = foo' is equivalent to
'details.imageData = {'19': foo}'",
Great comment!

Nits: It would be nice to call out 'scale' as a variable. I think these sections
support html, so surrounding it with <code> or <code><var> should work.

"Either an ImageData object or a dictionary {size -> ImageData}"

s/If number/If the number/

s/should/must/?

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
chrome/common/extensions/api/browser_action.json:114: "description": "Either
relative image path or dictionary {size -> relative image path} pointing to icon
to be set. If the icon is specified as a dictionary, the actual image to be used
is chosen depending on screen's pixel density. If number of image pixels that
fit into one screen space unit equals scale, then image with size scale * 19
will be selected. Initially only scales 1 and 2 will be supported. At least one
image should be specified. Note that 'details.path = foo' is equivalent to
'details.imageData = {'19': foo}'",
Same nits as above.

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
File chrome/common/extensions/api/page_action.json (right):

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
chrome/common/extensions/api/page_action.json:80: "description": "Sets the icon
for the page action. The icon can be specified either as the path to an image
file or as the pixel data from a canvas element, or as dictionary of either one
of those. Either the <b>path</b>, the <b>imageData</b>, or
<b>iconIndex</b>property must be specified.",
I believe it was intentional to omit iconIndex from this documentation, since
it's a deprecated interface.

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
chrome/common/extensions/api/page_action.json:98: "description": "Either
ImageData object or dictionary {size -> ImageData} representing icon to be set.
If the icon is specified as a dictionary, the actual image to be used is chosen
depending on screen's pixel density. If number of image pixels that fit into one
screen space unit equals scale, then image with size scale * 19 will be
selected. Initially only scales 1 and 2 will be supported. At least one image
should be specified. Note that 'details.imageData = foo' is equivalent to
'details.imageData = {'19': foo}'",
Ditto from browser_action.json.

http://codereview.chromium.org/10855154/diff/7041/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/browser_action/no_icon/background.js
(right):

http://codereview.chromium.org/10855154/diff/7041/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/browser_action/no_icon/background.js:15:
{imageData: {'19': canvas}},
Would it be a good test to check only setting a 2x icon? There's probably no
need to do it for both imageData and path; your other tests cover that
conversion.

Another possible test would be passing an empty dictionary to imageData or path
and checking that it throws a reasonable exception.

[tbarzic, 2012-08-22 00:12:48]

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/browser/exte...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:133:
uint32_t action_icon_last_id = action_icon.AsBitmap().getGenerationID();
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> It might be nice to have one sanity-check here, of the form:
> EXPECT_EQ(action_icon_last_id,
>           extension->browser_action()->GetIcon(0).
>           ToBitmap()->getGenerationID());
> Otherwise it's possible that GetIcon returns an entirely new bitmap, which
would
> cause your assertions below to fail to test anything.
> 
> I'm suggesting ToBitmap because of the discussion at http://crbug.com/142930,
> and because I don't think you need AsBitmap()'s empty behavior in this case.

Done.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/browser/exte...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:455:
icon.AddRepresentation(gfx::ImageSkiaRep(bitmap,kIconSizes[i].scale));
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> Nit: Include a space after commas.

Done.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
File chrome/common/extensions/api/browser_action.json (right):

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
chrome/common/extensions/api/browser_action.json:26: "description": "Pixel data
for an image. Must be an ImageData object (for example, from a
<code>canvas</code> element)."
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> On 2012/08/21 00:24:24, tbarzic wrote:
> > On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> > > How does Canvas.getImageDataHD() fit into all of this?
> > 
> > I don't think we support Canvas.getImageDataHD (see
> > http://code.google.com/p/chromium/issues/detail?id=127852#c29)
> > 
> > +rbyers in case I'm missing something
> 
> Ok, we should document that here. Would you like to do that in this CL, or
> should I try it in a subsequent one?

ok, I'll add something tomorrow.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
chrome/common/extensions/api/browser_action.json:100: "description": "Either
ImageData object or dictionary {size -> ImageData} representing icon to be set.
If the icon is specified as a dictionary, the actual image to be used is chosen
depending on screen's pixel density. If number of image pixels that fit into one
screen space unit equals scale, then image with size scale * 19 will be
selected. Initially only scales 1 and 2 will be supported. At least one image
should be specified. Note that 'details.imageData = foo' is equivalent to
'details.imageData = {'19': foo}'",
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> Great comment!
> 
> Nits: It would be nice to call out 'scale' as a variable. I think these
sections
> support html, so surrounding it with <code> or <code><var> should work.
> 
> "Either an ImageData object or a dictionary {size -> ImageData}"
> 
> s/If number/If the number/
> 
> s/should/must/?

Done.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
chrome/common/extensions/api/browser_action.json:114: "description": "Either
relative image path or dictionary {size -> relative image path} pointing to icon
to be set. If the icon is specified as a dictionary, the actual image to be used
is chosen depending on screen's pixel density. If number of image pixels that
fit into one screen space unit equals scale, then image with size scale * 19
will be selected. Initially only scales 1 and 2 will be supported. At least one
image should be specified. Note that 'details.path = foo' is equivalent to
'details.imageData = {'19': foo}'",
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> Same nits as above.

Done.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
File chrome/common/extensions/api/page_action.json (right):

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
chrome/common/extensions/api/page_action.json:80: "description": "Sets the icon
for the page action. The icon can be specified either as the path to an image
file or as the pixel data from a canvas element, or as dictionary of either one
of those. Either the <b>path</b>, the <b>imageData</b>, or
<b>iconIndex</b>property must be specified.",
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> I believe it was intentional to omit iconIndex from this documentation, since
> it's a deprecated interface.

Done.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
chrome/common/extensions/api/page_action.json:98: "description": "Either
ImageData object or dictionary {size -> ImageData} representing icon to be set.
If the icon is specified as a dictionary, the actual image to be used is chosen
depending on screen's pixel density. If number of image pixels that fit into one
screen space unit equals scale, then image with size scale * 19 will be
selected. Initially only scales 1 and 2 will be supported. At least one image
should be specified. Note that 'details.imageData = foo' is equivalent to
'details.imageData = {'19': foo}'",
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> Ditto from browser_action.json.

Done.

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/test/data/ex...
File chrome/test/data/extensions/api_test/browser_action/no_icon/background.js
(right):

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/test/data/ex...
chrome/test/data/extensions/api_test/browser_action/no_icon/background.js:15:
{imageData: {'19': canvas}},
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> Would it be a good test to check only setting a 2x icon? There's probably no
> need to do it for both imageData and path; your other tests cover that
> conversion.
> 
> Another possible test would be passing an empty dictionary to imageData or
path
> and checking that it throws a reasonable exception.

Done.

[Jeffrey Yasskin, 2012-08-22 04:42:21]

LGTM, but I think you'll still need an owner's approval. Thanks!

https://chromiumcodereview.appspot.com/10855154/diff/17001/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://chromiumcodereview.appspot.com/10855154/diff/17001/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:41:
"The imageData property must contain an ImageData object or dictionary "
I would usually stick the expected error message directly in the assertion, but
it doesn't matter much.

https://chromiumcodereview.appspot.com/10855154/diff/17001/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:140: //
Let's check that |GetIcon| doesn't always return bitmap wiht new id.
s/wiht/with/

[tbarzic, 2012-08-22 18:43:16]

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
File chrome/common/extensions/api/browser_action.json (right):

https://chromiumcodereview.appspot.com/10855154/diff/7041/chrome/common/exten...
chrome/common/extensions/api/browser_action.json:26: "description": "Pixel data
for an image. Must be an ImageData object (for example, from a
<code>canvas</code> element)."
On 2012/08/22 00:12:49, tbarzic wrote:
> On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> > On 2012/08/21 00:24:24, tbarzic wrote:
> > > On 2012/08/17 23:20:28, Jeffrey Yasskin wrote:
> > > > How does Canvas.getImageDataHD() fit into all of this?
> > > 
> > > I don't think we support Canvas.getImageDataHD (see
> > > http://code.google.com/p/chromium/issues/detail?id=127852#c29)
> > > 
> > > +rbyers in case I'm missing something
> > 
> > Ok, we should document that here. Would you like to do that in this CL, or
> > should I try it in a subsequent one?
> 
> ok, I'll add something tomorrow.

On second thought, I'm not too sure this is the best place to document that we
don't support canvas.getImageDataHD.
We shouldn't really care how we get the ImageData object..

https://chromiumcodereview.appspot.com/10855154/diff/17001/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://chromiumcodereview.appspot.com/10855154/diff/17001/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:140: //
Let's check that |GetIcon| doesn't always return bitmap wiht new id.
On 2012/08/22 04:42:21, Jeffrey Yasskin wrote:
> s/wiht/with/

Done.

[Aaron Boodman, 2012-08-22 20:10:31]

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
File chrome/common/extensions/api/page_action.json (right):

http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
chrome/common/extensions/api/page_action.json:80: "description": "Sets the icon
for the page action. The icon can be specified either as the path to an image
file or as the pixel data from a canvas element, or as dictionary of either one
of those. Either the <b>path</b>, the <b>imageData</b>, or
<b>iconIndex</b>property must be specified.",
On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> I believe it was intentional to omit iconIndex from this documentation, since
> it's a deprecated interface.

Yep.

[tbarzic, 2012-08-28 17:02:02]

On 2012/08/22 20:10:31, Aaron Boodman wrote:
>
http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
> File chrome/common/extensions/api/page_action.json (right):
> 
>
http://codereview.chromium.org/10855154/diff/7041/chrome/common/extensions/ap...
> chrome/common/extensions/api/page_action.json:80: "description": "Sets the
icon
> for the page action. The icon can be specified either as the path to an image
> file or as the pixel data from a canvas element, or as dictionary of either
one
> of those. Either the <b>path</b>, the <b>imageData</b>, or
> <b>iconIndex</b>property must be specified.",
> On 2012/08/21 20:10:38, Jeffrey Yasskin wrote:
> > I believe it was intentional to omit iconIndex from this documentation,
since
> > it's a deprecated interface.
> 
> Yep.

Aaron: ping

[Aaron Boodman, 2012-08-30 04:35:07]

Sorry, LGTM

[tbarzic, 2012-08-31 18:45:38]

sky: Can you take a look at chrome/browser/ui/*

[sky, 2012-08-31 19:35:12]

LGTM with the following change.

http://codereview.chromium.org/10855154/diff/31002/chrome/browser/ui/views/br...
File chrome/browser/ui/views/browser_action_view.cc (right):

http://codereview.chromium.org/10855154/diff/31002/chrome/browser/ui/views/br...
chrome/browser/ui/views/browser_action_view.cc:417: gfx::ImageSkia
BrowserActionButton::GetIconForTest() {
Make position match header.

[commit-bot: I haz the power, 2012-09-10 20:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10855154/43031

[commit-bot: I haz the power, 2012-09-10 23:02:04]

Change committed as 155856