Make CanvasPainter raster directly to an SkBitmap

Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
different concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas, but this is smaller than the alternative.

BUG=671433
Review-Url: https://codereview.chromium.org/2694023003
Cr-Commit-Position: refs/heads/master@{#450594}
Committed: https://chromium.googlesource.com/chromium/src/+/ed75029a4736960aef2fe37cefb1f7ccc8417d83

[enne (OOO), 2017-02-13 22:06:32]

Description was changed from

==========
[wip] canvas painter changes
==========

to

==========
[wip] canvas painter changes

This refactoring is removing a dependency where code wants to raster
from a DisplayItemList into a cc::PaintCanvas.

ui::CanvasPainter previously provided a PaintContext to paint from a
PaintContext into a DisplayItemList and then raster that into a
gfx::Canvas (is-a SkCanvas) backed by an SkBitmap.

gfx::Canvas is currently backed by a cc::PaintCanvas instead of
an SkCanvas, and once cc::PaintCanvas is a different type instead of
a typedef to SkCanvas then these callers will break.

DisplayItemList will only raster into an SkCanvas and not into a
cc::PaintCanvas interface.  It doesn't seem desirable to make
DisplayItemList and cc::PaintRecord to have duplicate Raster functions
that go into both SkCanvas and cc::PaintCanvas at this time.

Therefore, instead make ui::CanvasPainter go directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit
of logic from both gfx::Canvas as well as CanvasSkiaPaint, but this
is smaller than the alternative.

BUG=671433
==========

[enne (OOO), 2017-02-13 22:23:54]

Description was changed from

==========
[wip] canvas painter changes

This refactoring is removing a dependency where code wants to raster
from a DisplayItemList into a cc::PaintCanvas.

ui::CanvasPainter previously provided a PaintContext to paint from a
PaintContext into a DisplayItemList and then raster that into a
gfx::Canvas (is-a SkCanvas) backed by an SkBitmap.

gfx::Canvas is currently backed by a cc::PaintCanvas instead of
an SkCanvas, and once cc::PaintCanvas is a different type instead of
a typedef to SkCanvas then these callers will break.

DisplayItemList will only raster into an SkCanvas and not into a
cc::PaintCanvas interface.  It doesn't seem desirable to make
DisplayItemList and cc::PaintRecord to have duplicate Raster functions
that go into both SkCanvas and cc::PaintCanvas at this time.

Therefore, instead make ui::CanvasPainter go directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit
of logic from both gfx::Canvas as well as CanvasSkiaPaint, but this
is smaller than the alternative.

BUG=671433
==========

to

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
differently concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas as well as CanvasSkiaPaint, but this is
smaller than the alternative.

BUG=671433
==========

[enne (OOO), 2017-02-13 22:37:55]

Description was changed from

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
differently concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas as well as CanvasSkiaPaint, but this is
smaller than the alternative.

BUG=671433
==========

to

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
different concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas as well as CanvasSkiaPaint, but this is
smaller than the alternative.

BUG=671433
==========

[enne (OOO), 2017-02-14 19:28:26]

enne@chromium.org changed reviewers:
+ danakj@chromium.org, tapted@chromium.org

[enne (OOO), 2017-02-14 19:49:26]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[enne (OOO), 2017-02-14 19:49:38]

Description was changed from

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
different concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas as well as CanvasSkiaPaint, but this is
smaller than the alternative.

BUG=671433
==========

to

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
different concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas, but this is smaller than the alternative.

BUG=671433
==========

[commit-bot: I haz the power, 2017-02-14 19:50:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-02-14 19:52:47]

I should add that I tested these changes manually with and without device scale
on Linux.  The button_drag_utils changes are tested by dragging from the
bookmark bar.  The textfield changes are tested by dragging from the omnibox.

[commit-bot: I haz the power, 2017-02-14 21:25:59]

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

[commit-bot: I haz the power, 2017-02-14 21:26:00]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-02-14 21:37:26]

LGTM

https://codereview.chromium.org/2694023003/diff/40001/ui/compositor/canvas_pa...
File ui/compositor/canvas_painter.h (right):

