Load extension icons for more scale factors.

Load extension icons for more scale factors.

The "supported" scale factors are just the ones that we load raster
assets for (in ResourceBundle). This patch additionally looks for
icons in scale factors that correlate with the currently attached
displays.

Practically speaking, this means a machine that's running at 1.5x such
as a Surface Pro will have better looking extension icons in the
renderer context menu.

BUG=596757
Review-Url: https://codereview.chromium.org/2609853003
Cr-Commit-Position: refs/heads/master@{#444128}
Committed: https://chromium.googlesource.com/chromium/src/+/47ce132c79896db4fe4832a96b097adc643e6e99

[Evan Stade, 2017-01-03 23:09:31]

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

[Devlin, 2017-01-04 15:58:13]

On 2017/01/03 23:09:31, Evan Stade wrote:

This looks okay, but can we add a unittest?  Also, do you have screenshots that
you can post to the bug for posterity?

[Devlin, 2017-01-04 15:59:10]

https://codereview.chromium.org/2609853003/diff/1/extensions/browser/image_lo...
File extensions/browser/image_loader.h (right):

https://codereview.chromium.org/2609853003/diff/1/extensions/browser/image_lo...
extensions/browser/image_loader.h:90: // Loads and returns a gfx::Image that has
representations at all scale
And one small nit: this function doesn't return; it calls the callback with the
result (this was wrong before, too, but since we're updating the comment anyway
it might be worth fixing).

[Evan Stade, 2017-01-06 21:57:42]

test added. Screenshots aren't very informative beyond what's already posted on
the bug. (Despite my previous assertions, I believe comment 2 is actually
displaying blurriness at 1.5x, and a screenshot of what it looks like with this
patch is just the same but with a smoother looking icon.)

https://codereview.chromium.org/2609853003/diff/1/extensions/browser/image_lo...
File extensions/browser/image_loader.h (right):

https://codereview.chromium.org/2609853003/diff/1/extensions/browser/image_lo...
extensions/browser/image_loader.h:90: // Loads and returns a gfx::Image that has
representations at all scale
On 2017/01/04 15:59:10, Devlin wrote:
> And one small nit: this function doesn't return; it calls the callback with
the
> result (this was wrong before, too, but since we're updating the comment
anyway
> it might be worth fixing).

Done.

[Evan Stade, 2017-01-06 21:58:09]

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

[commit-bot: I haz the power, 2017-01-06 21:58:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 22:40:38]

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

[commit-bot: I haz the power, 2017-01-06 22:40:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-01-06 23:22:19]

Looks like the latest patchset is breaking a bunch of bots; mind updating and
re-pinging?

[Evan Stade, 2017-01-07 01:17:38]

https://codereview.chromium.org/2609853003/diff/40001/extensions/browser/exte...
File extensions/browser/extension_icon_image.cc (left):

https://codereview.chromium.org/2609853003/diff/40001/extensions/browser/exte...
extensions/browser/extension_icon_image.cc:204: scale_factor));
the failure was due to scale_factor being an enum value --- I changed
ImageRepresentation's constructor and this was implicitly cast to float.

I fixed that and also changed more of the ScaleFactors in this file to be float.
Not sure if that's OK, or if there is a reason to use the ScaleFactor enum.
Devlin or Oshima, do you know if converting these ScaleFactors to floats is OK?

[Evan Stade, 2017-01-07 01:17:42]

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

[commit-bot: I haz the power, 2017-01-07 01:18:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 01:43:33]

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

[commit-bot: I haz the power, 2017-01-07 01:43:34]

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

[Evan Stade, 2017-01-09 21:00:00]

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

[commit-bot: I haz the power, 2017-01-09 21:00:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2017-01-09 21:32:10]

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

[Evan Stade, 2017-01-09 21:32:12]

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

[commit-bot: I haz the power, 2017-01-09 21:33:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2017-01-09 21:58:33]

lgtm % nits

https://codereview.chromium.org/2609853003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_icon_manager_unittest.cc (right):

https://codereview.chromium.org/2609853003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_icon_manager_unittest.cc:279: // Now check
that the DSFs for active displays are respected.
we should say what a dsf is here, since I don't think it's terribly common and
isn't referenced anywhere else in this file.

https://codereview.chromium.org/2609853003/diff/40001/extensions/browser/exte...
File extensions/browser/extension_icon_image.cc (left):

https://codereview.chromium.org/2609853003/diff/40001/extensions/browser/exte...
extensions/browser/extension_icon_image.cc:204: scale_factor));
On 2017/01/07 01:17:38, Evan Stade wrote:
> the failure was due to scale_factor being an enum value --- I changed
> ImageRepresentation's constructor and this was implicitly cast to float.
> 
> I fixed that and also changed more of the ScaleFactors in this file to be
float.
> Not sure if that's OK, or if there is a reason to use the ScaleFactor enum.
> Devlin or Oshima, do you know if converting these ScaleFactors to floats is
OK?

As far as I know, this is fine, but oshima@ might know better.

https://codereview.chromium.org/2609853003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_icon_manager_unittest.cc (right):

