Make cc/paint have concrete types

Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2690583002
Cr-Commit-Position: refs/heads/master@{#455840}
Committed: https://chromium.googlesource.com/chromium/src/+/d2501577f2df0764eda66614ddf429e230062562

[enne (OOO), 2017-02-10 16:56:53]

Description was changed from

==========
[wip] concrete cdl
==========

to

==========
[wip] concrete cdl
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-02-10 16:56:54]

Description was changed from

==========
[wip] concrete cdl
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[wip] concrete cdl
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[enne (OOO), 2017-02-17 07:06:13]

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

[commit-bot: I haz the power, 2017-02-17 07:06:41]

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-17 07:27:47]

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

[commit-bot: I haz the power, 2017-02-17 07:27:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[enne (OOO), 2017-02-17 18:27:21]

Description was changed from

==========
[wip] concrete cdl
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[wip] concrete cdl
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-02-25 03:45:02]

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

[commit-bot: I haz the power, 2017-02-25 03:45:14]

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-25 03:47:44]

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

[commit-bot: I haz the power, 2017-02-25 03:47:46]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[enne (OOO), 2017-02-26 06:30:25]

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

[commit-bot: I haz the power, 2017-02-26 06:30:31]

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-26 06:52:44]

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

[commit-bot: I haz the power, 2017-02-26 06:52:45]

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

[enne (OOO), 2017-02-26 07:57:39]

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

[commit-bot: I haz the power, 2017-02-26 07:57:45]

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-26 08:29:09]

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

[commit-bot: I haz the power, 2017-02-26 08:29:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[enne (OOO), 2017-02-28 21:01:41]

Description was changed from

==========
[wip] concrete cdl
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-02-28 21:01:41]

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

[enne (OOO), 2017-02-28 22:00:26]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-02-28 22:07:11]

This is waiting on a dependency patch for trivial type changes, but is otherwise
ready for review, I think.  danakj for overall review, and junov for all the
Blink canvas and image buffer changes.

Cluster telemetry finally agrees that this patch is ready to go:
https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/e...

Rasterize time says it regresses <1% here on linux, but a previous patch said it
improved by ~1%, so I think that's within the noise.  It's hard to imagine why
it would change at all, as PaintRecord is-a SkPicture.

I ended up having to make PaintRecorder not allocate a PaintCanvas and instead
have an internal SetSkCanvas method.  This was to avoid an allocation that was
regressing paint time by 1% on Linux and 4% on Android.

[danakj, 2017-03-02 16:56:50]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-02 18:19:35]

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

[commit-bot: I haz the power, 2017-03-02 18:19:55]

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-03-02 18:19:55]

(ping?)

[commit-bot: I haz the power, 2017-03-02 18:44:07]

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

[commit-bot: I haz the power, 2017-03-02 18:44:08]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2017-03-02 19:23:37]

enne@chromium.org changed reviewers:
+ vmpstr@chromium.org

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

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

[commit-bot: I haz the power, 2017-03-02 19:26:38]

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

