arc: Support non-standard display scale factors.

arc: Support non-standard display scale factors.

This CL makes it possible to use ARC app icons in notification view,
ARC uninstall dialog view and other places.

TEST=Unit test + manually on device.
BUG=725927
BUG=b/36790645
BUG=b/34521325

Review-Url: https://codereview.chromium.org/2902153002
Cr-Commit-Position: refs/heads/master@{#475185}
Committed: https://chromium.googlesource.com/chromium/src/+/756189476715f0585dc70426f734a46be74d585e

[khmel, 2017-05-24 15:05:41]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org

[khmel, 2017-05-24 15:05:41]

Hi Xiyuan,

Quite tricky case. On some platform we have display scale factor "non-standard",
for example 1.5. From other side ImageSkia primarily supports 'supported scales'
such as 1.0, 2.0, 3.0. For other scales it uses nearest supported scale and
rescales it internally. In case of ARC that means we are asked for 2.0 image and
ImageSkia internally converts it to 1.25. But ARC app icon loading is async.
That means we returned 2.0 default app icon and scheduled async update for 2.0
icon. Once icon is updated we update relevant scale, 2.0. But we are not aware
about 1.25 scale that was generated from default image.
One my attempt was to pre-allocate scale 1.25 before using ARC icon. In this
case ImageSkia would find already existing scale and will use it. However here I
faced with another problem. Normally we can access icon on Android side only for
1.0, 1.5 2.0 and 3.0 scales.
So suggested solution looks as following. Once ARC icon is updated we also
update non-standard scales, generated from supported scale.

PTAL

[xiyuan, 2017-05-24 16:37:10]

lgtm

https://codereview.chromium.org/2902153002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right):

https://codereview.chromium.org/2902153002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:362: // scales that are mapped to
updated scale.
nit: Emphasize on removing dependent reps of the new bitmap,

e.g.
  // Regenerate existing non-supported image representations that
  // depend on the rep of |scale_factor|.

[khmel, 2017-05-24 20:20:58]

khmel@chromium.org changed reviewers:
+ msw@chromium.org

[khmel, 2017-05-24 20:20:58]

Thanks Xiyuan for review!

Adding Mike for approval ui/
PTAL

https://codereview.chromium.org/2902153002/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right):

https://codereview.chromium.org/2902153002/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:362: // scales that are mapped to
updated scale.
On 2017/05/24 16:37:09, xiyuan wrote:
> nit: Emphasize on removing dependent reps of the new bitmap,
> 
> e.g.
>   // Regenerate existing non-supported image representations that
>   // depend on the rep of |scale_factor|.

Nice...

[msw, 2017-05-24 20:27:59]

msw@chromium.org changed reviewers:
+ oshima@chromium.org

[msw, 2017-05-24 20:27:59]

ui/gfx/image_skia.* lgtm with a nit.
+oshima, a better OWNER of ui/gfx/image_skia*

https://codereview.chromium.org/2902153002/diff/40001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

https://codereview.chromium.org/2902153002/diff/40001/ui/gfx/image/image_skia...
ui/gfx/image/image_skia.h:78: // Maps to closest supported scale.
nit: "Maps |scale| to the closest supported scale."
Should it say something about preferring higher or lower scales?

[khmel, 2017-05-24 21:32:37]

Thanks Mike, will wait Oshima's review.

https://codereview.chromium.org/2902153002/diff/40001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

https://codereview.chromium.org/2902153002/diff/40001/ui/gfx/image/image_skia...
ui/gfx/image/image_skia.h:78: // Maps to closest supported scale.
On 2017/05/24 20:27:59, msw wrote:
> nit: "Maps |scale| to the closest supported scale."
> Should it say something about preferring higher or lower scales?

Done.

[msw, 2017-05-24 21:44:18]

https://codereview.chromium.org/2902153002/diff/60001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

https://codereview.chromium.org/2902153002/diff/60001/ui/gfx/image/image_skia...
ui/gfx/image/image_skia.h:78: // Maps to the closest supported scale. In case
scale is greater than the
nit: This is worded a bit awkwardly; consider something like "Maps to the
closest supported scale. Returns an exact match, a smaller scale within 0.2
units, the nearest larger scale, or the min/max supported scale."

https://codereview.chromium.org/2902153002/diff/60001/ui/gfx/image/image_skia...
ui/gfx/image/image_skia.h:149: // There is no guarantee that this will r+images
rep for
fix

[khmel, 2017-05-26 15:34:53]

Thanks Mike!

https://codereview.chromium.org/2902153002/diff/60001/ui/gfx/image/image_skia.h
File ui/gfx/image/image_skia.h (right):

https://codereview.chromium.org/2902153002/diff/60001/ui/gfx/image/image_skia...
ui/gfx/image/image_skia.h:78: // Maps to the closest supported scale. In case
scale is greater than the
On 2017/05/24 21:44:18, msw wrote:
> nit: This is worded a bit awkwardly; consider something like "Maps to the
> closest supported scale. Returns an exact match, a smaller scale within 0.2
> units, the nearest larger scale, or the min/max supported scale."

Done.

https://codereview.chromium.org/2902153002/diff/60001/ui/gfx/image/image_skia...
ui/gfx/image/image_skia.h:149: // There is no guarantee that this will r+images
rep for
On 2017/05/24 21:44:18, msw wrote:
> fix

Done.

[oshima, 2017-05-26 16:03:09]

one qq.

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right):

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:333:
MaybeRequestIcon(read_result->scale_factor);
How is this scale_factor obtained?

