Change ui::wm::Shadow to pass ImageSkia instead of SkBitmap.

Change ui::wm::Shadow to pass ImageSkia instead of SkBitmap.

This allows ui::Layer to take scale factor into consideration when updating the cc::NinePatchLayer.

BUG=466950
R=danakj@chromium.org
R=oshima@chromium.org
TBR=sky@chromium.org
TEST=verify on HiDPI display as well as HiDPI + LowDPI extended mode that shadows are correct.

Committed: https://crrev.com/270f62296ebb49c6bcaff3dc8715346d3f0d321a
Cr-Commit-Position: refs/heads/master@{#320974}

[hshi1, 2015-03-14 01:20:14]

hshi@chromium.org changed reviewers:
+ oshima@chromium.org, thakis@chromium.org

[hshi1, 2015-03-14 01:20:15]

PTAL. This CL intends to let ui::Layer take care of device scale factor related
calculations so that ui::wm::Shadow can simply pass the ImageSkia without
worrying about the scale factor itself.

[hshi1, 2015-03-14 01:38:48]

On 2015/03/14 01:20:15, hshi1 wrote:
> PTAL. This CL intends to let ui::Layer take care of device scale factor
related
> calculations so that ui::wm::Shadow can simply pass the ImageSkia without
> worrying about the scale factor itself.

Hmm, please hold off reviewing as I'm still seeing some issues.

It appears that ui::Layer::device_scale_factor_ is not always correct. For
example on Pixel single display mode I'm seeing that the factor is still 1.0f
for the Ash desktop context menus (change wallpaper / shelf position etc.),
although window shadows layer do have 2.0f.

[oshima, 2015-03-14 05:39:46]

On 2015/03/14 01:38:48, hshi1 wrote:
> On 2015/03/14 01:20:15, hshi1 wrote:
> > PTAL. This CL intends to let ui::Layer take care of device scale factor
> related
> > calculations so that ui::wm::Shadow can simply pass the ImageSkia without
> > worrying about the scale factor itself.
> 
> Hmm, please hold off reviewing as I'm still seeing some issues.
> 
> It appears that ui::Layer::device_scale_factor_ is not always correct. For
> example on Pixel single display mode I'm seeing that the factor is still 1.0f
> for the Ash desktop context menus (change wallpaper / shelf position etc.),
> although window shadows layer do have 2.0f.

It's 1.0f until it's added to the tree, and that's probably what you saw in
Shadow::Init()

[oshima, 2015-03-14 05:42:55]

https://codereview.chromium.org/1002393002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1002393002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:82: nine_patch_layer_bitmap_scale_factor_(1.0f) {
Why you need this? You should just use device_scale_factor_

https://codereview.chromium.org/1002393002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:717: device_scale_factor_ = device_scale_factor;
you need to update the image here.

[hshi1, 2015-03-16 16:50:48]

PTAL patch set #2. I have verified that it handles device scale factor changes
correctly. Thanks!

https://codereview.chromium.org/1002393002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1002393002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:82: nine_patch_layer_bitmap_scale_factor_(1.0f) {
On 2015/03/14 05:42:55, oshima wrote:
> Why you need this? You should just use device_scale_factor_

Done.

https://codereview.chromium.org/1002393002/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:717: device_scale_factor_ = device_scale_factor;
On 2015/03/14 05:42:55, oshima wrote:
> you need to update the image here.

Done.

[hshi1, 2015-03-16 17:09:59]

hshi@chromium.org changed reviewers:
+ danakj@chromium.org, piman@chromium.org
- thakis@chromium.org

[hshi1, 2015-03-16 17:09:59]

Nico is traveling.

piman/danakj (OWNER of ui/compositor) can you review? Thanks!

[hshi1, 2015-03-17 17:29:33]

Ping...? I think this is a fairly straightforward change, please?

[danakj, 2015-03-17 17:59:40]

LGTM

https://codereview.chromium.org/1002393002/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1002393002/diff/20001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:641: nine_patch_layer_aperture_ = aperture;
can you add "in_dip" to this variable name?

https://codereview.chromium.org/1002393002/diff/20001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:725: UpdateNinePatchLayerImage(nine_patch_layer_image_);
Can you move these up by RecomputeDrwasContentAndUVRect() and
RecomputePosition()? That way we compute things for |this| all in the same order
relative to the OnDeviceScaleFactorChanged calls.

[hshi1, 2015-03-17 18:24:48]

https://codereview.chromium.org/1002393002/diff/20001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1002393002/diff/20001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:641: nine_patch_layer_aperture_ = aperture;
On 2015/03/17 17:59:40, danakj wrote:
> can you add "in_dip" to this variable name?

Done.

https://codereview.chromium.org/1002393002/diff/20001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:725: UpdateNinePatchLayerImage(nine_patch_layer_image_);
On 2015/03/17 17:59:40, danakj wrote:
> Can you move these up by RecomputeDrwasContentAndUVRect() and
> RecomputePosition()? That way we compute things for |this| all in the same
order
> relative to the OnDeviceScaleFactorChanged calls.

Done.

[hshi1, 2015-03-17 18:25:18]

The CQ bit was checked by hshi@chromium.org

[hshi1, 2015-03-17 18:25:19]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1002393002/#ps40001
(title: "address Dana's comments.")

[commit-bot: I haz the power, 2015-03-17 18:25:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1002393002/40001

[oshima, 2015-03-17 18:29:56]

lgtm

[commit-bot: I haz the power, 2015-03-17 19:00:45]

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

[commit-bot: I haz the power, 2015-03-17 19:00:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hshi1, 2015-03-17 20:37:43]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2015-03-17 20:38:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1002393002/40001

[commit-bot: I haz the power, 2015-03-17 21:07:09]

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

[commit-bot: I haz the power, 2015-03-17 21:07:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hshi1, 2015-03-17 21:11:59]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2015-03-17 21:13:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1002393002/40001

[commit-bot: I haz the power, 2015-03-17 21:18:43]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 21:19:47]

Patchset 3 (id:??) landed as
https://crrev.com/270f62296ebb49c6bcaff3dc8715346d3f0d321a
Cr-Commit-Position: refs/heads/master@{#320974}