https://codereview.chromium.org/2609853003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_icon_manager_unittest.cc:274: else if
(ui::IsSupportedScale(scale))
if it's *not* a supported scale, can we assert that we don't have a
representation (since presumably we don't have a screen here)?

[commit-bot: I haz the power, 2017-01-09 22:27:22]

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

[commit-bot: I haz the power, 2017-01-09 22:27:23]

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

[Evan Stade, 2017-01-09 23:27:39]

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

[commit-bot: I haz the power, 2017-01-09 23:28:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 00:07:44]

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

[commit-bot: I haz the power, 2017-01-10 00:07:45]

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

[Evan Stade, 2017-01-11 01:58:13]

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

[commit-bot: I haz the power, 2017-01-11 01:58:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 03:41:46]

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

[commit-bot: I haz the power, 2017-01-11 03:41:47]

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

[Evan Stade, 2017-01-12 23:17:04]

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

[commit-bot: I haz the power, 2017-01-12 23:18:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 00:15:15]

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

[commit-bot: I haz the power, 2017-01-13 00:15:16]

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

[Evan Stade, 2017-01-13 02:16:40]

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

[commit-bot: I haz the power, 2017-01-13 02:17:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 05:00:18]

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

[commit-bot: I haz the power, 2017-01-13 05:00:19]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-01-13 05:51:24]

finally got it all green, ptal

[Devlin, 2017-01-13 15:53:48]

s lgtm

https://codereview.chromium.org/2609853003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_icon_manager_unittest.cc (right):

https://codereview.chromium.org/2609853003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_icon_manager_unittest.cc:274: else if
(ui::IsSupportedScale(scale))
On 2017/01/09 21:58:33, Devlin wrote:
> if it's *not* a supported scale, can we assert that we don't have a
> representation (since presumably we don't have a screen here)?

ping on this one

https://codereview.chromium.org/2609853003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_icon_manager_unittest.cc (right):

https://codereview.chromium.org/2609853003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_icon_manager_unittest.cc:56:
base::test::ScopedCommandLine cl_;
nitty nit: style discourages abbreviation in variable names. [1]  Maybe just
command_line_ or scoped_command_line_?

[1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

[Evan Stade, 2017-01-17 19:07:11]

estade@chromium.org changed reviewers:
+ derat@chromium.org

[Evan Stade, 2017-01-17 19:07:11]

+derat for one line change in ui/display

https://codereview.chromium.org/2609853003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_icon_manager_unittest.cc (right):

https://codereview.chromium.org/2609853003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_icon_manager_unittest.cc:274: else if
(ui::IsSupportedScale(scale))
On 2017/01/13 15:53:47, Devlin (possibly slow) wrote:
> On 2017/01/09 21:58:33, Devlin wrote:
> > if it's *not* a supported scale, can we assert that we don't have a
> > representation (since presumably we don't have a screen here)?
> 
> ping on this one

sorry for not responding. I reverted this change.

[Evan Stade, 2017-01-17 19:08:02]

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

[commit-bot: I haz the power, 2017-01-17 19:08:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2017-01-17 19:20:05]

lgtm for ui/display

[Evan Stade, 2017-01-17 20:01:01]

https://codereview.chromium.org/2609853003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_icon_manager_unittest.cc (right):

https://codereview.chromium.org/2609853003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_icon_manager_unittest.cc:56:
base::test::ScopedCommandLine cl_;
On 2017/01/13 15:53:48, Devlin (possibly slow) wrote:
> nitty nit: style discourages abbreviation in variable names. [1]  Maybe just
> command_line_ or scoped_command_line_?
> 
> [1] https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

Done.

[Evan Stade, 2017-01-17 20:01:14]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2017-01-17 20:01:17]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-01-17 20:01:17]

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/2609853003/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-17 20:01:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 20:37:51]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484683277291860,
"parent_rev": "cd451bb0505aff90663e653d7a27b1f33ebcb6ba", "commit_rev":
"47ce132c79896db4fe4832a96b097adc643e6e99"}

[commit-bot: I haz the power, 2017-01-17 20:38:21]

Description was changed from

==========
Load extension icons for more scale factors.

The "supported" scale factors are just the ones that we load raster
assets for (in ResourceBundle). This patch additionally looks for
icons in scale factors that correlate with the currently attached
displays.

Practically speaking, this means a machine that's running at 1.5x such
as a Surface Pro will have better looking extension icons in the
renderer context menu.

BUG=596757
==========

to

==========
Load extension icons for more scale factors.

The "supported" scale factors are just the ones that we load raster
assets for (in ResourceBundle). This patch additionally looks for
icons in scale factors that correlate with the currently attached
displays.

Practically speaking, this means a machine that's running at 1.5x such
as a Surface Pro will have better looking extension icons in the
renderer context menu.

BUG=596757

Review-Url: https://codereview.chromium.org/2609853003
Cr-Commit-Position: refs/heads/master@{#444128}
Committed:
https://chromium.googlesource.com/chromium/src/+/47ce132c79896db4fe4832a96b09...
==========

[commit-bot: I haz the power, 2017-01-17 20:38:22]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/47ce132c79896db4fe4832a96b09...