[danakj, 2017-03-02 19:44:40]

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:410: std::unique_ptr<SkCanvas> owned_;
DISALLOW_COPY_AND_ASSIGN for each class that shouldn't be copied (most of these)

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_flags.h
File cc/paint/paint_flags.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:25: PaintFlags(const PaintFlags& flags) :
paint_(flags.paint_) {}
copy constructors should be paired with operator=

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:32: ALWAYS_INLINE Style getStyle() const { return
(Style)paint_.getStyle(); }
no c-style cast

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:34: paint_.setStyle((SkPaint::Style)style);
no c-style cast

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_record.cc
File cc/paint/paint_record.cc (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_record....
cc/paint/paint_record.cc:7: #include "base/logging.h"
unused

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_record.h
File cc/paint/paint_record.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_record....
cc/paint/paint_record.h:45: return sk_sp<SkPicture>(ToSkPicture(record.get()));
This adopts the pointer. But |record| will deref it, leading to a refcount
mismatch?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_record....
cc/paint/paint_record.h:50: return sk_sp<const
SkPicture>(ToSkPicture(record.get()));
same?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:23: SkBBHFactory* bbhFactory = nullptr,
why not chromium-style parameter names?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:56: PaintCanvas canvas_;
you could use Optional to avoid the private constructor/method on PaintCanvas

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_shader.h
File cc/paint/paint_shader.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_shader....
cc/paint/paint_shader.h:26: inline sk_sp<PaintShader> MakePaintShaderImage(const
sk_sp<SkImage> image,
aside.. const sk_sp..

This should be const SkImage* really?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_shader....
cc/paint/paint_shader.h:38: return
SkShader::MakePictureShader(ToSkPicture(record), tx, ty, local_matrix,
move record

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface.h
File cc/paint/paint_surface.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface...
cc/paint/paint_surface.h:27: return sk_sp<PaintSurface>(new
PaintSurface(std::move(s)));
return sk_make_sp<PaintSurface>(std::move(s))?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface...
cc/paint/paint_surface.h:34: return sk_sp<PaintSurface>(new
PaintSurface(std::move(s)));
return sk_make_sp<PaintSurface>(std::move(s))?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface...
cc/paint/paint_surface.h:43: canvas_ = base::WrapUnique(new
PaintCanvas(surface_->getCanvas()));
MakeUnique?

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface...
cc/paint/paint_surface.h:51: std::unique_ptr<PaintCanvas> canvas_;
you could use Optional if you want to avoid malloc

https://codereview.chromium.org/2690583002/diff/540001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2690583002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp:62:
if (!m_surface || !m_surface->getCanvas())
I don't see how m_surface can return a null canvas
https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkSurface.cpp...

[commit-bot: I haz the power, 2017-03-02 20:14:08]

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

[commit-bot: I haz the power, 2017-03-02 20:14:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[enne (OOO), 2017-03-02 20:53:46]

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

[commit-bot: I haz the power, 2017-03-02 20:54:17]

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-03-02 21:13:38]

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

[commit-bot: I haz the power, 2017-03-02 21:13:39]

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

[vmpstr, 2017-03-02 21:52:42]

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:48: ALWAYS_INLINE bool peekPixels(SkPixmap* pixmap) {
Where is this used? (along with readPixels/writePixels). These seem like not
really "paint" ops

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:86: enum {
Can you move these to the top (before the functions). Also name the enum plz

Also, should we name the enums in Chromium style here? or are you just using
this so calling code can just s/SkCanvas::/PaintCanvas::/?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:98: ALWAYS_INLINE void restoreToCount(int saveCount) {
save_count

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:117: bool doAntiAlias) {
do_anti_alias (throughout)

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:120: ALWAYS_INLINE void clipRect(const SkRect& rect,
SkClipOp op) {
Is this the same as above with doAntiAlias = false?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:148: ALWAYS_INLINE void clipRegion(const SkRegion&
deviceRgn,
device_region?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:236: enum SrcRectConstraint {
Move to top plz?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_flags.h
File cc/paint/paint_flags.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:66: enum Hinting {
move to top pls (throughout?)

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:153: paint_.setTypeface(typeface);
std::move(typeface) (throughout where we take sk_sp by value)

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:16: class PaintRecorder : private SkPictureRecorder {
I'm a bit hesitant about private inheritance... Why this instead of composition?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:43: sk_sp<SkPicture> p =
SkPictureRecorder::finishRecordingAsPicture(endFlags);
p = picture?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:44: return
sk_ref_sp(static_cast<PaintRecord*>(p.get()));
That's kind of an awkward pattern... Does static_cast<sk_sp<PaintRecord>>(p) not
work? I think that works for unique_ptr, but I'm guessing sk_sp doesn't
implement this type of conversion

[danakj, 2017-03-02 22:05:06]

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:86: enum {
On 2017/03/02 21:52:42, vmpstr wrote:
> Can you move these to the top (before the functions). Also name the enum plz

These are constants, not an actualy enum. Anonymous is right here, it's the way
of the style guide. Tho it asks each to be in a separate enum{} but if its anon
then same deal.

> Also, should we name the enums in Chromium style here? or are you just using
> this so calling code can just s/SkCanvas::/PaintCanvas::/?

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:90: using SaveLayerFlags = SkCanvas::SaveLayerFlags;
I do note that this doesn't appear anywhere else so maybe it's unused? Then
maybe the enum constants are also?

[vmpstr, 2017-03-02 22:11:42]

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:86: enum {
On 2017/03/02 22:05:06, danakj wrote:
> On 2017/03/02 21:52:42, vmpstr wrote:
> > Can you move these to the top (before the functions). Also name the enum plz
> 
> These are constants, not an actualy enum. Anonymous is right here, it's the
way
> of the style guide. Tho it asks each to be in a separate enum{} but if its
anon
> then same deal.

In that case, SrcRectConstraint below should also be anon? It's a bit ambiguous
what's an enum and what's a constant, but the kNaming suggests these should all
be separate anon enums then

> 
> > Also, should we name the enums in Chromium style here? or are you just using
> > this so calling code can just s/SkCanvas::/PaintCanvas::/?
>

[danakj, 2017-03-02 22:14:43]

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:86: enum {
On 2017/03/02 22:11:42, vmpstr wrote:
> On 2017/03/02 22:05:06, danakj wrote:
> > On 2017/03/02 21:52:42, vmpstr wrote:
> > > Can you move these to the top (before the functions). Also name the enum
plz
> > 
> > These are constants, not an actualy enum. Anonymous is right here, it's the
> way
> > of the style guide. Tho it asks each to be in a separate enum{} but if its
> anon
> > then same deal.
> 
> In that case, SrcRectConstraint below should also be anon? It's a bit
ambiguous
> what's an enum and what's a constant, but the kNaming suggests these should
all
> be separate anon enums then

SrcRectConstraint looks like an actual enum, it's passed as a parameter. The
names are just the skia names cuz this is changing the types not the names (note
the skia method name styles too). Would be nice to TODO on the enums that should
be turned into SHOUT_CASE probably.

> > 
> > > Also, should we name the enums in Chromium style here? or are you just
using
> > > this so calling code can just s/SkCanvas::/PaintCanvas::/?
> >

[Justin Novosad, 2017-03-02 22:19:24]

https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:572:
WTF::wrapUnique(new PaintCanvas(m_surface->getCanvas()));
Why do we need to make of the canvas?

[Justin Novosad, 2017-03-02 22:23:15]

https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:572:
WTF::wrapUnique(new PaintCanvas(m_surface->getCanvas()));
On 2017/03/02 22:19:24, Justin Novosad wrote:
> Why do we need to make of the canvas?

Actually I am not sure I understand what this does. AFAICT PaintCanvas is an
alias for SkCanvas, but SkCanvas is non-copyable.  This should not compile.  I
am obviously misreading this code.  What am I getting wrong?

[danakj, 2017-03-02 22:35:36]

https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:572:
WTF::wrapUnique(new PaintCanvas(m_surface->getCanvas()));
On 2017/03/02 22:23:15, Justin Novosad wrote:
> On 2017/03/02 22:19:24, Justin Novosad wrote:
> > Why do we need to make of the canvas?
> 
> Actually I am not sure I understand what this does. AFAICT PaintCanvas is an
> alias for SkCanvas, but SkCanvas is non-copyable.  This should not compile.  I
> am obviously misreading this code.  What am I getting wrong?

This patch changes it to be a type wrapping an SkCanvas. See the cc/ changes.

[enne (OOO), 2017-03-02 23:57:39]

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

[commit-bot: I haz the power, 2017-03-02 23:58:04]

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-03-02 23:59:44]

Addressed comments.  Specific replies below.

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:23: SkBBHFactory* bbhFactory = nullptr,
On 2017/03/02 at 19:44:40, danakj wrote:
> why not chromium-style parameter names?

Just following SkPictureRecorder example.  Changed, and removed recordFlags,
since it's not used.

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface.h
File cc/paint/paint_surface.h (right):

https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_surface...
cc/paint/paint_surface.h:51: std::unique_ptr<PaintCanvas> canvas_;
On 2017/03/02 at 19:44:40, danakj wrote:
> you could use Optional if you want to avoid malloc

Yeah, just made this optional.

https://codereview.chromium.org/2690583002/diff/540001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp
(right):

https://codereview.chromium.org/2690583002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp:62:
if (!m_surface || !m_surface->getCanvas())
On 2017/03/02 at 19:44:40, danakj wrote:
> I don't see how m_surface can return a null canvas
https://cs.chromium.org/chromium/src/third_party/skia/src/image/SkSurface.cpp...

Good catch.  Removed this.

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:48: ALWAYS_INLINE bool peekPixels(SkPixmap* pixmap) {
On 2017/03/02 at 21:52:42, vmpstr wrote:
> Where is this used? (along with readPixels/writePixels). These seem like not
really "paint" ops

https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_vid...

Yeah, there are a number of things in here (like flush) that are not "paint" ops
but are tied very much to SkCanvas being both a recording and a raster
interface.  I think there is a bunch of cleanup to be had here that I am
deliberately punting on because it's not that important.

To fix a number of them, we'd have to duplicate a number of codepaths (e.g.
through drawing a video, which may be drawn into page content or into a canvas,
which itself might be hardware, bitmap, or display list bakced) to be explicit
about the kind of PaintCanvas vs SkCanvas that was being used.

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:86: enum {
On 2017/03/02 at 22:14:43, danakj wrote:
> On 2017/03/02 22:11:42, vmpstr wrote:
> > On 2017/03/02 22:05:06, danakj wrote:
> > > On 2017/03/02 21:52:42, vmpstr wrote:
> > > > Can you move these to the top (before the functions). Also name the enum
plz
> > > 
> > > These are constants, not an actualy enum. Anonymous is right here, it's
the
> > way
> > > of the style guide. Tho it asks each to be in a separate enum{} but if its
> > anon
> > > then same deal.
> > 
> > In that case, SrcRectConstraint below should also be anon? It's a bit
ambiguous
> > what's an enum and what's a constant, but the kNaming suggests these should
all
> > be separate anon enums then
> 
> SrcRectConstraint looks like an actual enum, it's passed as a parameter. The
names are just the skia names cuz this is changing the types not the names (note
the skia method name styles too). Would be nice to TODO on the enums that should
be turned into SHOUT_CASE probably.
> 
> > > 
> > > > Also, should we name the enums in Chromium style here? or are you just
using
> > > > this so calling code can just s/SkCanvas::/PaintCanvas::/?
> > >

Re: enum discussion.  Removed others.  Left names unchanged to avoid churn. 
Left enums in place inline instead of moving to the top, because I think most of
them are pretty contextual in terms of being parameters to specific functions vs
being general class enums.

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:90: using SaveLayerFlags = SkCanvas::SaveLayerFlags;
On 2017/03/02 at 22:05:06, danakj wrote:
> I do note that this doesn't appear anywhere else so maybe it's unused? Then
maybe the enum constants are also?

Yeah, removed.

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:120: ALWAYS_INLINE void clipRect(const SkRect& rect,
SkClipOp op) {
On 2017/03/02 at 21:52:42, vmpstr wrote:
> Is this the same as above with doAntiAlias = false?

Sure, removed.

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:16: class PaintRecorder : private SkPictureRecorder {
On 2017/03/02 at 21:52:42, vmpstr wrote:
> I'm a bit hesitant about private inheritance... Why this instead of
composition?

Sure, changed to composition here.  I think PaintRecord needs to stay being
private inheritance so that it's possible to cast directly from sk_sp of
SkPicture to PaintRecord.

https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:44: return
sk_ref_sp(static_cast<PaintRecord*>(p.get()));
On 2017/03/02 at 21:52:42, vmpstr wrote:
> That's kind of an awkward pattern... Does static_cast<sk_sp<PaintRecord>>(p)
not work? I think that works for unique_ptr, but I'm guessing sk_sp doesn't
implement this type of conversion

Yeah, it doesn't work, sorry.

[commit-bot: I haz the power, 2017-03-03 00:03:35]

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

[commit-bot: I haz the power, 2017-03-03 00:03:37]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2017-03-03 01:54:45]

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

[commit-bot: I haz the power, 2017-03-03 01:55:24]

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-03-03 01:56:29]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2017-03-03 01:57:10]

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

[commit-bot: I haz the power, 2017-03-03 01:57:47]

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-03-03 03:10:44]

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

[commit-bot: I haz the power, 2017-03-03 03:10:45]

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

[vmpstr, 2017-03-03 07:12:08]

cc lgtm aside from the nits:

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h
File cc/paint/paint_flags.h (right):

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:23: PaintFlags() = default;
I don't think you need to declare any ctors or assignment ops. As far as I can
tell the stuff here is all going to be automatically generated anyway (plus a
move ctor which right now is not generated due to dtor and copy ctor). Or did I
miss a subtlety somewhere?

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_record.h
File cc/paint/paint_record.h (right):

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_record....
cc/paint/paint_record.h:18: class CC_PAINT_EXPORT PaintRecord : private
SkPicture {
This class kinda looks funny, since it has no constructors :P LIke the DISALLOW
COPY prevents the default ctor from being generated so it looks like the class
can't be created, but it's because we're casting from SkPicture xD

Maybe leave a small comment on the class, however temporary, about how an object
of this class is created

[danakj, 2017-03-03 18:33:19]

LGTM, looks like you changed most of the parameter names, thanks! I spotted a
few more.

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:68: size_t rowBytes,
row_bytes

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h
File cc/paint/paint_flags.h (right):

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:64: ALWAYS_INLINE void setSubpixelText(bool subpixelText)
{
sub_pixel_text

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h...
cc/paint/paint_flags.h:70: ALWAYS_INLINE void setLCDRenderText(bool lcdText) {
lcd_text

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:44: uint32_t endFlags = 0) {
end_flags

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:50: const SkRect& cullRect,
cull_rect

https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:51: uint32_t endFlags = 0) {
end_flags

[enne (OOO), 2017-03-03 19:24:30]

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

[commit-bot: I haz the power, 2017-03-03 19:24:53]

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-03-03 19:26:10]

Thanks.  Addressed review comments.  Found a few more parameters not in Chromium
style to rename too.  /o\

https://codereview.chromium.org/2690583002/diff/660001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/660001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:49: canvas_.reset();
Apparently getRecordingCanvas is (reasonably) only be valid between begin/finish
recording, which makes sense.  Printing code depended on this being null after
finish recording, so I added it in.

[danakj, 2017-03-03 19:27:22]

https://codereview.chromium.org/2690583002/diff/660001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2690583002/diff/660001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:49: canvas_.reset();
On 2017/03/03 19:26:10, enne wrote:
> Apparently getRecordingCanvas is (reasonably) only be valid between
begin/finish
> recording, which makes sense.  Printing code depended on this being null after
> finish recording, so I added it in.

Oh wild, glad Optional makes that easy.

[enne (OOO), 2017-03-03 20:43:50]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-03 20:54:49]

enne@chromium.org changed reviewers:
+ ddorwin@chromium.org, derat@chromium.org, nick@chromium.org,
vitalybuka@chromium.rg

[enne (OOO), 2017-03-03 20:54:51]

junov: blink

Some more trivial owner changes now that this is closer to landing:

derat: ash/
nick: content/
ddorwin: media/
vitalybuka: printing/

[Daniel Erat, 2017-03-03 21:12:03]

lgtm for /ash

[commit-bot: I haz the power, 2017-03-03 21:34:33]

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

[commit-bot: I haz the power, 2017-03-03 21:34:35]

Dry run: This issue passed the CQ dry run.

[Justin Novosad, 2017-03-03 21:47:32]

lgtm for blink

[enne (OOO), 2017-03-06 17:27:46]

ddorwin, nick, vitalybuka: ping for small owners review

[ddorwin, 2017-03-06 17:37:23]

media/ LGTM

[enne (OOO), 2017-03-06 17:46:20]

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

[commit-bot: I haz the power, 2017-03-06 17:46:50]

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-03-06 18:03:29]

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

[commit-bot: I haz the power, 2017-03-06 18:03:31]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[ncarter (slow), 2017-03-06 19:20:40]

content lgtm

[enne (OOO), 2017-03-06 19:21:28]

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

[commit-bot: I haz the power, 2017-03-06 19:22:23]

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-03-06 21:01:12]

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

[commit-bot: I haz the power, 2017-03-06 21:01:13]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-03-06 21:02:19]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

TBR=vitalybuka@chromium.org
BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-06 21:03:52]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

TBR=vitalybuka@chromium.org
BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Vitaly Buka (NO REVIEWS), 2017-03-06 21:23:01]

vitalybuka@chromium.org changed reviewers:
+ thestig@chromium.org, vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2017-03-06 21:23:03]

lgtm

[Vitaly Buka (NO REVIEWS), 2017-03-06 21:23:28]

printing/ lgtm

[enne (OOO), 2017-03-06 21:24:48]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-03-06 21:24:49]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
vmpstr@chromium.org, derat@chromium.org, junov@chromium.org,
ddorwin@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2690583002/#ps700001
(title: "Fix chromeos")

[commit-bot: I haz the power, 2017-03-06 21:25:34]

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-03-06 21:30:31]

CQ is committing da patch.

Bot data: {"patchset_id": 700001, "attempt_start_ts": 1488835488832670,
"parent_rev": "eacb517593b07269d8c830a3f4be4061e7e654e9", "commit_rev":
"762276ecdfbff938d3b7431ad0ed00c3ab6a4136"}

[commit-bot: I haz the power, 2017-03-06 21:31:14]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2690583002
Cr-Commit-Position: refs/heads/master@{#454962}
Committed:
https://chromium.googlesource.com/chromium/src/+/762276ecdfbff938d3b7431ad0ed...
==========

[commit-bot: I haz the power, 2017-03-06 21:31:16]

Committed patchset #36 (id:700001) as
https://chromium.googlesource.com/chromium/src/+/762276ecdfbff938d3b7431ad0ed...

[enne (OOO), 2017-03-07 18:51:42]

A revert of this CL (patchset #36 id:700001) has been created in
https://codereview.chromium.org/2739533003/ by enne@chromium.org.

The reason for reverting is: Invalid casts from SkMiniPicture to PaintRecord

BUG=699105.

[enne (OOO), 2017-03-07 21:35:41]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2690583002
Cr-Commit-Position: refs/heads/master@{#454962}
Committed:
https://chromium.googlesource.com/chromium/src/+/762276ecdfbff938d3b7431ad0ed...
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-07 21:37:27]

danakj, vmpstr: ptal at changes

Due to SkMiniPicture which is a derived SkPicture class, I think it's impossible
to have PaintRecord not be a typedef until it implements something real on its
own.  So, PaintRecord is not just a typedef to SkPicture again.  The other
changes are because you can no longer SkPicture::playback(cc::PaintCanvas*) and
so these all change to cc::PaintCanvas::drawPicture(PaintRecord* aka
SkPicture*).

[enne (OOO), 2017-03-07 21:37:31]

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

[commit-bot: I haz the power, 2017-03-07 21:38:16]

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-03-07 23:58:23]

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

[commit-bot: I haz the power, 2017-03-07 23:58:25]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-03-07 23:58:35]

LGTM

[vmpstr, 2017-03-08 01:13:11]

lgtm as well

[enne (OOO), 2017-03-08 17:58:17]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-03-08 17:58:18]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
derat@chromium.org, junov@chromium.org, ddorwin@chromium.org,
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2690583002/#ps740001
(title: "PaintRecord as typedef, fixup playback calls")

[commit-bot: I haz the power, 2017-03-08 17:58:52]

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-03-08 18:10:42]

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

[commit-bot: I haz the power, 2017-03-08 18:10:46]

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

[enne (OOO), 2017-03-08 19:04:33]

enne@chromium.org changed reviewers:
+ pdr@chromium.org

[enne (OOO), 2017-03-08 19:04:47]

enne@chromium.org changed reviewers:
- pdr@chromium.org

[enne (OOO), 2017-03-08 19:04:50]

+pdr for Blink Source/web

[enne (OOO), 2017-03-09 01:40:10]

enne@chromium.org changed reviewers:
+ pdr@chromium.org

[enne (OOO), 2017-03-09 01:40:24]

On 2017/03/08 at 19:04:50, enne wrote:
> +pdr for Blink Source/web

+pdr for real this time

[pdr., 2017-03-09 17:58:41]

On 2017/03/09 at 01:40:24, enne wrote:
> On 2017/03/08 at 19:04:50, enne wrote:
> > +pdr for Blink Source/web
> 
> +pdr for real this time

for real pdr LGTM

[enne (OOO), 2017-03-09 17:59:56]

On 2017/03/09 at 17:58:41, pdr wrote:
> On 2017/03/09 at 01:40:24, enne wrote:
> > On 2017/03/08 at 19:04:50, enne wrote:
> > > +pdr for Blink Source/web
> > 
> > +pdr for real this time
> 
> for real pdr LGTM

much better than those imaginary pdrs

[enne (OOO), 2017-03-09 18:00:00]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2017-03-09 18:00:20]

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-03-09 19:59:22]

CQ is committing da patch.

Bot data: {"patchset_id": 740001, "attempt_start_ts": 1489082400063910,
"parent_rev": "3e8277dbaf900a53ab0ec812a030ac4a95f7323a", "commit_rev":
"d2501577f2df0764eda66614ddf429e230062562"}

[commit-bot: I haz the power, 2017-03-09 20:00:52]

Description was changed from

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Make cc/paint have concrete types

This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder
to all be real types (that forward to Skia types internally).  PaintShader
is left as-is for now.  This will force callers to use the correct
types in the rest of Chromium as the internals of these classes are
rewritten in future patches.

This code also changes a number of callers elsewhere in the codebase
that want to wrap an SkCanvas in a PaintCanvas.  As SkCanvas has no
constructor that takes an SkCanvas*, this change had to wait for this
final conversion patch.

In general, if code wants to raster directly into a bitmap with local
code, it should use Skia directly.  If code wants to raster into a bitmap
with paint callers, it can wrap that bitmap in a PaintCanvas.  Similarly,
if code wants to go into an accelerated SkSurface, it should wrap that
surface's canvas in a PaintCanvas (as blink html canvas code does).

BUG=671433

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

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

[commit-bot: I haz the power, 2017-03-09 20:00:55]

Committed patchset #38 (id:740001) as
https://chromium.googlesource.com/chromium/src/+/d2501577f2df0764eda66614ddf4...