cc: 9patch: fix shadow scaling issue

cc: 9patch: fix scaling shadow issue

The NinePatchLayer receives parameters in different coordinate spaces
(source image space & destination layer space).

Unfortunately 8b134c7492731333fabeda5e358f0604ae5f565e made the assumption
that scaling factors for both coordinate system would be the same. This
isn't true as the image space might be scaled on hidpi display while the
layer space is not yet scaled for hidpi, this happens later after the quads
are emitted.

BUG=607033
TEST=verify windows' shadows are visible on hidpi display (for example on link)
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[llandwerlin-old, 2016-04-29 17:10:44]

Description was changed from

==========
cc: 9patch: fix scaling shadow scaling issue

The NinePatchLayer receives parameters in different coordinate spaces
(source image space & destination layer space).

Unfortunately 8b134c7492731333fabeda5e358f0604ae5f565e made the assumption
that scaling factors for both coordinate system would be the same. This
isn't true as the image space might be scaled on hidpi display while the
layer space is not yet scaled for hidpi, this happens later after the quads
are emitted.

BUG=607033
TEST=verify windows' shadows are visible on hidpi display (for example on link)
==========

to

==========
cc: 9patch: fix scaling shadow scaling issue

The NinePatchLayer receives parameters in different coordinate spaces
(source image space & destination layer space).

Unfortunately 8b134c7492731333fabeda5e358f0604ae5f565e made the assumption
that scaling factors for both coordinate system would be the same. This
isn't true as the image space might be scaled on hidpi display while the
layer space is not yet scaled for hidpi, this happens later after the quads
are emitted.

BUG=607033
TEST=verify windows' shadows are visible on hidpi display (for example on link)
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[llandwerlin-old, 2016-04-29 17:14:16]

lionel.g.landwerlin@intel.com changed reviewers:
+ aelias@chromium.org, oshima@chromium.org, sky@chromium.org

[llandwerlin-old, 2016-04-29 17:14:16]

aelias@chromium.org: Please review changes in cc/

sky@chromium.org: Please review changes in ui/

oshima@: Tested this with all the resolution on link

[llandwerlin-old, 2016-04-29 17:14:29]

Description was changed from

==========
cc: 9patch: fix scaling shadow scaling issue

The NinePatchLayer receives parameters in different coordinate spaces
(source image space & destination layer space).

Unfortunately 8b134c7492731333fabeda5e358f0604ae5f565e made the assumption
that scaling factors for both coordinate system would be the same. This
isn't true as the image space might be scaled on hidpi display while the
layer space is not yet scaled for hidpi, this happens later after the quads
are emitted.

BUG=607033
TEST=verify windows' shadows are visible on hidpi display (for example on link)
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: 9patch: fix scaling shadow issue

The NinePatchLayer receives parameters in different coordinate spaces
(source image space & destination layer space).

Unfortunately 8b134c7492731333fabeda5e358f0604ae5f565e made the assumption
that scaling factors for both coordinate system would be the same. This
isn't true as the image space might be scaled on hidpi display while the
layer space is not yet scaled for hidpi, this happens later after the quads
are emitted.

BUG=607033
TEST=verify windows' shadows are visible on hidpi display (for example on link)
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[oshima, 2016-04-29 17:38:59]

On 2016/04/29 17:14:16, llandwerlin wrote:
> mailto:aelias@chromium.org: Please review changes in cc/
> 
> mailto:sky@chromium.org: Please review changes in ui/
> 
> oshima@: Tested this with all the resolution on link

Still looks broken with DSF=1.25 case. (see my comment in the bug)

[llandwerlin-old, 2016-04-29 18:10:35]

On 2016/04/29 17:38:59, oshima wrote:
> On 2016/04/29 17:14:16, llandwerlin wrote:
> > mailto:aelias@chromium.org: Please review changes in cc/
> > 
> > mailto:sky@chromium.org: Please review changes in ui/
> > 
> > oshima@: Tested this with all the resolution on link
> 
> Still looks broken with DSF=1.25 case. (see my comment in the bug)

Would that be equivalent to 1920x1275 on link?

[oshima, 2016-04-29 18:11:55]

On 2016/04/29 18:10:35, llandwerlin wrote:
> On 2016/04/29 17:38:59, oshima wrote:
> > On 2016/04/29 17:14:16, llandwerlin wrote:
> > > mailto:aelias@chromium.org: Please review changes in cc/
> > > 
> > > mailto:sky@chromium.org: Please review changes in ui/
> > > 
> > > oshima@: Tested this with all the resolution on link
> > 
> > Still looks broken with DSF=1.25 case. (see my comment in the bug)
> 
> Would that be equivalent to 1920x1275 on link?