https://codereview.chromium.org/2694023003/diff/40001/ui/compositor/canvas_pa...
ui/compositor/canvas_painter.h:29: CanvasPainter(SkBitmap* output,
In blink at least this type of thing should be an SkCanvas cuz they generate
SkPictures sometimes, but I don't think UI does so u can go right to SkBitmap
here I guess.

https://codereview.chromium.org/2694023003/diff/40001/ui/compositor/canvas_pa...
ui/compositor/canvas_painter.h:30: gfx::Size paint_size,
const&

https://codereview.chromium.org/2694023003/diff/40001/ui/compositor/canvas_pa...
ui/compositor/canvas_painter.h:40: SkColor clear_color_;
const?

https://codereview.chromium.org/2694023003/diff/40001/ui/compositor/paint_con...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/2694023003/diff/40001/ui/compositor/paint_con...
ui/compositor/paint_context.h:68: float device_scale_factor() const { return
device_scale_factor_; }
It doesn't look like you need to add this if u store the raster scale. My
thought is that this API is intentionally left bare so it is very clear to go
thru a *Recorder to do anything, and PaintRecorder has a gfx::Canvas with a
scale on it.

https://codereview.chromium.org/2694023003/diff/40001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.cc (right):

https://codereview.chromium.org/2694023003/diff/40001/ui/views/button_drag_ut...
ui/views/button_drag_utils.cc:86: SkColor color = SkColorSetARGB(0, 0, 0, 0);
This is Sk_ColorTRANSPARENT

https://codereview.chromium.org/2694023003/diff/40001/ui/views/controls/label...
File ui/views/controls/label_unittest.cc (right):

https://codereview.chromium.org/2694023003/diff/40001/ui/views/controls/label...
ui/views/controls/label_unittest.cc:56: SkColor color(SkColorSetARGB(0, 0, 0,
0));
TRANSPARENT

https://codereview.chromium.org/2694023003/diff/40001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2694023003/diff/40001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1074: SkColor color(SkColorSetARGB(0,
0, 0, 0));
TRANSPARENT

https://codereview.chromium.org/2694023003/diff/40001/ui/views/drag_utils.h
File ui/views/drag_utils.h (right):

https://codereview.chromium.org/2694023003/diff/40001/ui/views/drag_utils.h#n...
ui/views/drag_utils.h:37: VIEWS_EXPORT float
GetDeviceScaleForNativeView(views::Widget* widget);
same order as in .cc file. Any comment that would add to understanding this
method?

[enne (OOO), 2017-02-14 22:03:47]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[enne (OOO), 2017-02-14 22:03:52]

Done, thanks! :)

[commit-bot: I haz the power, 2017-02-14 22:04:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 23:33:28]

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

[commit-bot: I haz the power, 2017-02-14 23:33:29]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-02-14 23:34:08]

I think GetDeviceScaleForNativeView(..) needs a different name, otherwise just
nits