[khmel, 2017-05-26 16:10:54]

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right):

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:333:
MaybeRequestIcon(read_result->scale_factor);
On 2017/05/26 16:03:09, oshima wrote:
> How is this scale_factor obtained?

There is a long chain of calls and finally this scale factor is obtained from
device_scale_factor.
https://cs.chromium.org/chromium/src/ui/compositor/layer.h?l=389

Note, there is similar issue for extension-based apps. In that case we don't use
IconImage but use IconImageLoader and force loading image via loader of 100P. I
think we can do better, similar I do for ARC apps. I plan in one of the next CL
to do similar think for extension-based apps. To use ChromeAppIcon and map image
to non-standard reps.
In last case I think it is better to move functionality to update non-supported
reps to ImageSkia. But this is not for this CL.

[oshima, 2017-05-26 16:14:17]

On 2017/05/26 16:10:54, khmel wrote:
>
https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
> File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right):
> 
>
https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
> chrome/browser/ui/app_list/arc/arc_app_icon.cc:333:
> MaybeRequestIcon(read_result->scale_factor);
> On 2017/05/26 16:03:09, oshima wrote:
> > How is this scale_factor obtained?
> 
> There is a long chain of calls and finally this scale factor is obtained from
> device_scale_factor.
> https://cs.chromium.org/chromium/src/ui/compositor/layer.h?l=389

That is float value. My question was where the value of ui::ScaleFactor is
obtained for this.

> 
> Note, there is similar issue for extension-based apps. In that case we don't
use
> IconImage but use IconImageLoader and force loading image via loader of 100P.
I
> think we can do better, similar I do for ARC apps. I plan in one of the next
CL
> to do similar think for extension-based apps. To use ChromeAppIcon and map
image
> to non-standard reps.
> In last case I think it is better to move functionality to update
non-supported
> reps to ImageSkia. But this is not for this CL.

[khmel, 2017-05-26 16:21:01]

On 2017/05/26 16:14:17, oshima wrote:
> On 2017/05/26 16:10:54, khmel wrote:
> >
>
https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
> > File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right):
> > 
> >
>
https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
> > chrome/browser/ui/app_list/arc/arc_app_icon.cc:333:
> > MaybeRequestIcon(read_result->scale_factor);
> > On 2017/05/26 16:03:09, oshima wrote:
> > > How is this scale_factor obtained?
> > 
> > There is a long chain of calls and finally this scale factor is obtained
from
> > device_scale_factor.
> > https://cs.chromium.org/chromium/src/ui/compositor/layer.h?l=389
> 
> That is float value. My question was where the value of ui::ScaleFactor is
> obtained for this.

Sorry, got you Q wrongly.
1. Request to load 1.25 scale came here: 
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.cc?l=191

2. ImageSkia detects that this is non-supported scale:
https://cs.chromium.org/chromium/src/ui/gfx/image/image_skia.cc?l=234

3. Next ImageSkia asks for supported scale rep 200P from our source: Line 235
there.

4. Rep got on step 3 re-scaled and stored for 1.25, Line 238 there.



> 
> > 
> > Note, there is similar issue for extension-based apps. In that case we don't
> use
> > IconImage but use IconImageLoader and force loading image via loader of
100P.
> I
> > think we can do better, similar I do for ARC apps. I plan in one of the next
> CL
> > to do similar think for extension-based apps. To use ChromeAppIcon and map
> image
> > to non-standard reps.
> > In last case I think it is better to move functionality to update
> non-supported
> > reps to ImageSkia. But this is not for this CL.

[oshima, 2017-05-26 16:29:11]

Looks like it's coming from

https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_i...

Let's chat offline.

[oshima, 2017-05-26 16:29:13]

Looks like it's coming from

