Change ui::wm::Shadow to use cc::NinePatchLayer.

Change ui::wm::Shadow to use cc::NinePatchLayer.

Added combined shadow assets used by the class ui::wm::Shadow and remove the unneeded
assets for individual shadow patches.

BUG=377319
TEST=run 1.0x, 1.25x and 2.0x scale factor and verify all shadows are rendered correctly.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283558

[hshi1, 2014-07-11 20:59:40]

PTAL

[hshi1, 2014-07-11 22:47:40]

PTAL patch set #2. I talked to oshima@ and realized that the aperture/border
needs to be separate from the inset.

With the new patch I have verified that the inner rounded corner is correctly
rendered (with a test shadow image that has bright colorful corner colors)

[oshima, 2014-07-11 23:03:41]

thanks. lgtm with a nit.

https://codereview.chromium.org/385123005/diff/1/ui/resources/ui_resources.grd
File ui/resources/ui_resources.grd (right):

https://codereview.chromium.org/385123005/diff/1/ui/resources/ui_resources.gr...
ui/resources/ui_resources.grd:106: <structure type="chrome_scaled_image"
name="IDR_AURA_SHADOW_INACTIVE" file="common/window_shadow_inactive.png" />
nice!

https://codereview.chromium.org/385123005/diff/20001/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (right):

https://codereview.chromium.org/385123005/diff/20001/ui/wm/core/shadow.cc#new...
ui/wm/core/shadow.cc:20: // Shadow aperture for different styles.
can you add comment that why we're using different insets and aperture values?

[hshi1, 2014-07-11 23:18:03]

+sky (OWNERS of ui/)

https://codereview.chromium.org/385123005/diff/20001/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (right):

https://codereview.chromium.org/385123005/diff/20001/ui/wm/core/shadow.cc#new...
ui/wm/core/shadow.cc:20: // Shadow aperture for different styles.
On 2014/07/11 23:03:41, oshima wrote:
> can you add comment that why we're using different insets and aperture values?

Done.

[sky, 2014-07-14 15:30:06]

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

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:592: SetFillsBoundsOpaquely(false);
Why the SetFillsBoundsOpaquely here? Isn't that separate from whether this layer
is drawing a nine-patch?

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:601: if (!nine_patch_layer_.get())
This be a DCHECK, right?

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:277: void SetShowNinePatch();
I would have expected nine patch to be a new type, eg LAYER_TYPE_NINE_PATCH. Is
there a reason it isn't?

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:279: void UpdateNinePatchBitmap(const SkBitmap& bitmap,
You need to add descriptions of the arguments.

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (left):

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc#old...
ui/wm/core/shadow.cc:140: &res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE_TOP_LEFT),
This is the more common style used in chrome. See also
ui/gfx/nine_image_painter. Is there a reason for not going with separate images?
The advantage of separate images is that updated them doesn't require updating
code (the apeture values you have I believe).

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.h
File ui/wm/core/shadow.h (right):

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.h#newc...
ui/wm/core/shadow.h:65: void UpdateImageBounds();
UpdateLayerBounds

[ccameron, 2014-07-14 17:32:06]

This lgtm w sky's remarks.

[hshi1, 2014-07-14 21:23:38]

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

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:592: SetFillsBoundsOpaquely(false);
On 2014/07/14 15:30:06, sky wrote:
> Why the SetFillsBoundsOpaquely here? Isn't that separate from whether this
layer
> is drawing a nine-patch?

Done. I'm moving this call the layer owner (Shadow).

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:601: if (!nine_patch_layer_.get())
On 2014/07/14 15:30:06, sky wrote:
> This be a DCHECK, right?

Done.

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:277: void SetShowNinePatch();
On 2014/07/14 15:30:06, sky wrote:
> I would have expected nine patch to be a new type, eg LAYER_TYPE_NINE_PATCH.
Is
> there a reason it isn't?

Done.

https://codereview.chromium.org/385123005/diff/40001/ui/compositor/layer.h#ne...
ui/compositor/layer.h:279: void UpdateNinePatchBitmap(const SkBitmap& bitmap,
On 2014/07/14 15:30:06, sky wrote:
> You need to add descriptions of the arguments.

Done.

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (left):

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc#old...
ui/wm/core/shadow.cc:140: &res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE_TOP_LEFT),
On 2014/07/14 15:30:06, sky wrote:
> This is the more common style used in chrome. See also
> ui/gfx/nine_image_painter. Is there a reason for not going with separate
images?
> The advantage of separate images is that updated them doesn't require updating
> code (the apeture values you have I believe).