https://codereview.chromium.org/2694023003/diff/60001/ui/views/button_drag_ut...
File ui/views/button_drag_utils.cc (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/button_drag_ut...
ui/views/button_drag_utils.cc:86: SkColor color(SK_ColorTRANSPARENT);
nit: SkColor color = SK_ColorTRANSPARENT;

(I couldn't find any examples of SkColor being initialized this way under
ui/views, but there's a bunch using the `foo =` pattern, which tends to be
preferred for primitives).

https://codereview.chromium.org/2694023003/diff/60001/ui/views/controls/label...
File ui/views/controls/label_unittest.cc (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/controls/label...
ui/views/controls/label_unittest.cc:56: SkColor color(SK_ColorTRANSPARENT);
color =

https://codereview.chromium.org/2694023003/diff/60001/ui/views/controls/textf...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/controls/textf...
ui/views/controls/textfield/textfield.cc:1072: SkColor
color(GetBackgroundColor());
color = (same below)

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc
File ui/views/drag_utils.cc (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc#...
ui/views/drag_utils.cc:15: float GetDeviceScaleForNativeView(views::Widget*
widget) {
nit: reorder to match .h  (and remove views:: prefix from Widget)

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc#...
ui/views/drag_utils.cc:24: }
random orthogonal thought: I wonder if we should have something like
display::Screen::GetMaxDisplayScaleFactor() -- i.e. return the device_scale from
the Display with the highest scale factor. Since presumably you could always
drag this asset to that display, and it would look a bit rubbish if it was drawn
at a smaller scale. (maybe even this method could just always return 2.0f
without breaking anything..).

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h
File ui/views/drag_utils.h (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h#n...
ui/views/drag_utils.h:33: // view associated with this widget.
nit: Widget (or |widget|) -> perhaps ".. the display associated with |widget|'s
native view."

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h#n...
ui/views/drag_utils.h:34: VIEWS_EXPORT float
GetDeviceScaleForNativeView(views::Widget* widget);
nit: views:: not required

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h#n...
ui/views/drag_utils.h:34: VIEWS_EXPORT float
GetDeviceScaleForNativeView(views::Widget* widget);
The name of this method feels too generic now that it's exposed in the .h. Maybe
ScaleFactorForDragFromWidget() ?

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h#n...
ui/views/drag_utils.h:38: VIEWS_EXPORT gfx::Canvas*
GetCanvasForDragImage(Widget* widget,
(not much uses this any more (just menu_controller.cc). I looked for ways we
might drop some of the boilerplate by changing this to return a gfx::ImageSkia
or similar, but that's probably not going to work :/).

[enne (OOO), 2017-02-15 00:02:05]

Done, thanks!

I should also mention that this patch fixes the scale of icons being dragged
from the bookmark bar.  Previously they were fuzzy, due to the 1.0 scale being
used to select images and now it passes the real scale.

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc
File ui/views/drag_utils.cc (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc#...
ui/views/drag_utils.cc:15: float GetDeviceScaleForNativeView(views::Widget*
widget) {
On 2017/02/14 at 23:34:07, tapted wrote:
> nit: reorder to match .h  (and remove views:: prefix from Widget)

You might want to consider a presubmit like
https://cs.chromium.org/chromium/src/cc/PRESUBMIT.py?q=cc/PRES+package:%5Echr...

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc#...
ui/views/drag_utils.cc:24: }
On 2017/02/14 at 23:34:07, tapted wrote:
> random orthogonal thought: I wonder if we should have something like
display::Screen::GetMaxDisplayScaleFactor() -- i.e. return the device_scale from
the Display with the highest scale factor. Since presumably you could always
drag this asset to that display, and it would look a bit rubbish if it was drawn
at a smaller scale. (maybe even this method could just always return 2.0f
without breaking anything..).

Yeah, that would make a lot of sense.  You could still get color space wrong,
but at least it wouldn't be fuzzy.

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h
File ui/views/drag_utils.h (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.h#n...
ui/views/drag_utils.h:38: VIEWS_EXPORT gfx::Canvas*
GetCanvasForDragImage(Widget* widget,
On 2017/02/14 at 23:34:07, tapted wrote:
> (not much uses this any more (just menu_controller.cc). I looked for ways we
might drop some of the boilerplate by changing this to return a gfx::ImageSkia
or similar, but that's probably not going to work :/).

I looked at that as well and saw that it couldn't quite get removed yet.

[enne (OOO), 2017-02-15 00:03:10]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 00:04:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-02-15 00:41:24]

ui/views lgtm - thanks!

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc
File ui/views/drag_utils.cc (right):

https://codereview.chromium.org/2694023003/diff/60001/ui/views/drag_utils.cc#...
ui/views/drag_utils.cc:15: float GetDeviceScaleForNativeView(views::Widget*
widget) {
On 2017/02/15 00:02:05, enne wrote:
> On 2017/02/14 at 23:34:07, tapted wrote:
> > nit: reorder to match .h  (and remove views:: prefix from Widget)
> 
> You might want to consider a presubmit like
>
https://cs.chromium.org/chromium/src/cc/PRESUBMIT.py?q=cc/PRES+package:%5Echr...

ooh - indeed :) also for gfx::.. just came up in another review

[enne (OOO), 2017-02-15 01:09:47]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2017-02-15 01:09:51]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-02-15 01:09:51]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2694023003/#ps80001
(title: "tapted review")

[commit-bot: I haz the power, 2017-02-15 01:10:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 06:29:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487120991256880,
"parent_rev": "d039981e6de2c11e1dfdc1c0f6ba61804cc92c49", "commit_rev":
"ed75029a4736960aef2fe37cefb1f7ccc8417d83"}

[commit-bot: I haz the power, 2017-02-15 06:30:18]

Description was changed from

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
different concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas, but this is smaller than the alternative.

BUG=671433
==========

to

==========
Make CanvasPainter raster directly to an SkBitmap

This refactoring is removing a dependency where code wants to raster
a DisplayItemList into a gfx::Canvas / cc::PaintCanvas.

Currently, gfx::Canvas is backed by a cc::PaintCanvas instead of an
SkCanvas.  DisplayItemList can only raster into an SkCanvas and not into
a cc::PaintCanvas interface.  This works currently because PaintCanvas
is-a SkCanvas (via a typedef), but will break once they become
different concrete types.

It doesn't seem desirable to make DisplayItemList and cc::PaintRecord to
have duplicate Raster functions for both SkCanvas and PaintCanvas.

Therefore, instead make ui::CanvasPainter raster directly from
DisplayItemList into an SkBitmap without a gfx::Canvas or PaintCanvas
intermediary.  This means that CanvasPainter needs to grow a tiny bit of
logic from both gfx::Canvas, but this is smaller than the alternative.

BUG=671433

Review-Url: https://codereview.chromium.org/2694023003
Cr-Commit-Position: refs/heads/master@{#450594}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed75029a4736960aef2fe37cefb1...
==========

[commit-bot: I haz the power, 2017-02-15 06:30:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ed75029a4736960aef2fe37cefb1...