Add target scale factor to gfx::Canvas

Add ability to build a canvas with a target scale factor.
This allows ExtractBitmap() to return an ImageSkiaRep. 
This simplifies code around using intermediate bitmaps a lot.

BUG=131475
TEST=Compiles
R=oshima,sky
TBR=sadrul

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144489

[oshima, 2012-06-18 17:13:08]

http://codereview.chromium.org/10562027/diff/6003/ui/base/native_theme/native...
File ui/base/native_theme/native_theme_android.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/base/native_theme/native...
ui/base/native_theme/native_theme_android.cc:670: gfx::Canvas canvas(sk_canvas,
target_scale_factor, false);
webkit must have scale factor information as well right?
(given that you can specify 1x/2x in css)
How difficult it would be to pass scale factor from webkit?

[oshima, 2012-06-18 22:30:59]

http://codereview.chromium.org/10562027/diff/6003/ui/base/native_theme/native...
File ui/base/native_theme/native_theme_base.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/base/native_theme/native...
ui/base/native_theme/native_theme_base.cc:1020: gfx::Canvas canvas(sk_canvas,
target_scale_factor, false);
Looks like there is a similar scale factor parameter in WebKit. Can you file a
bug to pass this parameter to NativeTheme?

views should do the same (pass the scale factor of gfx::Canvas. Or just pass
gfx::Canvas.

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc#newcode79
ui/gfx/canvas.cc:79: ApplyTargetScaleFactor(target_scale_factor, scale_canvas);
what happens if scale factor is 2x, but scale_canvas == false?

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.h#newcode104
ui/gfx/canvas.h:104: // Creates canvas with provided |size| and a target scale
factor of 1x.
provided |size| in DIP

[pkotwicz, 2012-06-20 16:30:20]

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc#newcode79
ui/gfx/canvas.cc:79: ApplyTargetScaleFactor(target_scale_factor, scale_canvas);
The idea is not to apply the scale factor twice. In the case of
native_theme_base, the scale factor is already applied to the SkCanvas, so we
don't want to apply it a second time when we create a gfx::Canvas from the
SkCanvas

[oshima, 2012-06-20 18:04:40]

it's time to add OWNERS i think.

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc#newcode79
ui/gfx/canvas.cc:79: ApplyTargetScaleFactor(target_scale_factor, scale_canvas);
On 2012/06/20 16:30:20, pkotwicz wrote:
> The idea is not to apply the scale factor twice. In the case of
> native_theme_base, the scale factor is already applied to the SkCanvas, so we
> don't want to apply it a second time when we create a gfx::Canvas from the
> SkCanvas

So this is specific to NativeTheme and only use case is scale_canvas=false?
Maybe it makes more sense to provide factory method like

Canvas* Canvas::CreateUnscaledCanvas(SkCanvas*canvas, ui::ScaleFactor
scale_factor);

? I'll defer the decision to OWNER though.

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.h#newcode168
ui/gfx/canvas.h:168: gfx::ImageSkiaRep ExtractBitmap() const;
ExtractImageSkia()

I'm fine  if you want to do it in separate CL

[pkotwicz, 2012-06-20 23:07:05]

Adding Scott for his opinion

http://codereview.chromium.org/10562027/diff/6003/ui/base/native_theme/native...
File ui/base/native_theme/native_theme_android.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/base/native_theme/native...
ui/base/native_theme/native_theme_android.cc:670: gfx::Canvas canvas(sk_canvas,
target_scale_factor, false);
Its not too easy to pass this from Webkit. And we will hopefully soon not be
using images anymore in native_theme.

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc#newcode79
ui/gfx/canvas.cc:79: ApplyTargetScaleFactor(target_scale_factor, scale_canvas);
It is also used in layer.cc when scale_content_ is false.
I am ok with a factory method but CreateUnscaledCanvas is the wrong name for it.
|scale_canvas| implies that we don't apply an additional scale, not that the
SkCanvas is not scaled to begin with. Any suggestions?

[sky, 2012-06-21 00:03:32]

http://codereview.chromium.org/10562027/diff/17001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/10562027/diff/17001/ui/gfx/canvas.h#newcode113
ui/gfx/canvas.h:113: Canvas(const gfx::Size& size,
Oy, too many constructors. Any chance we could have this single one? If not can
we nuke the last one and lets folks apply the scale themselves? And homw many
places use:

Canvas(const gfx::ImageSkiaRep& image_rep, bool is_opaque);

? I would like to nuke that one too.

[pkotwicz, 2012-06-21 01:27:10]

http://codereview.chromium.org/10562027/diff/17001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/10562027/diff/17001/ui/gfx/canvas.h#newcode113
ui/gfx/canvas.h:113: Canvas(const gfx::Size& size,
This code actually only adds a single constructor. Canvas(const SkBitmap&
bitmap, bool is_opaque); should be removed really soon. Hopefully within a day
or two of this patch landing.
There's a few places, though not many which use Canvas(const gfx::ImageSkiaRep&
image_rep, bool is_opaque);, pretty much all the places which used to use
Canvas(const SkBitmap& bitmap, bool is_opaque); I agree that there are a lot of
constructors, but I think the constructor which takes an ImageSkiaRep is pretty
useful as it avoids a fair bit of typing.

[sky, 2012-06-21 17:34:06]

LGTM, as long you come back and clean this up. I like the factory idea so that
canvas has one or maybe two constructors.

[oshima, 2012-06-21 17:47:31]

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.cc#newcode79
ui/gfx/canvas.cc:79: ApplyTargetScaleFactor(target_scale_factor, scale_canvas);
On 2012/06/20 23:07:05, pkotwicz wrote:
> It is also used in layer.cc when scale_content_ is false.
> I am ok with a factory method but CreateUnscaledCanvas is the wrong name for
it.
> |scale_canvas| implies that we don't apply an additional scale, not that the
> SkCanvas is not scaled to begin with. Any suggestions?

How about CraeteCanvasWithoutScal{e/ing}()?

It's true that underlying sk canvas may or may not be scaled, but 
it's also true that applying 2x scale may result in no scale if the SkCanvas
was scaled .5x.
 At least, the name of the factory method can describe what it returns and
that's advantage over plain constructor.

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

http://codereview.chromium.org/10562027/diff/6003/ui/gfx/canvas.h#newcode168
ui/gfx/canvas.h:168: gfx::ImageSkiaRep ExtractBitmap() const;
On 2012/06/20 18:04:40, oshima wrote:
> ExtractImageSkia()
> 
> I'm fine  if you want to do it in separate CL
can you comment on this?

[pkotwicz, 2012-06-25 18:02:32]

Changes as requested.
In particular, added a factory method to canvas.cc CreateCanvasWithoutScaling
Thus in layer.cc, we are now still doing an explicit scale.

Also added gfx::Canvas::RecreateBackingCanvas such that canvas_skia_paint does
the right thing

[oshima, 2012-06-25 22:58:40]

factory method look good. I'd recommend not to add new code to  LGTM'ed code and
do it in separate CL next time.

http://codereview.chromium.org/10562027/diff/24001/skia/ext/canvas_paint_mac.h
File skia/ext/canvas_paint_mac.h (right):

http://codereview.chromium.org/10562027/diff/24001/skia/ext/canvas_paint_mac....
skia/ext/canvas_paint_mac.h:91: CGFloat scale = scaled_unit_rect.size.width;
is it possible to add DCHECK? will it break?

[sky, 2012-06-25 23:42:53]

I agree. Can we land the previously LGTMd patch then clean this up?

http://codereview.chromium.org/10562027/diff/24001/ui/gfx/canvas.cc
File ui/gfx/canvas.cc (right):

http://codereview.chromium.org/10562027/diff/24001/ui/gfx/canvas.cc#newcode25
ui/gfx/canvas.cc:25: Canvas::Canvas(const gfx::Size& size, bool is_opaque) {
Initialize members here. I know you set them in RecreateBackingCanvas, but its
safer to do it own both places. And by members I mean canvas_ and scale_factor_.

http://codereview.chromium.org/10562027/diff/24001/ui/gfx/canvas.cc#newcode38
ui/gfx/canvas.cc:38: bool is_opaque) {
Same comment here about initializing members.

[commit-bot: I haz the power, 2012-06-26 03:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10562027/31002

[commit-bot: I haz the power, 2012-06-26 18:41:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10562027/35016

[commit-bot: I haz the power, 2012-06-26 19:16:33]

Try job failure for 10562027-35016 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-06-26 19:19:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10562027/35016

[commit-bot: I haz the power, 2012-06-26 20:15:06]

Try job failure for 10562027-35016 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-26 20:20:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10562027/41001

[commit-bot: I haz the power, 2012-06-26 22:48:06]

Try job failure for 10562027-41001 (retry) on win_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-26 23:22:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10562027/46002

[commit-bot: I haz the power, 2012-06-27 02:12:08]

Try job failure for 10562027-46002 (retry) on win_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-06-27 16:58:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10562027/50001

[commit-bot: I haz the power, 2012-06-27 17:58:42]

Change committed as 144489