https://cs.chromium.org/chromium/src/chrome/browser/ui/app_list/arc/arc_app_i...

Let's chat offline.

[oshima, 2017-05-26 18:13:23]

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_icon.cc (left):

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:370:
image_skia_.AddRepresentation(image_rep);
I looked into a bit more, and the problem seems to be that it has a cache
created from default app icon.
You can clear the cache for unsupported scales like this:

  for (const gfx::ImageSkiaRep& image_rep_to_test : image_skia_.image_reps()) {
    if (!ui::IsSupportedScale(image_rep_to_test.scale()))
      image_skia_.RemoveRepresentation(image_rep_to_test.scale());
  }

I tested on caroline + 1.25dsf and this did fix  both issues.

Maybe ImageSkia should have a method to clear the cache for unsupported scales,
so you can add it to ImageSkia.
I'm fine with either way.

[khmel, 2017-05-26 19:31:21]

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/arc/arc_app_icon.cc (left):

https://codereview.chromium.org/2902153002/diff/80001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/arc/arc_app_icon.cc:370:
image_skia_.AddRepresentation(image_rep);
On 2017/05/26 18:13:23, oshima wrote:
> I looked into a bit more, and the problem seems to be that it has a cache
> created from default app icon.
> You can clear the cache for unsupported scales like this:
> 
>   for (const gfx::ImageSkiaRep& image_rep_to_test : image_skia_.image_reps())
{
>     if (!ui::IsSupportedScale(image_rep_to_test.scale()))
>       image_skia_.RemoveRepresentation(image_rep_to_test.scale());
>   }
> 
> I tested on caroline + 1.25dsf and this did fix  both issues.
> 
> Maybe ImageSkia should have a method to clear the cache for unsupported
scales,
> so you can add it to ImageSkia.
> I'm fine with either way.

Thanks for offline chat! As discussed I moved invalidation code to ImageSkia.

PTAL

[oshima, 2017-05-26 20:01:05]

lgtm if you addressed the following.

https://codereview.chromium.org/2902153002/diff/100001/ui/gfx/image/image_ski...
File ui/gfx/image/image_skia.cc (right):

https://codereview.chromium.org/2902153002/diff/100001/ui/gfx/image/image_ski...
ui/gfx/image/image_skia.cc:486: std::vector<float> scales_to_remove;
image reps returns the vector by value, so you can just remove it in the loop.

[khmel, 2017-05-26 20:13:55]

Thank you for review!

https://codereview.chromium.org/2902153002/diff/100001/ui/gfx/image/image_ski...
File ui/gfx/image/image_skia.cc (right):

https://codereview.chromium.org/2902153002/diff/100001/ui/gfx/image/image_ski...
ui/gfx/image/image_skia.cc:486: std::vector<float> scales_to_remove;
On 2017/05/26 20:01:05, oshima wrote:
> image reps returns the vector by value, so you can just remove it in the loop.

Good point, done.

[khmel, 2017-05-26 20:14:01]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-05-26 20:14:01]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
msw@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2902153002/#ps120001
(title: "remove rep in loop")

[commit-bot: I haz the power, 2017-05-26 20:14:34]

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-05-26 20:16:48]

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

[commit-bot: I haz the power, 2017-05-26 20:16:49]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[khmel, 2017-05-26 22:03:18]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-05-26 22:03:19]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
oshima@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2902153002/#ps140001
(title: "rebase + unit test update to match this CL")

[commit-bot: I haz the power, 2017-05-26 22:04:01]

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-05-26 23:29:55]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495836198521050,
"parent_rev": "d6ea165b0d47b5442e7b87cbb0b0fd1006f2a115", "commit_rev":
"756189476715f0585dc70426f734a46be74d585e"}

[commit-bot: I haz the power, 2017-05-26 23:30:07]

Description was changed from

==========
arc: Support non-standard display scale factors.

This CL makes it possible to use ARC app icons in notification view,
ARC uninstall dialog view and other places.

TEST=Unit test + manually on device.
BUG=725927
BUG=b/36790645
BUG=b/34521325
==========

to

==========
arc: Support non-standard display scale factors.

This CL makes it possible to use ARC app icons in notification view,
ARC uninstall dialog view and other places.

TEST=Unit test + manually on device.
BUG=725927
BUG=b/36790645
BUG=b/34521325

Review-Url: https://codereview.chromium.org/2902153002
Cr-Commit-Position: refs/heads/master@{#475185}
Committed:
https://chromium.googlesource.com/chromium/src/+/756189476715f0585dc70426f734...
==========

[commit-bot: I haz the power, 2017-05-26 23:30:09]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/756189476715f0585dc70426f734...