No. It's used in FHD (1920x1080) devices.

[llandwerlin-old, 2016-04-29 18:43:32]

On 2016/04/29 18:11:55, oshima wrote:
> On 2016/04/29 18:10:35, llandwerlin wrote:
> > On 2016/04/29 17:38:59, oshima wrote:
> > > On 2016/04/29 17:14:16, llandwerlin wrote:
> > > > mailto:aelias@chromium.org: Please review changes in cc/
> > > > 
> > > > mailto:sky@chromium.org: Please review changes in ui/
> > > > 
> > > > oshima@: Tested this with all the resolution on link
> > > 
> > > Still looks broken with DSF=1.25 case. (see my comment in the bug)
> > 
> > Would that be equivalent to 1920x1275 on link?
> 
> No. It's used in FHD (1920x1080) devices.

I've tried --ash-host-window-bounds=1536x864*1.25 on an dell xps 13 with a 1080p
screen, but screen remains blank. Is this how you're testing or just on desktop?

[oshima, 2016-04-29 18:45:58]

On 2016/04/29 18:43:32, llandwerlin wrote:
> On 2016/04/29 18:11:55, oshima wrote:
> > On 2016/04/29 18:10:35, llandwerlin wrote:
> > > On 2016/04/29 17:38:59, oshima wrote:
> > > > On 2016/04/29 17:14:16, llandwerlin wrote:
> > > > > mailto:aelias@chromium.org: Please review changes in cc/
> > > > > 
> > > > > mailto:sky@chromium.org: Please review changes in ui/
> > > > > 
> > > > > oshima@: Tested this with all the resolution on link
> > > > 
> > > > Still looks broken with DSF=1.25 case. (see my comment in the bug)
> > > 
> > > Would that be equivalent to 1920x1275 on link?
> > 
> > No. It's used in FHD (1920x1080) devices.
> 
> I've tried --ash-host-window-bounds=1536x864*1.25 on an dell xps 13 with a
1080p
> screen, but screen remains blank. Is this how you're testing or just on
desktop?

that flag is only for developing on desktop use, and the behavior on device is
undefined.

[oshima, 2016-04-29 18:46:00]

On 2016/04/29 18:43:32, llandwerlin wrote:
> On 2016/04/29 18:11:55, oshima wrote:
> > On 2016/04/29 18:10:35, llandwerlin wrote:
> > > On 2016/04/29 17:38:59, oshima wrote:
> > > > On 2016/04/29 17:14:16, llandwerlin wrote:
> > > > > mailto:aelias@chromium.org: Please review changes in cc/
> > > > > 
> > > > > mailto:sky@chromium.org: Please review changes in ui/
> > > > > 
> > > > > oshima@: Tested this with all the resolution on link
> > > > 
> > > > Still looks broken with DSF=1.25 case. (see my comment in the bug)
> > > 
> > > Would that be equivalent to 1920x1275 on link?
> > 
> > No. It's used in FHD (1920x1080) devices.
> 
> I've tried --ash-host-window-bounds=1536x864*1.25 on an dell xps 13 with a
1080p
> screen, but screen remains blank. Is this how you're testing or just on
desktop?

that flag is only for developing on desktop use, and the behavior on device is
undefined.

[llandwerlin-old, 2016-05-03 11:42:20]

PTAL, Thanks.