While I agree of the advantage of separate images, the existing design of
cc::NinePatchLayer requires a single image. My understand is that (ccameron@
please correct me) the NinePatch layer implementation takes advantage of the
single image to allow seamless scaling with fractional scaling factors such as
1.25x -- something very hard to do with multiple separate images.

If we go with separate images then we'll need a wrapper to combine these images
at run time, and allocate a temporary buffer to hold the combined images (for
every style). It feels like the unnecessary complexity outweighs the benefits.

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.h
File ui/wm/core/shadow.h (right):

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.h#newc...
ui/wm/core/shadow.h:65: void UpdateImageBounds();
On 2014/07/14 15:30:06, sky wrote:
> UpdateLayerBounds

Done.

[oshima, 2014-07-14 22:02:32]

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc
File ui/wm/core/shadow.cc (left):

https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc#old...
ui/wm/core/shadow.cc:140: &res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE_TOP_LEFT),
On 2014/07/14 21:23:37, hshi1 wrote:
> On 2014/07/14 15:30:06, sky wrote:
> > This is the more common style used in chrome. See also
> > ui/gfx/nine_image_painter. Is there a reason for not going with separate
> images?
> > The advantage of separate images is that updated them doesn't require
updating
> > code (the apeture values you have I believe).
> 
> While I agree of the advantage of separate images, the existing design of
> cc::NinePatchLayer requires a single image. My understand is that (ccameron@
> please correct me) the NinePatch layer implementation takes advantage of the
> single image to allow seamless scaling with fractional scaling factors such as
> 1.25x -- something very hard to do with multiple separate images.
> 
> If we go with separate images then we'll need a wrapper to combine these
images
> at run time, and allocate a temporary buffer to hold the combined images (for
> every style). It feels like the unnecessary complexity outweighs the benefits.

I actually prefer single image and would like nine painter to adopt it. It's
less error prone (no way to mix up wrong images, and they're consistent), 
easy to maintain (one image vs 8 images), and simpler code.

The code update issue can be addressed by embedding this information in the
image like
Android does.

Note that this particular patch uses the nine patch in special way,
so it can require the code change even if we uses separate images.
(aperture and insets are different)

[sky, 2014-07-15 04:11:11]

On Mon, Jul 14, 2014 at 3:02 PM, <oshima@chromium.org> wrote:

>
> https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc
> File ui/wm/core/shadow.cc (left):
>
> https://codereview.chromium.org/385123005/diff/40001/ui/
> wm/core/shadow.cc#oldcode140
> ui/wm/core/shadow.cc:140:
> &res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE_TOP_LEFT),
> On 2014/07/14 21:23:37, hshi1 wrote:
>
>> On 2014/07/14 15:30:06, sky wrote:
>> > This is the more common style used in chrome. See also
>> > ui/gfx/nine_image_painter. Is there a reason for not going with
>>
> separate
>
>> images?
>> > The advantage of separate images is that updated them doesn't
>>
> require updating
>
>> > code (the apeture values you have I believe).
>>
>
>  While I agree of the advantage of separate images, the existing design
>>
> of
>
>> cc::NinePatchLayer requires a single image. My understand is that
>>
> (ccameron@
>
>> please correct me) the NinePatch layer implementation takes advantage
>>
> of the
>
>> single image to allow seamless scaling with fractional scaling factors
>>
> such as
>
>> 1.25x -- something very hard to do with multiple separate images.
>>
>
>  If we go with separate images then we'll need a wrapper to combine
>>
> these images
>
>> at run time, and allocate a temporary buffer to hold the combined
>>
> images (for
>
>> every style). It feels like the unnecessary complexity outweighs the
>>
> benefits.
>
> I actually prefer single image and would like nine painter to adopt it.
> It's
> less error prone (no way to mix up wrong images, and they're
> consistent),
> easy to maintain (one image vs 8 images), and simpler code.
>
> The code update issue can be addressed by embedding this information in
> the image like
> Android does.
>
> Note that this particular patch uses the nine patch in special way,
> so it can require the code change even if we uses separate images.
> (aperture and insets are different)


I seem to remember some of the designers complaining about a single image
and the need to use a separate tool to update meta information. The
designers are going to be the ones that maintain these images, so it's
worth talking with them for their perspective. That said, I do like a
single image for the reasons you mention here too, but I want to see how
embedding extra info works in practice. Can we embed the extra state needed
here in the image too?

  -Scott

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hshi1, 2014-07-15 16:41:43]

