Handle more scale factors for extension Browser Action icons

Handle more scale factors for extension Browser Action icons

Also use 16x16 if provided in Material Design mode.
Also, fix size of default icon (generated from extension's first letter) for MD.

BUG=564926
Committed: https://crrev.com/39ea51bd9379fa9aaa974f8b91a559879175e681
Cr-Commit-Position: refs/heads/master@{#363278}

[Evan Stade, 2015-12-02 23:21:04]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-02 23:22:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492073003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492073003/20001

[Evan Stade, 2015-12-02 23:25:31]

estade@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Evan Stade, 2015-12-02 23:25:32]

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.cc (right):

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.cc:309: // Fall back to the product
icons if no action icon exists.
this is a behavioral change (and for the better, IMO). Before if you specified
19 but not 38, the 38 would get filled by a product icon. This means that for 2x
scale you have a different browser action icon than 1x scale (instead of a
blown-up version of 1x). Now you'd only get the product icon here if you have no
browser action icons specified at any size whatsoever.

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.h (right):

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.h:30: bool
LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
this is still used for product icons. We might should do the same thing for
product icons that this patch does for browser actions (i.e. not restrict
developers to a specific set of sizes), and then this function would go away.

[Devlin, 2015-12-03 00:00:38]

lgtm

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action.cc (right):

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action.cc:309: // Fall back to the product
icons if no action icon exists.
On 2015/12/02 23:25:32, Evan Stade wrote:
> this is a behavioral change (and for the better, IMO). Before if you specified
> 19 but not 38, the 38 would get filled by a product icon. This means that for
2x
> scale you have a different browser action icon than 1x scale (instead of a
> blown-up version of 1x). Now you'd only get the product icon here if you have
no
> browser action icons specified at any size whatsoever.

+1

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action_storage_manager.cc (right):

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action_storage_manager.cc:97: int int_value;
while you're here, could you also initialize this?  (I'd also prefer it moved
closer to it's first use, but I don't feel terrible strongly.)

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.h (right):

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.h:30: bool
LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
On 2015/12/02 23:25:32, Evan Stade wrote:
> this is still used for product icons. We might should do the same thing for
> product icons that this patch does for browser actions (i.e. not restrict
> developers to a specific set of sizes), and then this function would go away.

That would be preferable, yes.  Is it too big to do in this CL?

[commit-bot: I haz the power, 2015-12-03 00:03:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 00:03:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2015-12-04 01:29:53]

estade@chromium.org changed reviewers:
+ rohitrao@chromium.org, sky@chromium.org

[Evan Stade, 2015-12-04 01:29:53]

+rohitrao for c/b/ui/cocoa
+sky for c/b/ui/views/

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_action_storage_manager.cc (right):

https://codereview.chromium.org/1492073003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_action_storage_manager.cc:97: int int_value;
On 2015/12/03 00:00:38, Devlin wrote:
> while you're here, could you also initialize this?  (I'd also prefer it moved
> closer to it's first use, but I don't feel terrible strongly.)

Done.

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.h (right):

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.h:30: bool
LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
On 2015/12/03 00:00:38, Devlin wrote:
> On 2015/12/02 23:25:32, Evan Stade wrote:
> > this is still used for product icons. We might should do the same thing for
> > product icons that this patch does for browser actions (i.e. not restrict
> > developers to a specific set of sizes), and then this function would go
away.
> 
> That would be preferable, yes.  Is it too big to do in this CL?

I like atomic commits, so I'd prefer to do that in a follow up.

[Evan Stade, 2015-12-04 01:29:58]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 01:35:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492073003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492073003/60001

[commit-bot: I haz the power, 2015-12-04 02:22:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 02:22:42]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2015-12-04 03:54:40]

LGTM

[Evan Stade, 2015-12-04 04:32:17]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 04:33:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492073003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492073003/80001

[commit-bot: I haz the power, 2015-12-04 04:43:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-04 04:43:40]

Dry run: This issue passed the CQ dry run.

[Devlin, 2015-12-04 16:51:39]

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.h (right):

https://codereview.chromium.org/1492073003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.h:30: bool
LoadIconsFromDictionary(const base::DictionaryValue* icons_value,
On 2015/12/04 01:29:53, Evan Stade wrote:
> On 2015/12/03 00:00:38, Devlin wrote:
> > On 2015/12/02 23:25:32, Evan Stade wrote:
> > > this is still used for product icons. We might should do the same thing
for
> > > product icons that this patch does for browser actions (i.e. not restrict
> > > developers to a specific set of sizes), and then this function would go
> away.
> > 
> > That would be preferable, yes.  Is it too big to do in this CL?
> 
> I like atomic commits, so I'd prefer to do that in a follow up.

SGTM.  If you think you'll get to it, great, otherwise go ahead and file a bug
against me so that it doesn't get dropped. :)

[groby-ooo-7-16, 2015-12-04 19:54:56]

groby@chromium.org changed reviewers:
+ groby@chromium.org

[groby-ooo-7-16, 2015-12-04 19:54:57]

c/b/ui/cocoa LGTM

[Evan Stade, 2015-12-04 19:55:04]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-12-04 19:55:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1492073003/#ps80001
(title: "test catches real bug!")

[commit-bot: I haz the power, 2015-12-04 19:55:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1492073003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1492073003/80001

[commit-bot: I haz the power, 2015-12-04 20:10:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-04 20:11:19]

Description was changed from

==========
Handle more scale factors for extension Browser Action icons

Also use 16x16 if provided in Material Design mode.
Also, fix size of default icon (generated from extension's first letter) for MD.

BUG=564926
==========

to

==========
Handle more scale factors for extension Browser Action icons

Also use 16x16 if provided in Material Design mode.
Also, fix size of default icon (generated from extension's first letter) for MD.

BUG=564926

Committed: https://crrev.com/39ea51bd9379fa9aaa974f8b91a559879175e681
Cr-Commit-Position: refs/heads/master@{#363278}
==========

[commit-bot: I haz the power, 2015-12-04 20:11:20]

Patchset 5 (id:??) landed as
https://crrev.com/39ea51bd9379fa9aaa974f8b91a559879175e681
Cr-Commit-Position: refs/heads/master@{#363278}