[aelias_OOO_until_Jul13, 2016-05-05 04:17:57]

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
cc/layers/nine_patch_layer_impl.cc:87: if (border_.x() == 0 || border_.y() == 0
||
All this state is known to the client-side code in ui/ so can you move this
logic there and leave NinePatchLayerImpl simply checking
(layer_occlusion_.IsEmpty())?  This will make NinePatchLayerImpl's behavior more
predictable (in the sense that it will never silently drop the occlusion, which
would regress performance in a hard-to-notice way).

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
File cc/layers/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
cc/layers/nine_patch_layer_impl_unittest.cc:34: bool ShouldUseOcclusion(const
gfx::Size& bitmap_size,
It's a bad practice to copy-paste production code into a test.  This is moot if
you follow my suggestion above, but I would've suggested instead adding "bool
shouldUseOcclusion" argument to the helper method and have each caller pass it
in depending on the expected result.

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
cc/layers/nine_patch_layer_impl_unittest.cc:72:
expected_layer_remaining.SetRect(
All this is too much logic for a test, this makes it difficult to reason about
what the test is intending to check.  Please just write in literal values for
the output of this algorithm instead.  This probably calls for creating a new
helper method NinePatchLayerLayoutWithOcclusionTest() that takes more arguments.

[llandwerlin-old, 2016-05-06 16:26:48]

PTAL.

Sorry for this late update.
I spent too much time on corner cases that (as aelias noted) are not relevant.
Added a check sanity check in CheckGeometryLimitations().

Thanks!

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
File cc/layers/nine_patch_layer_impl.cc (right):

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
cc/layers/nine_patch_layer_impl.cc:87: if (border_.x() == 0 || border_.y() == 0
||
On 2016/05/05 04:17:57, aelias_OOO_until_may9 wrote:
> All this state is known to the client-side code in ui/ so can you move this
> logic there and leave NinePatchLayerImpl simply checking
> (layer_occlusion_.IsEmpty())?  This will make NinePatchLayerImpl's behavior
more
> predictable (in the sense that it will never silently drop the occlusion,
which
> would regress performance in a hard-to-notice way).

Done.

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
File cc/layers/nine_patch_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
cc/layers/nine_patch_layer_impl_unittest.cc:34: bool ShouldUseOcclusion(const
gfx::Size& bitmap_size,
On 2016/05/05 04:17:57, aelias_OOO_until_may9 wrote:
> It's a bad practice to copy-paste production code into a test.  This is moot
if
> you follow my suggestion above, but I would've suggested instead adding "bool
> shouldUseOcclusion" argument to the helper method and have each caller pass it
> in depending on the expected result.

Done.

https://codereview.chromium.org/1937493002/diff/40001/cc/layers/nine_patch_la...
cc/layers/nine_patch_layer_impl_unittest.cc:72:
expected_layer_remaining.SetRect(
On 2016/05/05 04:17:57, aelias_OOO_until_may9 wrote:
> All this is too much logic for a test, this makes it difficult to reason about
> what the test is intending to check.  Please just write in literal values for
> the output of this algorithm instead.  This probably calls for creating a new
> helper method NinePatchLayerLayoutWithOcclusionTest() that takes more
arguments.

Done.

[aelias_OOO_until_Jul13, 2016-05-07 05:25:14]

lgtm, thanks!

[llandwerlin-old, 2016-05-12 10:36:06]

oshima@, sky@ : ping?

[oshima, 2016-05-13 17:32:50]

I tested the patch at 2x, and the bottom left/right corners have no rounding.

Can you also add tests for 2x/1.5x case?

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

https://codereview.chromium.org/1937493002/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:630: DCHECK(type_ == LAYER_NINE_PATCH &&
nine_patch_layer_.get());
DCHECK_EQ

[llandwerlin-old, 2016-05-16 09:42:45]

Patchset #5 (id:80001) has been deleted

[llandwerlin-old, 2016-05-16 10:05:09]

Sorry I didn't noticed the lower corners.

The rounding corners come from the shadow's image, so we need to overdraw on the
window just by the size of the image's aperture.

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

https://codereview.chromium.org/1937493002/diff/60001/ui/compositor/layer.cc#...
ui/compositor/layer.cc:630: DCHECK(type_ == LAYER_NINE_PATCH &&
nine_patch_layer_.get());
On 2016/05/13 17:32:50, oshima wrote:
> DCHECK_EQ

Done.

[llandwerlin-old, 2016-05-16 10:07:08]

Patchset #5 (id:100001) has been deleted

[oshima, 2016-05-16 17:31:52]

On 2016/05/16 10:05:09, llandwerlin wrote:
> Sorry I didn't noticed the lower corners.
> 
> The rounding corners come from the shadow's image, so we need to overdraw on
the
> window just by the size of the image's aperture.
> 
> https://codereview.chromium.org/1937493002/diff/60001/ui/compositor/layer.cc
> File ui/compositor/layer.cc (right):
> 
>
https://codereview.chromium.org/1937493002/diff/60001/ui/compositor/layer.cc#...
> ui/compositor/layer.cc:630: DCHECK(type_ == LAYER_NINE_PATCH &&
> nine_patch_layer_.get());
> On 2016/05/13 17:32:50, oshima wrote:
> > DCHECK_EQ
> 
> Done.

The corners looks okay now. Can yo add a test that can catch this issue?

I'll leave the rest to owners as I'm not the owner.