ui: Fix NinePatchPainter to work in physical pixels.

ui: Fix NinePatchPainter to work in physical pixels.

This fixes NinePatchPainter to do layout in physical pixels after the
change to use slimming-paint in the ui compositor. Instead of using
the matrix on the canvas (which won't be meaningful for scale), it uses
the gfx::Canvas::SaveAndUnscale() method to move into a physical space
and do layout there.

R=oshima, sky
BUG=539496
Committed: https://crrev.com/4bfbb34d989ec59f138b3589e956a3947c139ec3
Cr-Commit-Position: refs/heads/master@{#356983}

[danakj, 2015-10-28 23:13:00]

Hello, this seems to work for me for 1.25, 2.25, etc. PTAL and let me know what
I did wrong. :)

[danakj, 2015-10-28 23:13:12]

Description was changed from

==========
ui: Fix NinePatchPainter to work in physical pixels.

This fixes NinePatchPainter to do layout in physical pixels after the
change to use slimming-paint in the ui compositor. Instead of using
the matrix on the canvas (which won't be meaningful for scale), it uses
the gfx::Canvas::SaveAndUnscale() method to move into a physical space
and do layout there.

R=oshima, sky
BUG=539496
==========

to

==========
ui: Fix NinePatchPainter to work in physical pixels.

This fixes NinePatchPainter to do layout in physical pixels after the
change to use slimming-paint in the ui compositor. Instead of using
the matrix on the canvas (which won't be meaningful for scale), it uses
the gfx::Canvas::SaveAndUnscale() method to move into a physical space
and do layout there.

R=oshima, sky
BUG=539496
==========

[danakj, 2015-10-28 23:13:25]

Oh, +pkasting FYI as another SaveAndUnscale situation.

[danakj, 2015-10-28 23:13:35]

Patchset #1 (id:1) has been deleted

[danakj, 2015-10-28 23:17:11]

https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
File ui/gfx/nine_image_painter.cc (right):

https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
ui/gfx/nine_image_painter.cc:106:
canvas->Translate(gfx::Vector2d(left_in_pixels, top_in_pixels));
I couldn't find a place that actually has non-0 left/top to test that.. if you
have any ideas let me know. I thought the buttons maybe would but they're 0,0
too. Or maybe Paint() shouldn't take a rect bounds then.

[Peter Kasting, 2015-10-28 23:22:53]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-10-28 23:22:53]

https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
File ui/gfx/nine_image_painter.cc (right):

https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
ui/gfx/nine_image_painter.cc:151: if (!has_room_for_border) {
Nit: I think it would be better to do:

  if (has_room_for_border) {
    Fill(...);
    ...
  }

  canvas->Restore();
}

Alternately we could develop some sort of scoping object for Canvas that
auto-Restored on destruction, instantiate that above, and then the early return
wouldn't need a separate restore call.

[danakj, 2015-10-28 23:23:58]

On Wed, Oct 28, 2015 at 4:22 PM, <pkasting@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
> File ui/gfx/nine_image_painter.cc (right):
>
>
>
https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
> ui/gfx/nine_image_painter.cc:151: if (!has_room_for_border) {
> Nit: I think it would be better to do:
>
>   if (has_room_for_border) {
>     Fill(...);
>     ...
>   }
>
>   canvas->Restore();
> }
>
> Alternately we could develop some sort of scoping object for Canvas that
> auto-Restored on destruction, instantiate that above, and then the early
> return wouldn't need a separate restore call.
>

Ya :) I was thinking about building SaveAndUnscale into ScopedCanvas after
this CL.

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

[danakj, 2015-10-28 23:25:05]

On Wed, Oct 28, 2015 at 4:23 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 28, 2015 at 4:22 PM, <pkasting@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
>> File ui/gfx/nine_image_painter.cc (right):
>>
>>
>>
https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
>> ui/gfx/nine_image_painter.cc:151: if (!has_room_for_border) {
>> Nit: I think it would be better to do:
>>
>>   if (has_room_for_border) {
>>     Fill(...);
>>     ...
>>   }
>>
>>   canvas->Restore();
>> }
>>
>
Removed the early out for this CL as you suggested tho. I like that better.


>
>> Alternately we could develop some sort of scoping object for Canvas that
>> auto-Restored on destruction, instantiate that above, and then the early
>> return wouldn't need a separate restore call.
>>
>
> Ya :) I was thinking about building SaveAndUnscale into ScopedCanvas after
> this CL.
>

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

[sky, 2015-10-28 23:33:47]

On Wed, Oct 28, 2015 at 4:23 PM, Dana Jansens <danakj@chromium.org> wrote:

> On Wed, Oct 28, 2015 at 4:22 PM, <pkasting@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
>> File ui/gfx/nine_image_painter.cc (right):
>>
>>
>>
https://codereview.chromium.org/1424983005/diff/40001/ui/gfx/nine_image_paint...
>> ui/gfx/nine_image_painter.cc:151: if (!has_room_for_border) {
>> Nit: I think it would be better to do:
>>
>>   if (has_room_for_border) {
>>     Fill(...);
>>     ...
>>   }
>>
>>   canvas->Restore();
>> }
>>
>> Alternately we could develop some sort of scoping object for Canvas that
>> auto-Restored on destruction, instantiate that above, and then the early
>> return wouldn't need a separate restore call.
>>
>
> Ya :) I was thinking about building SaveAndUnscale into ScopedCanvas after
> this CL.
>

+1

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

[sky, 2015-10-28 23:34:31]

LGTM - make sure you test rtl as well.

[danakj, 2015-10-28 23:42:45]

On Wed, Oct 28, 2015 at 4:34 PM, <sky@chromium.org> wrote:

> LGTM - make sure you test rtl as well.
>

Seems to work right.

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

[danakj, 2015-10-28 23:43:38]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-10-28 23:44:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424983005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424983005/60001

[commit-bot: I haz the power, 2015-10-29 00:04:54]

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

[commit-bot: I haz the power, 2015-10-29 00:04:54]

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

[danakj, 2015-10-29 21:19:10]

There are tests for this! Cool, I updated the tests a bit.

They were making a ImageSkia that was not able to give back a scaled
ImageSkiaRep for the right scale. Which would cause NinePatchPainter to DCHECK.
From talking with oshima, real uses of NinePatchPainter should be giving
ImageSkias that are able to give a scaled ImageSkiaRep for any given scale. So I
made the tests use an ImageSkia with a 2x rep in them.

Also made the tests output the bitmap if they fail so you can see what they did.

Also split the test for scale into a separate test from the test for high dpi.

PTAL

[Peter Kasting, 2015-10-29 21:26:15]

LGTM

https://codereview.chromium.org/1424983005/diff/80001/ui/gfx/nine_image_paint...
File ui/gfx/nine_image_painter_unittest.cc (right):

https://codereview.chromium.org/1424983005/diff/80001/ui/gfx/nine_image_paint...
ui/gfx/nine_image_painter_unittest.cc:71: gfx::Vector2d paint_offset(40, 20); 
// Translated 20,10 with 2x dpi.
Why not store the original translation vector from above in a temp, then scale
it here by the image_scale you chose?  Then there's no need for magic numbers
with comments.

Same applies to the places below where we could programmatically calculate this
similarly.

https://codereview.chromium.org/1424983005/diff/80001/ui/gfx/nine_image_paint...
ui/gfx/nine_image_painter_unittest.cc:73: for (int y = paint_offset.y(); y < 100
+ paint_offset.y(); y++) {
You repeat this loop several times; factor out into a helper function?

[danakj, 2015-10-29 22:15:12]

https://codereview.chromium.org/1424983005/diff/80001/ui/gfx/nine_image_paint...
File ui/gfx/nine_image_painter_unittest.cc (right):

https://codereview.chromium.org/1424983005/diff/80001/ui/gfx/nine_image_paint...
ui/gfx/nine_image_painter_unittest.cc:71: gfx::Vector2d paint_offset(40, 20); 
// Translated 20,10 with 2x dpi.
On 2015/10/29 21:26:15, Peter Kasting wrote:
> Why not store the original translation vector from above in a temp, then scale
> it here by the image_scale you chose?  Then there's no need for magic numbers
> with comments.
> 
> Same applies to the places below where we could programmatically calculate
this
> similarly.

Sure, done. Except the negative one is weird, I think I need a comment there
basically to get numbers that you can both understand and get why.

https://codereview.chromium.org/1424983005/diff/80001/ui/gfx/nine_image_paint...
ui/gfx/nine_image_painter_unittest.cc:73: for (int y = paint_offset.y(); y < 100
+ paint_offset.y(); y++) {
On 2015/10/29 21:26:15, Peter Kasting wrote:
> You repeat this loop several times; factor out into a helper function?

Done.

[danakj, 2015-10-29 22:15:13]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-10-29 22:15:14]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1424983005/#ps100001
(title: "ninepatchscale: testreview")

[commit-bot: I haz the power, 2015-10-29 22:16:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1424983005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1424983005/100001

[commit-bot: I haz the power, 2015-10-29 23:38:32]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-10-29 23:39:14]

Patchset 5 (id:??) landed as
https://crrev.com/4bfbb34d989ec59f138b3589e956a3947c139ec3
Cr-Commit-Position: refs/heads/master@{#356983}