Add a Canvas::SaveAndUnscale() method for pixel-accurate drawing.

Add a Canvas::SaveAndUnscale() method for pixel-accurate drawing.

This will be useful for various Material Design implementation bits.

BUG=none
TEST=none

Committed: https://crrev.com/0ca0ebd49346783db18dd53c493c1e39a93826bc
Cr-Commit-Position: refs/heads/master@{#353946}

[Peter Kasting, 2015-10-13 22:13:50]

pkasting@chromium.org changed reviewers:
+ danakj@chromium.org, pkotwicz@chromium.org, sky@chromium.org

[Peter Kasting, 2015-10-13 22:13:50]

pkotwicz: ash/frame/ OWNERS
sky: chrome/browser/ui/views/ OWNERS
danakj: ui/gfx/ OWNERS

[sky, 2015-10-13 22:23:11]

https://codereview.chromium.org/1393013007/diff/1/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/1393013007/diff/1/ui/gfx/canvas.cc#newcode183
ui/gfx/canvas.cc:183: float Canvas::SaveAndUnscale() {
How expensive is save? The common case is scale=1. Is it worth optimizing that?

https://codereview.chromium.org/1393013007/diff/1/ui/gfx/canvas.cc#newcode185
ui/gfx/canvas.cc:185: SkScalar scale_factor = 1.0f / image_scale_;
I'm not familiar enough with what we want to happen, but did you consider
unscaling by the current transform?

[Peter Kasting, 2015-10-13 22:34:42]

https://codereview.chromium.org/1393013007/diff/1/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

https://codereview.chromium.org/1393013007/diff/1/ui/gfx/canvas.cc#newcode183
ui/gfx/canvas.cc:183: float Canvas::SaveAndUnscale() {
On 2015/10/13 22:23:11, sky wrote:
> How expensive is save? The common case is scale=1. Is it worth optimizing
that?

Save() is very cheap.  Also we can't get rid of it when the scale factor is 1
because callers are going to unconditionally Restore().

scale(1, 1) is pretty cheap as well, so I didn't bother to try to optimize it
away.

https://codereview.chromium.org/1393013007/diff/1/ui/gfx/canvas.cc#newcode185
ui/gfx/canvas.cc:185: SkScalar scale_factor = 1.0f / image_scale_;
On 2015/10/13 22:23:11, sky wrote:
> I'm not familiar enough with what we want to happen, but did you consider
> unscaling by the current transform?

I did, but I'm not convinced that's what we actually want.  We want to undo the
"device pixel -> DIP" transformation, but imagine if some other caller, e.g. a
"magnifying glass effect" implementation, had added an additional scale atop
this.  We'd still want that scale to apply to the current canvas.

[danakj, 2015-10-13 22:40:38]

This is at odds with how painting is done today. We have painting done in a
scale-agnostic way so it works and scales appropriately at different DSF.

What happens here if they are drawing in physical pixels? Now the painting code
has to resize things to be larger if the scale is too large? Isn't this going to
make for very complicated paint code?

[Peter Kasting, 2015-10-13 22:44:24]

On 2015/10/13 22:40:38, danakj wrote:
> This is at odds with how painting is done today. We have painting done in a
> scale-agnostic way so it works and scales appropriately at different DSF.

Unfortunately, that has problems, and my understanding is that we're eventually
going to entirely remove it so views are always in device pixels, not in DIPs.

In any case, we need the ability to draw directly in device pixels now in a
small but growing number of locations.

> What happens here if they are drawing in physical pixels? Now the painting
code
> has to resize things to be larger if the scale is too large? Isn't this going
to
> make for very complicated paint code?

I don't understand your question.

Code that wants to call this method is doing so because it wants to draw
directly in device pixels and understands what it needs to do in order to make
that look correct.  Look at how browser_view.cc works, for example.

For the tabstrip MD rework, there's no good way to draw without this, because we
want lines that are a precise number of device pixels (not DIPs) in any scale
factor, and we need to adjust curve shapes and the like differently for every
scale factor.  It's complicated, yes, but it's complicated because the needs are
complicated.

[sky, 2015-10-13 23:09:36]

LGTM

[sky, 2015-10-13 23:18:51]

On 2015/10/13 22:44:24, Peter Kasting wrote:
> On 2015/10/13 22:40:38, danakj wrote:
> > This is at odds with how painting is done today. We have painting done in a
> > scale-agnostic way so it works and scales appropriately at different DSF.
> 
> Unfortunately, that has problems, and my understanding is that we're
eventually
> going to entirely remove it so views are always in device pixels, not in DIPs.
> 
> In any case, we need the ability to draw directly in device pixels now in a
> small but growing number of locations.
> 
> > What happens here if they are drawing in physical pixels? Now the painting
> code
> > has to resize things to be larger if the scale is too large? Isn't this
going
> to
> > make for very complicated paint code?
> 
> I don't understand your question.
> 
> Code that wants to call this method is doing so because it wants to draw
> directly in device pixels and understands what it needs to do in order to make
> that look correct.  Look at how browser_view.cc works, for example.
> 
> For the tabstrip MD rework, there's no good way to draw without this, because
we
> want lines that are a precise number of device pixels (not DIPs) in any scale
> factor, and we need to adjust curve shapes and the like differently for every
> scale factor.  It's complicated, yes, but it's complicated because the needs
are
> complicated.

Be warned that getting drawing that spans multiple views to align nicely at
fractional scales is going to be painful. I'm specifically thinking of cases
where you want a container to draw something that is aligned with drawing in a
child, e.g. the parent drawing a line that stops at a child and having the child
continue to draw the line.

[Peter Kasting, 2015-10-13 23:23:45]

On 2015/10/13 23:18:51, sky wrote:
> Be warned that getting drawing that spans multiple views to align nicely at
> fractional scales is going to be painful. I'm specifically thinking of cases
> where you want a container to draw something that is aligned with drawing in a
> child, e.g. the parent drawing a line that stops at a child and having the
child
> continue to draw the line.

The MD tabstrip/toolbar separator is actually drawn cooperatively by three
separate classes.  Alignment wasn't too bad, luckily.

[danakj, 2015-10-13 23:26:11]

On 2015/10/13 22:44:24, Peter Kasting wrote:
> On 2015/10/13 22:40:38, danakj wrote:
> > This is at odds with how painting is done today. We have painting done in a
> > scale-agnostic way so it works and scales appropriately at different DSF.
> 
> Unfortunately, that has problems, and my understanding is that we're
eventually
> going to entirely remove it so views are always in device pixels, not in DIPs.
> 
> In any case, we need the ability to draw directly in device pixels now in a
> small but growing number of locations.

OK that's interesting. Blink is going similar ways now, with oshima implementing
device scale via page zoom mechanics. (and only device scale, like you're doing
here.)

I'm not sure what the transition looks like to try get all the UI into
scale-specific code and how to find random pieces that aren't converted yet.
Making this an opt-in for now probably makes sense and we can figure something
out later. LGTM

> > What happens here if they are drawing in physical pixels? Now the painting
> code
> > has to resize things to be larger if the scale is too large? Isn't this
going
> to
> > make for very complicated paint code?
> 
> I don't understand your question.
> 
> Code that wants to call this method is doing so because it wants to draw
> directly in device pixels and understands what it needs to do in order to make
> that look correct.  Look at how browser_view.cc works, for example.
> 
> For the tabstrip MD rework, there's no good way to draw without this, because
we
> want lines that are a precise number of device pixels (not DIPs) in any scale
> factor, and we need to adjust curve shapes and the like differently for every
> scale factor.  It's complicated, yes, but it's complicated because the needs
are
> complicated.

[pkotwicz, 2015-10-14 01:21:43]

ash/frame LGTM

[pkotwicz, 2015-10-14 01:21:48]

ash/frame LGTM

[Peter Kasting, 2015-10-14 01:36:29]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-10-14 01:37:35]

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

[commit-bot: I haz the power, 2015-10-14 02:19:11]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-14 02:20:40]

Patchset 1 (id:??) landed as
https://crrev.com/0ca0ebd49346783db18dd53c493c1e39a93826bc
Cr-Commit-Position: refs/heads/master@{#353946}