On 2014/07/15 04:11:11, sky wrote:
> On Mon, Jul 14, 2014 at 3:02 PM, <mailto:oshima@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc
> > File ui/wm/core/shadow.cc (left):
> >
> > https://codereview.chromium.org/385123005/diff/40001/ui/
> > wm/core/shadow.cc#oldcode140
> > ui/wm/core/shadow.cc:140:
> > &res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE_TOP_LEFT),
> > On 2014/07/14 21:23:37, hshi1 wrote:
> >
> >> On 2014/07/14 15:30:06, sky wrote:
> >> > This is the more common style used in chrome. See also
> >> > ui/gfx/nine_image_painter. Is there a reason for not going with
> >>
> > separate
> >
> >> images?
> >> > The advantage of separate images is that updated them doesn't
> >>
> > require updating
> >
> >> > code (the apeture values you have I believe).
> >>
> >
> >  While I agree of the advantage of separate images, the existing design
> >>
> > of
> >
> >> cc::NinePatchLayer requires a single image. My understand is that
> >>
> > (ccameron@
> >
> >> please correct me) the NinePatch layer implementation takes advantage
> >>
> > of the
> >
> >> single image to allow seamless scaling with fractional scaling factors
> >>
> > such as
> >
> >> 1.25x -- something very hard to do with multiple separate images.
> >>
> >
> >  If we go with separate images then we'll need a wrapper to combine
> >>
> > these images
> >
> >> at run time, and allocate a temporary buffer to hold the combined
> >>
> > images (for
> >
> >> every style). It feels like the unnecessary complexity outweighs the
> >>
> > benefits.
> >
> > I actually prefer single image and would like nine painter to adopt it.
> > It's
> > less error prone (no way to mix up wrong images, and they're
> > consistent),
> > easy to maintain (one image vs 8 images), and simpler code.
> >
> > The code update issue can be addressed by embedding this information in
> > the image like
> > Android does.
> >
> > Note that this particular patch uses the nine patch in special way,
> > so it can require the code change even if we uses separate images.
> > (aperture and insets are different)
> 
> 
> I seem to remember some of the designers complaining about a single image
> and the need to use a separate tool to update meta information. The
> designers are going to be the ones that maintain these images, so it's
> worth talking with them for their perspective. That said, I do like a
> single image for the reasons you mention here too, but I want to see how
> embedding extra info works in practice. Can we embed the extra state needed
> here in the image too?
> 
>   -Scott
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I have CCed sgabriel@ on the bug. But I am not aware of any method to embed the
additional information (inset and aperture values) in the image itself.

[hshi1, 2014-07-15 17:05:49]

per sgabriel (see bug comments) this is an acceptable solution for designers.
thanks!

[oshima, 2014-07-15 17:20:38]

On 2014/07/15 16:41:43, hshi1 wrote:
> On 2014/07/15 04:11:11, sky wrote:
> > On Mon, Jul 14, 2014 at 3:02 PM, <mailto:oshima@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/385123005/diff/40001/ui/wm/core/shadow.cc
> > > File ui/wm/core/shadow.cc (left):
> > >
> > > https://codereview.chromium.org/385123005/diff/40001/ui/
> > > wm/core/shadow.cc#oldcode140
> > > ui/wm/core/shadow.cc:140:
> > > &res.GetImageNamed(IDR_AURA_SHADOW_ACTIVE_TOP_LEFT),
> > > On 2014/07/14 21:23:37, hshi1 wrote:
> > >
> > >> On 2014/07/14 15:30:06, sky wrote:
> > >> > This is the more common style used in chrome. See also
> > >> > ui/gfx/nine_image_painter. Is there a reason for not going with
> > >>
> > > separate
> > >
> > >> images?
> > >> > The advantage of separate images is that updated them doesn't
> > >>
> > > require updating
> > >
> > >> > code (the apeture values you have I believe).
> > >>
> > >
> > >  While I agree of the advantage of separate images, the existing design
> > >>
> > > of
> > >
> > >> cc::NinePatchLayer requires a single image. My understand is that
> > >>
> > > (ccameron@
> > >
> > >> please correct me) the NinePatch layer implementation takes advantage
> > >>
> > > of the
> > >
> > >> single image to allow seamless scaling with fractional scaling factors
> > >>
> > > such as
> > >
> > >> 1.25x -- something very hard to do with multiple separate images.
> > >>
> > >
> > >  If we go with separate images then we'll need a wrapper to combine
> > >>
> > > these images
> > >
> > >> at run time, and allocate a temporary buffer to hold the combined
> > >>
> > > images (for
> > >
> > >> every style). It feels like the unnecessary complexity outweighs the
> > >>
> > > benefits.
> > >
> > > I actually prefer single image and would like nine painter to adopt it.
> > > It's
> > > less error prone (no way to mix up wrong images, and they're
> > > consistent),
> > > easy to maintain (one image vs 8 images), and simpler code.
> > >
> > > The code update issue can be addressed by embedding this information in
> > > the image like
> > > Android does.
> > >
> > > Note that this particular patch uses the nine patch in special way,
> > > so it can require the code change even if we uses separate images.
> > > (aperture and insets are different)
> > 
> > 
> > I seem to remember some of the designers complaining about a single image
> > and the need to use a separate tool to update meta information. The
> > designers are going to be the ones that maintain these images, so it's
> > worth talking with them for their perspective. That said, I do like a
> > single image for the reasons you mention here too, but I want to see how
> > embedding extra info works in practice. Can we embed the extra state needed
> > here in the image too?
> > 
> >   -Scott
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> I have CCed sgabriel@ on the bug. But I am not aware of any method to embed
the
> additional information (inset and aperture values) in the image itself.

