Change browser/page action default icon defined in manifest to support hidpi.

Change browser/page action default icon defined in manifest to support hidpi.

To support hidpi for browser action default icons, ability to define dictionary
of icon in manifest as default icons is added.

Defining images of sizes 19 and 38 is allowed (other dictionary values will be
ignored). The image to be painted will be determined based on screen density.

Similary, for script badges, default icon is determined using 16 and 32 px icon
defined in extension manifest.

I have extracted actual icon loading code to extension_action_icon_factory.h/.cc, so it doesn't have to be implemented for all platform specific solutions.

BUG=138025, 135271
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157309

[tbarzic, 2012-08-29 22:01:28]

Guys, can you please take a look..

jyasskin, kalman(I need an owner) : */extensions
sky: browser/ui
pkotwicz: ui/gfx
rsesek: browser/ui/cocoa

http://codereview.chromium.org/10905005/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10905005/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:129:
dict->SetString(kIconStorageKey,
this is not really intended behaviour (it will save extension favicon instead of
default icon), but, I'm fixing this in another cl (which is under review)

[pkotwicz, 2012-08-29 22:07:42]

http://codereview.chromium.org/10905005/diff/2001/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.cc (left):

http://codereview.chromium.org/10905005/diff/2001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:136:
static_cast<int>(image_.size().width() * scale),
Can't you use ImageSkiaRep::GetWidth() and ImageSkiaRep::GetHeight() instead?

[tbarzic, 2012-08-29 22:18:12]

http://codereview.chromium.org/10905005/diff/2001/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.cc (left):

http://codereview.chromium.org/10905005/diff/2001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:136:
static_cast<int>(image_.size().width() * scale),
On 2012/08/29 22:07:43, pkotwicz wrote:
> Can't you use ImageSkiaRep::GetWidth() and ImageSkiaRep::GetHeight() instead?

Won't that return rep's size in dip? I need actual bitmap size here..

I can lose static_cast though

[pkotwicz, 2012-08-29 23:14:47]

http://codereview.chromium.org/10905005/diff/2001/ui/gfx/image/image_skia_ope...
File ui/gfx/image/image_skia_operations.cc (left):

http://codereview.chromium.org/10905005/diff/2001/ui/gfx/image/image_skia_ope...
ui/gfx/image/image_skia_operations.cc:136:
static_cast<int>(image_.size().width() * scale),
Sorry. My bad. You can use ImageSkiaRep::pixel_width() and
ImageSkiaRep::pixel_height() to get the actual bitmap size.

[tbarzic, 2012-08-29 23:25:56]

https://chromiumcodereview.appspot.com/10905005/diff/2001/ui/gfx/image/image_...
File ui/gfx/image/image_skia_operations.cc (left):

https://chromiumcodereview.appspot.com/10905005/diff/2001/ui/gfx/image/image_...
ui/gfx/image/image_skia_operations.cc:136:
static_cast<int>(image_.size().width() * scale),
On 2012/08/29 23:14:47, pkotwicz wrote:
> Sorry. My bad. You can use ImageSkiaRep::pixel_width() and
> ImageSkiaRep::pixel_height() to get the actual bitmap size.

yeah, now I feel silly for not noticing those two myself..

[tbarzic, 2012-08-30 18:06:44]

http://codereview.chromium.org/10905005/diff/9009/chrome/common/extensions/ex...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10905005/diff/9009/chrome/common/extensions/ex...
chrome/common/extensions/extension.cc:877: if
(extension_action->HasKey(keys::kPageActionDefaultIconSet)) {
I'm not too content with the name here ("default_icon_set").
Do you have any other suggestions?

What do you think of "default_icon" (same as now, when default icon is provided
by path to 19px icon), but allowing the property to be either string (to keep
back compatibility), in which case we assume it represents a path to 19px icon,
or a dictionary introduced here?

[Robert Sesek, 2012-08-30 19:08:21]

cocoa/ LGTM

http://codereview.chromium.org/10905005/diff/9009/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10905005/diff/9009/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:61:
~ExtensionActionIconFactoryBridge() {}
Should be virtual (yikes that it wasn't before!)

http://codereview.chromium.org/10905005/diff/9009/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:78:
ExtensionActionIconFactory* icon_factory() { return &icon_factory_; }
const method

[tbarzic, 2012-08-30 20:48:39]

http://codereview.chromium.org/10905005/diff/9009/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

http://codereview.chromium.org/10905005/diff/9009/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:61:
~ExtensionActionIconFactoryBridge() {}
On 2012/08/30 19:08:21, rsesek wrote:
> Should be virtual (yikes that it wasn't before!)

Done.

http://codereview.chromium.org/10905005/diff/9009/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:78:
ExtensionActionIconFactory* icon_factory() { return &icon_factory_; }
On 2012/08/30 19:08:21, rsesek wrote:
> const method

the method is not really const (since it returns non const ptr to member var).
but, giving it another thought, we should probably not expose icon_factory_..
ptal

[Robert Sesek, 2012-08-30 20:51:16]

LGTM

[Jeffrey Yasskin, 2012-09-01 03:54:33]

A few initial thoughts:

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:34: }  // namespace
.cc-local classes should go in anonymous namespaces, and it looks like
ExtensionActionIcon is local to this .cc file.

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:36: //
extension::IconImage wrapper for extension action icons. We need this
Why is this a separate class from the browser-level IconFactory implementation?
Can you simplify things by bundling them together?

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.h (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.h:17: class
ExtensionActionIconFactory : public ExtensionAction::IconFactory {
Comment what this is for.

Also, terribly confusing name: Don't just remove a :: token from a class and
implement the concatenation as a different class.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:136: bool LoadIconsFromDictionary(const
DictionaryValue* icons_value,
Comment what meaning you intend this function to have.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:153: icon_path = icon_path.substr(1);
This is odd. What are you trying to do? Should you be using a FilePath instead?

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:877: if
(extension_action->HasKey(keys::kPageActionDefaultIconSet)) {
On 2012/08/30 18:06:45, tbarzic wrote:
> I'm not too content with the name here ("default_icon_set").
> Do you have any other suggestions?
> 
> What do you think of "default_icon" (same as now, when default icon is
provided
> by path to 19px icon), but allowing the property to be either string (to keep
> back compatibility), in which case we assume it represents a path to 19px
icon,
> or a dictionary introduced here?

Right, I think detecting the type of "default_icon"'s value makes sense for the
same reasons as in your earlier CL.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:61: // Since ExtensionAction, living
in common/, can't interact with the browser
I skeched
https://chromiumcodereview.appspot.com/10833069/diff/1/chrome/common/extensio...
to do this sort of thing. It doesn't really seem like something that should be
embedded into ExtensionAction, although making it specific to images could make
sense.

I believe aa and kalman were concerned about how it would be implemented in
non-browser processes that can't necessarily load images.

Are these layer-reversing interfaces normally named with a "Delegate" suffix?

s/the UI code/the browser code/, right? This won't get implemented separately
for win/mac/linux, right?

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:64: // Whenever icon to return is
defined by its path (stored in ExtensionIconSet)
I don't understand this sentence, or how an ExtensionIconSet and action_type map
to a returned icon.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:204: void set_default_icon(const
ExtensionIconSet* icon_set) {
When you take ownership of a passed pointer, take it as a scoped_ptr<> to
communicate that.

[tbarzic, 2012-09-05 01:05:28]

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:34: }  // namespace
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> .cc-local classes should go in anonymous namespaces, and it looks like
> ExtensionActionIcon is local to this .cc file.

Done.

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:36: //
extension::IconImage wrapper for extension action icons. We need this
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> Why is this a separate class from the browser-level IconFactory
implementation?
> Can you simplify things by bundling them together?

Done.

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.h (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.h:17: class
ExtensionActionIconFactory : public ExtensionAction::IconFactory {
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> Comment what this is for.
> 
> Also, terribly confusing name: Don't just remove a :: token from a class and
> implement the concatenation as a different class.

Done.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:136: bool LoadIconsFromDictionary(const
DictionaryValue* icons_value,
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> Comment what meaning you intend this function to have.

Done.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:153: icon_path = icon_path.substr(1);
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> This is odd. What are you trying to do? Should you be using a FilePath
instead?

this isn't my code (I just moved it here), but it looks like it ensures the
passed path is not an absolute path..
I'm not sure using FilePath here would make it simpler..

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:877: if
(extension_action->HasKey(keys::kPageActionDefaultIconSet)) {
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> On 2012/08/30 18:06:45, tbarzic wrote:
> > I'm not too content with the name here ("default_icon_set").
> > Do you have any other suggestions?
> > 
> > What do you think of "default_icon" (same as now, when default icon is
> provided
> > by path to 19px icon), but allowing the property to be either string (to
keep
> > back compatibility), in which case we assume it represents a path to 19px
> icon,
> > or a dictionary introduced here?
> 
> Right, I think detecting the type of "default_icon"'s value makes sense for
the
> same reasons as in your earlier CL.

Done.

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:61: // Since ExtensionAction, living
in common/, can't interact with the browser
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> I skeched
>
https://chromiumcodereview.appspot.com/10833069/diff/1/chrome/common/extensio...
> to do this sort of thing. It doesn't really seem like something that should be
> embedded into ExtensionAction, although making it specific to images could
make
> sense.
>

OK, I've moved this interface out of ExtensionAction.
 
> I believe aa and kalman were concerned about how it would be implemented in
> non-browser processes that can't necessarily load images.
> 
> Are these layer-reversing interfaces normally named with a "Delegate" suffix?

Yes, they are.. 

> 
> s/the UI code/the browser code/, right? This won't get implemented separately
> for win/mac/linux, right?

I had put "UI code" since the object is owned (and passed to |GetIcon|) by UI
code (even though the implementation is common and in chrome/browser).

http://codereview.chromium.org/10905005/diff/18001/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:204: void set_default_icon(const
ExtensionIconSet* icon_set) {
On 2012/09/01 03:54:33, Jeffrey Yasskin wrote:
> When you take ownership of a passed pointer, take it as a scoped_ptr<> to
> communicate that.

Done.

[tbarzic, 2012-09-07 20:41:33]

ping...

[sky, 2012-09-07 21:49:09]

views LGTM, you need to get separate owners for gtk and cocoa

[Jeffrey Yasskin, 2012-09-13 00:23:35]

+mpcomplete to comment on the elimination of ExtensionAction:: icon_paths().

I suspect we won't get this whole series of patches done by Monday for the
Chrome 23 branch.

http://codereview.chromium.org/10905005/diff/20090/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

http://codereview.chromium.org/10905005/diff/20090/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:49:
DCHECK_EQ(last_size_, desired_size);
I don't see a reason to DCHECK that the sizes match when you could just handle
it by re-computing the icon when the size changes.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:2341:
script_badge_->set_default_icon(scoped_ptr<ExtensionIconSet>().Pass());
You shouldn't need .Pass() on this line. Temporary scoped_ptr<>s can be passed
directly to scoped_ptr<> arguments.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:65: void CopyExtensionIconSet(const
ExtensionIconSet* from, ExtensionIconSet* to) {
If we want this function, I suspect ExtensionIconSet should just grow a copy
constructor and assignment operator.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:86: gfx::ImageSkia
GetIconFromIconSet(const ExtensionIconSet* icon_set,
This function doesn't seem to pull its weight. I'd inline it.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:382: // Remeber icon width so it
can be used for badge painting.
This doesn't look like the width being rembered.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:435: int ExtensionAction::
GetIconWidth(int tab_id) const {
No space between :: and the method name.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:441: // the icon width will be set
depending on our action type.
typo: "the icon the icon width"

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:139: const std::vector<const
ExtensionIconSet*>& page_action_icons() const {
I think mpcomplete is removing the icon_paths() API entirely, so it may not be
worth refactoring it. I'm not sure of the timeline.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:333: // ExtensionIconSet with icon
image representations for default icon.
ExtensionIconSet doesn't hold icon image representations: it holds paths.

Does it actually make sense to overload ExtensionIconSet between holding paths
for several different size icons at the same scale vs. paths for several
different scale icons at the same size? I'm getting confused following these
changes because I'm not sure which meaning you're using in each place, and I'm
worried that's just going to get worse over time.

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
File chrome/common/extensions/extension_icon_factory_delegate.h (right):

http://codereview.chromium.org/10905005/diff/20090/chrome/common/extensions/e...
chrome/common/extensions/extension_icon_factory_delegate.h:23: int desired_size)
= 0;
Still trying to get my head around the meaning of this class ... I think what
this does is provide a way to load an icon of a given
gfx::Size-in-device-independent-pixels, given a set of icon paths for various
scales. Is that correct? If so, please include that in the comments. I think it
could make sense to pass gfx::Size instead of an int, even though all extension
icons are currently square.

Since these are paths and not pre-loaded images, any use of this class will
require an observer, but the observer is hidden away in browser/ with no
reference from here. It would be nice to make the returned object observable
directly, but if that's infeasible, I'd like some place to document the overall
arrangement of objects.

[tbarzic, 2012-09-13 02:01:01]

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/browser/ext...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/browser/ext...
chrome/browser/extensions/extension_action_icon_factory.cc:49:
DCHECK_EQ(last_size_, desired_size);
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> I don't see a reason to DCHECK that the sizes match when you could just handle
> it by re-computing the icon when the size changes.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension.cc:2341:
script_badge_->set_default_icon(scoped_ptr<ExtensionIconSet>().Pass());
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> You shouldn't need .Pass() on this line. Temporary scoped_ptr<>s can be passed
> directly to scoped_ptr<> arguments.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.cc:65: void CopyExtensionIconSet(const
ExtensionIconSet* from, ExtensionIconSet* to) {
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> If we want this function, I suspect ExtensionIconSet should just grow a copy
> constructor and assignment operator.

I don't think we want this functionality in production code (this is used only
in test).

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.cc:86: gfx::ImageSkia
GetIconFromIconSet(const ExtensionIconSet* icon_set,
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> This function doesn't seem to pull its weight. I'd inline it.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.cc:382: // Remeber icon width so it
can be used for badge painting.
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> This doesn't look like the width being rembered.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.cc:435: int ExtensionAction::
GetIconWidth(int tab_id) const {
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> No space between :: and the method name.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.cc:441: // the icon width will be set
depending on our action type.
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> typo: "the icon the icon width"

Done.

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.h:333: // ExtensionIconSet with icon
image representations for default icon.
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> ExtensionIconSet doesn't hold icon image representations: it holds paths.
> 
> Does it actually make sense to overload ExtensionIconSet between holding paths
> for several different size icons at the same scale vs. paths for several
> different scale icons at the same size? I'm getting confused following these
> changes because I'm not sure which meaning you're using in each place, and I'm
> worried that's just going to get worse over time.

I look at ExtensionIconSet as not aware of scale factors. (i.e. it's just a set
of bitmaps of different sizes, and we select image representation bitmaps from
that set).

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
File chrome/common/extensions/extension_icon_factory_delegate.h (right):

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_icon_factory_delegate.h:23: int desired_size)
= 0;
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> Still trying to get my head around the meaning of this class ... I think what
> this does is provide a way to load an icon of a given
> gfx::Size-in-device-independent-pixels, given a set of icon paths for various
> scales. Is that correct? If so, please include that in the comments. I think
it
> could make sense to pass gfx::Size instead of an int, even though all
extension
> icons are currently square.
> 
> Since these are paths and not pre-loaded images, any use of this class will
> require an observer, but the observer is hidden away in browser/ with no
> reference from here. It would be nice to make the returned object observable
> directly, but if that's infeasible, I'd like some place to document the
overall
> arrangement of objects.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/30003/chrome/common/exte...
File chrome/common/extensions/extension_icon_factory_delegate.h (right):

https://chromiumcodereview.appspot.com/10905005/diff/30003/chrome/common/exte...
chrome/common/extensions/extension_icon_factory_delegate.h:23: // image
representations are selected fromi the set of bitmaps whose paths are
s/fromi/from

[tbarzic, 2012-09-13 17:31:49]

+erg@ for chrome/browser/ui/gtk/

[tbarzic, 2012-09-13 17:57:53]

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10905005/diff/20090/chrome/common/exte...
chrome/common/extensions/extension_action.h:139: const std::vector<const
ExtensionIconSet*>& page_action_icons() const {
On 2012/09/13 00:23:36, Jeffrey Yasskin wrote:
> I think mpcomplete is removing the icon_paths() API entirely, so it may not be
> worth refactoring it. I'm not sure of the timeline.

fwiw, this change removes page action icons loading from ui code, so I guess
it's a good step in "removing the icon_paths() API" direction too :)

[Elliot Glaysher, 2012-09-13 18:08:18]

+estade, who is more familiar with the browser actions gtk code than I am.

[Jeffrey Yasskin, 2012-09-13 19:00:55]

I talked to Aaron about this a bit, and we think a lot of the
delegate/factory/observer complexity might be removed if we move ExtensionAction
from common/ to browser/. I'm writing a change to see if that's even possible
before we say that's definitely the way to do this.

I'll also look at http://crrev.com/10915251/ while that's compiling.

[tbarzic, 2012-09-13 19:14:27]

On 2012/09/13 19:00:55, Jeffrey Yasskin wrote:
> I talked to Aaron about this a bit, and we think a lot of the
> delegate/factory/observer complexity might be removed if we move
ExtensionAction
> from common/ to browser/. I'm writing a change to see if that's even possible
> before we say that's definitely the way to do this.
> 
> I'll also look at http://crrev.com/10915251/ while that's compiling.

Doesn't Extension have dependency on ExtensionAction?
moving ExtensionAction to browser could result in more complexity in extension
creation code..

[Matt Perry, 2012-09-13 23:52:58]

On 2012/09/13 19:14:27, tbarzic wrote:
> On 2012/09/13 19:00:55, Jeffrey Yasskin wrote:
> > I talked to Aaron about this a bit, and we think a lot of the
> > delegate/factory/observer complexity might be removed if we move
> ExtensionAction
> > from common/ to browser/. I'm writing a change to see if that's even
possible
> > before we say that's definitely the way to do this.
> > 
> > I'll also look at http://crrev.com/10915251/ while that's compiling.
> 
> Doesn't Extension have dependency on ExtensionAction?
> moving ExtensionAction to browser could result in more complexity in extension
> creation code..

I just removed ExtensionAction::icon_paths (
http://code.google.com/p/chromium/issues/detail?id=148602 ), which may simplify
this CL.

[Evan Stade, 2012-09-14 09:36:21]

gtk lgtm with nits

http://codereview.chromium.org/10905005/diff/19056/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10905005/diff/19056/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:104:
ALLOW_THIS_IN_INITIALIZER_LIST(icon_factory_(extension, this)),
you only need ALLOW_THIS... for windows

http://codereview.chromium.org/10905005/diff/19056/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:444: // The object that
browser action will use to create icon for us.
singular "icon" needs an article ("the"/"a")

http://codereview.chromium.org/10905005/diff/19056/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:446: // the action will be
blank), so we have to observe it for icon's updates.
"for updates to the icon"

http://codereview.chromium.org/10905005/diff/19056/chrome/browser/ui/gtk/loca...
File chrome/browser/ui/gtk/location_bar_view_gtk.h (right):

http://codereview.chromium.org/10905005/diff/19056/chrome/browser/ui/gtk/loca...
chrome/browser/ui/gtk/location_bar_view_gtk.h:297: // The object that page
action will use to create icon for us.
the page action
the icon

[Jeffrey Yasskin, 2012-09-14 19:10:45]

On 2012/09/13 19:14:27, tbarzic wrote:
> On 2012/09/13 19:00:55, Jeffrey Yasskin wrote:
> > I talked to Aaron about this a bit, and we think a lot of the
> > delegate/factory/observer complexity might be removed if we move
> ExtensionAction
> > from common/ to browser/. I'm writing a change to see if that's even
possible
> > before we say that's definitely the way to do this.
> > 
> > I'll also look at http://crrev.com/10915251/ while that's compiling.
> 
> Doesn't Extension have dependency on ExtensionAction?
> moving ExtensionAction to browser could result in more complexity in extension
> creation code..

It does make creation and destruction a bit more complicated, but that
complexity is along the same lines as other extension-related helper structures
and doesn't turn out to require any new delegates. It's also an improvement in
that other extension folks say Extension is meant to be immutable, but
ExtensionAction can be changed by the extension's javascript.

http://codereview.chromium.org/10911300/ starts this move and gets chrome (but
not the tests) compiling and running. It's a rough draft, so there are still
several things that would need to be cleaned up, and I'm not likely to finish
the move for another ~two weeks, so you probably want to patch in that change
and finish it in order to make the asynchronous hidpi stuff simple enough that
we can take it.

[tbarzic, 2012-09-14 22:24:16]

PTAL

https://chromiumcodereview.appspot.com/10905005/diff/21098/chrome/common/exte...
File chrome/common/extensions/extension_action_unittest.cc (left):

https://chromiumcodereview.appspot.com/10905005/diff/21098/chrome/common/exte...
chrome/common/extensions/extension_action_unittest.cc:71: gfx::Image icon2 =
LoadIcon("icon2.png");
this will be covered in extension_action_icon_factory_unittest

[Jeffrey Yasskin, 2012-09-14 23:47:12]

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:67: // If icon set
hasn't changed, return the present icon.
If GetIcon() is called with two different ExtensionActions, it looks like this
will always return the default icon of the first one. Can you set the
ExtensionAction as a constructor argument instead?

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:72: icon_.reset(new
IconImage(extension_,
I see a path from the observer callback through
BrowserActionButton::UpdateState() back into GetIcon(). I think there's no way
that callback can actually cause the IconImage to be re-created, but it deserves
a comment at the very least, and I'd be happier if the 'new' weren't callable
from this path at all.

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:62:
icon_factory_.reset(new ExtensionActionIconFactory(extension, this));
I think this could be a value member like tracker_ was instead of a scoped_ptr.

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:222:
icon_factory_.GetIcon(extension_->browser_action(),tab_id);
Commas should be followed by a space.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:65: int
GetIconWidthForActionType(ExtensionAction::Type type) {
You also have this function as GetIconSizeForType() in
chrome/browser/extensions/extension_action_icon_factory.cc. Please only add it
once.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:302: gfx::Image
ExtensionAction::ApplyAttentionAndAnimation(
Somewhere in this sequence of refactorings, the point of returning
gfx::Image—that it can cache the conversions to GdkPixbuf and NSImage—was lost.
No need to do it in this CL, but if it ever gets more convenient, feel free to
switch all of these to returning ImageSkia.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:160: // Icons are a bit different
because the default value can be set to either a
We should have a comment saying to retrieve an ExtensionAction's icon using
ExtensionActionIconFactory so that people know not to try to implement the
fallback behavior themselves.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:169: gfx::Image
ApplyAttentionAndAnimation(const gfx::ImageSkia& icon,
At some point, the icon-manipulation code should move to live next to
ExtensionActionIconFactory's code. Hopefully I'll be able to move
ExtensionAction into browser/ and unify it with the icon factory again, at which
point the move will be unnecessary, so you don't need to do it in this change.

[tbarzic, 2012-09-15 00:29:10]

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:67: // If icon set
hasn't changed, return the present icon.
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> If GetIcon() is called with two different ExtensionActions, it looks like this
> will always return the default icon of the first one. Can you set the
> ExtensionAction as a constructor argument instead?

Done.

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:72: icon_.reset(new
IconImage(extension_,
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> I see a path from the observer callback through
> BrowserActionButton::UpdateState() back into GetIcon(). I think there's no way
> that callback can actually cause the IconImage to be re-created, but it
deserves
> a comment at the very least, and I'd be happier if the 'new' weren't callable
> from this path at all.

if I get your comment correctly, you're afraid that we could  recreate
icon_image when the observer is invoked?
If that's true: "if (icon_.get())" above should guard as against that.

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:62:
icon_factory_.reset(new ExtensionActionIconFactory(extension, this));
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> I think this could be a value member like tracker_ was instead of a
scoped_ptr.

The constructor needs |extension| which is not created until the line before
this (and I'd rather have the extension and extension action set in the
constructor than having set_extension()/ set_extension_action).

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:222:
icon_factory_.GetIcon(extension_->browser_action(),tab_id);
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> Commas should be followed by a space.

Done.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:65: int
GetIconWidthForActionType(ExtensionAction::Type type) {
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> You also have this function as GetIconSizeForType() in
> chrome/browser/extensions/extension_action_icon_factory.cc. Please only add it
> once. 

Done.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.cc:302: gfx::Image
ExtensionAction::ApplyAttentionAndAnimation(
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> Somewhere in this sequence of refactorings, the point of returning
> gfx::Image—that it can cache the conversions to GdkPixbuf and NSImage—was
lost.
> No need to do it in this CL, but if it ever gets more convenient, feel free to
> switch all of these to returning ImageSkia.

Initially I wanted to make all of this to return ImageSkia, but rsesek@ had
objections about that (gfx::Image should be used when the image could be used on
more different platforms, which is the case here)

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:160: // Icons are a bit different
because the default value can be set to either a
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> We should have a comment saying to retrieve an ExtensionAction's icon using
> ExtensionActionIconFactory so that people know not to try to implement the
> fallback behavior themselves.

Done.

http://codereview.chromium.org/10905005/diff/23129/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:169: gfx::Image
ApplyAttentionAndAnimation(const gfx::ImageSkia& icon,
On 2012/09/14 23:47:12, Jeffrey Yasskin wrote:
> At some point, the icon-manipulation code should move to live next to
> ExtensionActionIconFactory's code. Hopefully I'll be able to move
> ExtensionAction into browser/ and unify it with the icon factory again, at
which
> point the move will be unnecessary, so you don't need to do it in this change.

yeah, I agree..

[Jeffrey Yasskin, 2012-09-17 20:18:39]

I think this is close. Thanks for simplifying the overall structure.

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:144: //
We should not be creating icons asynchronoulsy, so we don't need an
sp: asynchronoulsy

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
File chrome/browser/extensions/api/extension_action/page_action_apitest.cc
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/page_action_apitest.cc:75: // We
should not be creating icons asynchronoulsy, so we don't need an
sp: asynchronoulsy

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
File
chrome/browser/extensions/api/extension_action/page_as_browser_action_apitest.cc
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
chrome/browser/extensions/api/extension_action/page_as_browser_action_apitest.cc:97:
// We should not be creating icons asynchronoulsy, so we don't need an
sp: asynchronoulsy

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_icon_factory.cc:57: icon_.reset(new
IconImage(
Now that action_ is set in the constructor, please create the IconImage in the
constructor too so that you can simplify the fallback logic here.

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

http://codereview.chromium.org/10905005/diff/19065/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:446:
ExtensionActionIconFactory  icon_factory_;
Remove the second space between ExtensionActionIconFactory and icon_factory_.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/templates/intros/browserAction.html
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:58: If
you don't care about icon appearance when screens's device-independent
"don't care" is a little strong. The tech writers might overrule me on this, but
I'd say something like:

"If you only provide one of the 19px or 38px icon size, the extension system
will scale the icon you provide to the pixel density of the user's display,
which can lose detail or make it look fuzzy. The old syntax of
<code>"default_icon": "images/icon19.png"</code> is still supported as a
shorthand for <code>"default_icon": { "19": "images/icon19.png" }</code>.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:69:
<em>// equal to "default_icon": { "19": "images/icon19.png" }</em>
s/equal/equivalent/

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:85:
<p>Browser action icons can be up to 19 dips wide and high.
Explain the acronym the first time you use it in a page. So "dips
(device-independent pixels)" here.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:87:
use a 19-dip size square icon.</p>
No need for "size" since it wasn't there when "pixel" was used.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:109:
<code>size_in_dip / size_in_pixel</code>) is different than 1,
This should be "<code>size_in_pixels / size_in_dips</code>", right? Since then
you get "2" for mac-style hidpi displays?

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/templates/intros/pageAction.html
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/pageAction.html:57: If
you don't care about icon appearance when screens's device-independent
Ditto from browserAction.html.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/e...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/e...
chrome/common/extensions/extension_action.h:129: // should have. The icon should
be quadratic, so only one dimension is
s/quadratic/square/. "quadratic" refers to equations involving a squared term,
not the shape.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/e...
File chrome/common/extensions/extension_icon_set.h (right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/e...
chrome/common/extensions/extension_icon_set.h:49: void
CopyForTest(ExtensionIconSet* destination) const;
Despite the fact that this is only used in tests, Aaron and I think it should
just be a copy constructor or assignment operator on ExtensionIconSet. ...
Looking a bit more, there's no DISALLOW_COPY_AND_ASSIGN here, so you should just
be able to call the existing methods.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc:15:
{
Make each of these blocks its own TEST_F. That gives you more assurance that
they don't affect each other and allows each section to run even if other
sections fail.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc:17:
LoadAndExpectSuccess("browser_action_no_default_properties.json"));
Instead of storing the json you want to load separately, where it's hard to
compare it against the test's assertions, use
chrome/common/extensions/value_builder.h and possibly .../extension_builder.h to
write the manifest inline.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc:47:
EXPECT_EQ("icon38.png", icons->Get(38, ExtensionIconSet::MATCH_EXACTLY));
Please comment that you're testing that the 24px icon is ignored.

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:43:
// Verify that correnct icon paths are registered in default_icon.
sp: correnct

http://codereview.chromium.org/10905005/diff/19065/chrome/test/data/extension...
File
chrome/test/data/extensions/manifest_tests/browser_action_with_dictionary_default_icon.json
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/test/data/extension...
chrome/test/data/extensions/manifest_tests/browser_action_with_dictionary_default_icon.json:5:
"description": "Browser actin with a default icon set.",
s/actin/action/

http://codereview.chromium.org/10905005/diff/19065/chrome/test/data/extension...
File
chrome/test/data/extensions/manifest_tests/browser_action_with_string_default_icon.json
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/test/data/extension...
chrome/test/data/extensions/manifest_tests/browser_action_with_string_default_icon.json:5:
"description": "Browser actin with a default icon set.",
s/actin/action/

[mkearney1, 2012-09-17 21:17:45]

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
File chrome/common/extensions/docs/server2/templates/intros/browserAction.html
(right):

http://codereview.chromium.org/10905005/diff/19065/chrome/common/extensions/d...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:58: If
you don't care about icon appearance when screens's device-independent
I like Jeffrey's suggestion. But I think I would still put the old syntax in a
code snippet.

So, "If you only provide.... The old syntax for registering the default icon is
still supported:"

With code snippet.

On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> "don't care" is a little strong. The tech writers might overrule me on this,
but
> I'd say something like:
> 
> "If you only provide one of the 19px or 38px icon size, the extension system
> will scale the icon you provide to the pixel density of the user's display,
> which can lose detail or make it look fuzzy. The old syntax of
> <code>"default_icon": "images/icon19.png"</code> is still supported as a
> shorthand for <code>"default_icon": { "19": "images/icon19.png" }</code>.

[tbarzic, 2012-09-17 22:06:10]

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

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:144: //
We should not be creating icons asynchronoulsy, so we don't need an
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> sp: asynchronoulsy

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/page_action_apitest.cc
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/page_action_apitest.cc:75: // We
should not be creating icons asynchronoulsy, so we don't need an
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> sp: asynchronoulsy

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
File
chrome/browser/extensions/api/extension_action/page_as_browser_action_apitest.cc
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/page_as_browser_action_apitest.cc:97:
// We should not be creating icons asynchronoulsy, so we don't need an
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> sp: asynchronoulsy

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ext...
chrome/browser/extensions/extension_action_icon_factory.cc:57: icon_.reset(new
IconImage(
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Now that action_ is set in the constructor, please create the IconImage in the
> constructor too so that you can simplify the fallback logic here.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ui/...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/browser/ui/...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:446:
ExtensionActionIconFactory  icon_factory_;
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Remove the second space between ExtensionActionIconFactory and icon_factory_.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
File chrome/common/extensions/docs/server2/templates/intros/browserAction.html
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:58: If
you don't care about icon appearance when screens's device-independent
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> "don't care" is a little strong. The tech writers might overrule me on this,
but
> I'd say something like:
> 
> "If you only provide one of the 19px or 38px icon size, the extension system
> will scale the icon you provide to the pixel density of the user's display,
> which can lose detail or make it look fuzzy. The old syntax of
> <code>"default_icon": "images/icon19.png"</code> is still supported as a
> shorthand for <code>"default_icon": { "19": "images/icon19.png" }</code>.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:69:
<em>// equal to "default_icon": { "19": "images/icon19.png" }</em>
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> s/equal/equivalent/

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:85:
<p>Browser action icons can be up to 19 dips wide and high.
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Explain the acronym the first time you use it in a page. So "dips
> (device-independent pixels)" here.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:85:
<p>Browser action icons can be up to 19 dips wide and high.
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Explain the acronym the first time you use it in a page. So "dips
> (device-independent pixels)" here.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:87:
use a 19-dip size square icon.</p>
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> No need for "size" since it wasn't there when "pixel" was used.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/browserAction.html:109:
<code>size_in_dip / size_in_pixel</code>) is different than 1,
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> This should be "<code>size_in_pixels / size_in_dips</code>", right? Since then
> you get "2" for mac-style hidpi displays?

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
File chrome/common/extensions/docs/server2/templates/intros/pageAction.html
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/docs/server2/templates/intros/pageAction.html:57: If
you don't care about icon appearance when screens's device-independent
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Ditto from browserAction.html.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/extension_action.h:129: // should have. The icon should
be quadratic, so only one dimension is
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> s/quadratic/square/. "quadratic" refers to equations involving a squared term,
> not the shape.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
File chrome/common/extensions/extension_icon_set.h (right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/extension_icon_set.h:49: void
CopyForTest(ExtensionIconSet* destination) const;
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Despite the fact that this is only used in tests, Aaron and I think it should
> just be a copy constructor or assignment operator on ExtensionIconSet. ...
> Looking a bit more, there's no DISALLOW_COPY_AND_ASSIGN here, so you should
just
> be able to call the existing methods.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
File
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc:15:
{
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Make each of these blocks its own TEST_F. That gives you more assurance that
> they don't affect each other and allows each section to run even if other
> sections fail.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc:17:
LoadAndExpectSuccess("browser_action_no_default_properties.json"));
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Instead of storing the json you want to load separately, where it's hard to
> compare it against the test's assertions, use
> chrome/common/extensions/value_builder.h and possibly .../extension_builder.h
to
> write the manifest inline.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/manifest_tests/extension_manifests_browseraction_unittest.cc:47:
EXPECT_EQ("icon38.png", icons->Get(38, ExtensionIconSet::MATCH_EXACTLY));
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> Please comment that you're testing that the 24px icon is ignored.

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
File
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/common/exte...
chrome/common/extensions/manifest_tests/extension_manifests_scriptbadge_unittest.cc:43:
// Verify that correnct icon paths are registered in default_icon.
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> sp: correnct

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/test/data/e...
File
chrome/test/data/extensions/manifest_tests/browser_action_with_dictionary_default_icon.json
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/test/data/e...
chrome/test/data/extensions/manifest_tests/browser_action_with_dictionary_default_icon.json:5:
"description": "Browser actin with a default icon set.",
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> s/actin/action/

Done.

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/test/data/e...
File
chrome/test/data/extensions/manifest_tests/browser_action_with_string_default_icon.json
(right):

https://chromiumcodereview.appspot.com/10905005/diff/19065/chrome/test/data/e...
chrome/test/data/extensions/manifest_tests/browser_action_with_string_default_icon.json:5:
"description": "Browser actin with a default icon set.",
On 2012/09/17 20:18:39, Jeffrey Yasskin wrote:
> s/actin/action/

Done.

[Jeffrey Yasskin, 2012-09-17 22:27:22]

LGTM; just two nits.

https://chromiumcodereview.appspot.com/10905005/diff/170/chrome/browser/exten...
File chrome/browser/extensions/extension_action_icon_factory.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/170/chrome/browser/exten...
chrome/browser/extensions/extension_action_icon_factory.cc:61: if
(action_->default_icon())
I would probably make this conditional on icon_.get() ... and rename icon_ to
default_icon_. That lets the reader avoid checking the constructor to make sure
icon_ is set when action_->default_icon() is true.

https://chromiumcodereview.appspot.com/10905005/diff/170/chrome/common/extens...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10905005/diff/170/chrome/common/extens...
chrome/common/extensions/extension_action.cc:262: copy->default_icon_ =
default_icon_copy.Pass();
Use "copy->default_icon_.reset(new ExtensionIconSet(*default_icon));"

[Aaron Boodman, 2012-09-17 22:40:37]

Can you please provide a screen shot of what happens when an extension only has
a 1x browser action icon, but is displayed on a 2x device?

I originally requested that we never resize icons up. So I would expect that we
either fall back to a puzzle piece, or else display a teeny icon in the middle
of a big button.

I'm not sure whether the right behavior here is to go ahead and resize up, or
else fall back to the extension's manifest icon. A screen shot would probably
help clarify this.

[tbarzic, 2012-09-17 22:51:26]

On 2012/09/17 22:40:37, Aaron Boodman wrote:
> Can you please provide a screen shot of what happens when an extension only
has
> a 1x browser action icon, but is displayed on a 2x device?
> 
> I originally requested that we never resize icons up. So I would expect that
we
> either fall back to a puzzle piece, or else display a teeny icon in the middle
> of a big button.
> 
> I'm not sure whether the right behavior here is to go ahead and resize up, or
> else fall back to the extension's manifest icon. A screen shot would probably
> help clarify this.

the current state:
http://www.corp.google.com/~tbarzic/no_crawl/19px_default_icon.png

[Aaron Boodman, 2012-09-17 23:58:07]

LGTM, but it might be good to play with some different resize algorithms. This
is pretty important since it is what everyone is going to see (since no
extensions are going to have dual density icons for quite awhile).

You should ask UI team their opinion, but I just spent some quality time with a
high density display and this image: http://i.imgur.com/cIjn8.png.

If we're not already, it could be good to go to bicubic or lanczos (not pictured
here, but should be similar to bicubic sharper).

[commit-bot: I haz the power, 2012-09-18 01:01:07]

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

[commit-bot: I haz the power, 2012-09-18 01:01:23]

Presubmit check for 10905005-23143 failed and returned exit status 2.


All integration tests passed!
"/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2/PRESUBMIT.py"
had an exception.
[Errno 2] No such file or directory:
'/b/commit-queue/workdir/chromium/chrome/common/extensions/docs/server2/src/chrome/common/extensions/docs/server2/../../../../../third_party/handlebar'
Maybe your depot_tools is out of date?
If all fails, contact maruel@
Running presubmit commit checks ...