I'll work on it, as it also requires some change on Image loading and resource
management side.
We can take and update the ccameron's script to embed such information.

I'll go with the same format as android although It will be a subset of android
spec as android does more than what we need. (see
http://blog.fenrir-inc.com/us/2012/02/android-design-enlarge-images-with-nine...)

[sky, 2014-07-15 17:52:20]

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

https://codereview.chromium.org/385123005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:602: if (type_ == LAYER_SOLID_COLOR || (!delegate_ &&
!mailbox_.IsValid()))
Should SchedulePaint do nothing for nine patches too?

[danakj, 2014-07-15 17:53:34]

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

https://codereview.chromium.org/385123005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:591: if (!nine_patch_layer_.get())
You DCHECK'd this won't happen, so please don't try to handle it with code also.

[sky, 2014-07-15 17:55:19]

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

https://codereview.chromium.org/385123005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:591: if (!nine_patch_layer_.get())
On 2014/07/15 17:53:34, danakj wrote:
> You DCHECK'd this won't happen, so please don't try to handle it with code
also.

It would also be nice if you could dcheck on the type_.

[hshi1, 2014-07-15 18:16:39]

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

https://codereview.chromium.org/385123005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:591: if (!nine_patch_layer_.get())
On 2014/07/15 17:55:18, sky wrote:
> On 2014/07/15 17:53:34, danakj wrote:
> > You DCHECK'd this won't happen, so please don't try to handle it with code
> also.
> 
> It would also be nice if you could dcheck on the type_.

Done.

https://codereview.chromium.org/385123005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:591: if (!nine_patch_layer_.get())
On 2014/07/15 17:53:34, danakj wrote:
> You DCHECK'd this won't happen, so please don't try to handle it with code
also.

Done.

https://codereview.chromium.org/385123005/diff/80001/ui/compositor/layer.cc#n...
ui/compositor/layer.cc:602: if (type_ == LAYER_SOLID_COLOR || (!delegate_ &&
!mailbox_.IsValid()))
On 2014/07/15 17:52:20, sky wrote:
> Should SchedulePaint do nothing for nine patches too?

Done.

[Ken Russell (switch to Gerrit), 2014-07-15 19:04:35]

Note: the WebGLContextLostFromSelectElement failure looks real, and related to
this patch. It stresses context loss in the compositor. Please tell me if you
think there's a problem in the test or if you have trouble reproducing it.

[hshi1, 2014-07-15 19:13:09]

On 2014/07/15 19:04:35, Ken Russell wrote:
> Note: the WebGLContextLostFromSelectElement failure looks real, and related to
> this patch. It stresses context loss in the compositor. Please tell me if you
> think there's a problem in the test or if you have trouble reproducing it.

Thanks for catching this! Looks like we're failing a DCHECK because an immutable
bitmap is required.

[hshi1, 2014-07-16 18:16:30]

sky: PTAL - I believe PS#8 has eliminated the last few DCHECK failures in tests.

[sky, 2014-07-16 19:33:30]

LGTM

[hshi1, 2014-07-16 20:13:58]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:16:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hshi@chromium.org/385123005/140001

[hshi1, 2014-07-16 22:52:24]

The CQ bit was unchecked by hshi@chromium.org