Limit manual control of platform painting

Limit manual control of platform painting

Platform painting should be through skia::ScopedPlatformPaint.
(Only one compositor callsite was calling Begin/End directly.)

Remove skia::EndPlatformPaint() and all device-specific implementations,
which were effectively noops.

Remove skia::BeginPlatformPaint(), and make all device-specific impls
protected.

BUG=598695
R=fmalita@chromium.org
TBR=danakj@chromium.org

Committed: https://crrev.com/7b79fb0215d08827085fa40c6c1fa027628a3c95
Cr-Commit-Position: refs/heads/master@{#383820}

[tomhudson, 2016-03-29 16:03:25]

Lots of friends; maybe there's a better fix. (Moving ScopedPlatformPaint to
platform_device would clean up the include proliferation, but that doesn't seem
to be *absolutely* a problem.)

[tomhudson, 2016-03-29 16:58:00]

The CQ bit was checked by tomhudson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 16:58:16]

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

[f(malita), 2016-03-29 17:31:08]

Thanks for cleaning this up - looks good, just some nits.

(I find it kind of odd that none of the platform impls do meaningful work in
EndPlatformPaint, but it sure makes things simpler)

https://codereview.chromium.org/1839113002/diff/1/content/browser/compositor/...
File content/browser/compositor/software_output_device_win.cc (right):

https://codereview.chromium.org/1839113002/diff/1/content/browser/compositor/...
content/browser/compositor/software_output_device_win.cc:183:
skia::ScopedPlatformPaint spp (contents_.get());
Nit: extra space

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_cairo.h (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_cairo.h:13: #include
"skia/ext/platform_canvas.h"
Nit: forward-declare ScopedPlatformPaint instead of include?

(ditto for all other instances)

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_cairo.h:90: cairo_t* BeginPlatformPaint()
override;
Nit: if this is only called from ScopedPlatformPaint, can it be private?

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_cairo.h:123: friend class
skia::ScopedPlatformPaint;
Nit: we're in the 'skia' namespace, I don't think we need an explicit namespace.

(ditto for all other instances)

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:233: HDC source_dc =
BeginPlatformPaint();
Nit: use ScopedPlatformPaint here also?

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_win.h (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.h:107: int begin_paint_count_;
Nit: no longer used?

https://codereview.chromium.org/1839113002/diff/1/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:154: ~ScopedPlatformPaint();
Nit: dtor no longer needed?

https://codereview.chromium.org/1839113002/diff/1/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:157: PlatformSurface GetPlatformSurface();
Nit: this is trivial enough to keep inline.

[tomhudson, 2016-03-29 17:47:32]

> (I find it kind of odd that none of the platform impls do meaningful work in
> EndPlatformPaint, but it sure makes things simpler)

Some of the comments suggest to me that they may have formerly done OS-specific
flushes there which have since been moved to onAccessBitmap().

https://codereview.chromium.org/1839113002/diff/1/content/browser/compositor/...
File content/browser/compositor/software_output_device_win.cc (right):

https://codereview.chromium.org/1839113002/diff/1/content/browser/compositor/...
content/browser/compositor/software_output_device_win.cc:183:
skia::ScopedPlatformPaint spp (contents_.get());
On 2016/03/29 17:31:07, f(malita) wrote:
> Nit: extra space

Done.

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_cairo.h (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_cairo.h:13: #include
"skia/ext/platform_canvas.h"
On 2016/03/29 17:31:07, f(malita) wrote:
> Nit: forward-declare ScopedPlatformPaint instead of include?
> 
> (ditto for all other instances)

Ugh, I had problems with that when I was declaring friends, but I did it wrong
before.

Done

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_cairo.h:90: cairo_t* BeginPlatformPaint()
override;
On 2016/03/29 17:31:07, f(malita) wrote:
> Nit: if this is only called from ScopedPlatformPaint, can it be private?

Done.

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_cairo.h:123: friend class
skia::ScopedPlatformPaint;
On 2016/03/29 17:31:07, f(malita) wrote:
> Nit: we're in the 'skia' namespace, I don't think we need an explicit
namespace.
> 
> (ditto for all other instances)

Done.

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_win.cc (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.cc:233: HDC source_dc =
BeginPlatformPaint();
On 2016/03/29 17:31:07, f(malita) wrote:
> Nit: use ScopedPlatformPaint here also?

No; we're calling this->BeginPlatformPaint(), not
skia::BeginPlatformPaint(SkCanvas*).

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
File skia/ext/bitmap_platform_device_win.h (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/bitmap_platform_de...
skia/ext/bitmap_platform_device_win.h:107: int begin_paint_count_;
On 2016/03/29 17:31:07, f(malita) wrote:
> Nit: no longer used?

Done.

https://codereview.chromium.org/1839113002/diff/1/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1839113002/diff/1/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:154: ~ScopedPlatformPaint();
On 2016/03/29 17:31:08, f(malita) wrote:
> Nit: dtor no longer needed?

Done.

https://codereview.chromium.org/1839113002/diff/1/skia/ext/platform_canvas.h#...
skia/ext/platform_canvas.h:157: PlatformSurface GetPlatformSurface();
On 2016/03/29 17:31:08, f(malita) wrote:
> Nit: this is trivial enough to keep inline.

I expect to change this soon enough that I wanted to move it out of the header,
but OK.

[f(malita), 2016-03-29 18:14:13]

Thanks, LGTM.

https://codereview.chromium.org/1839113002/diff/20001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/1839113002/diff/20001/skia/ext/platform_devic...
skia/ext/platform_device.h:86: virtual PlatformSurface BeginPlatformPaint();
nit: now private

[tomhudson, 2016-03-29 19:23:15]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2016-03-29 19:23:16]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1839113002/#ps40001
(title: "Privatize")

[commit-bot: I haz the power, 2016-03-29 19:23:37]

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

[commit-bot: I haz the power, 2016-03-29 21:03:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-29 21:05:09]

Description was changed from

==========
Limit manual control of platform painting

Platform painting should be through skia::ScopedPlatformPaint.
(Only one compositor callsite was calling Begin/End directly.)

Remove skia::EndPlatformPaint() and all device-specific implementations,
which were effectively noops.

Remove skia::BeginPlatformPaint(), and make all device-specific impls
protected.

BUG=598695
R=fmalita@chromium.org
TBR=danakj@chromium.org
==========

to

==========
Limit manual control of platform painting

Platform painting should be through skia::ScopedPlatformPaint.
(Only one compositor callsite was calling Begin/End directly.)

Remove skia::EndPlatformPaint() and all device-specific implementations,
which were effectively noops.

Remove skia::BeginPlatformPaint(), and make all device-specific impls
protected.

BUG=598695
R=fmalita@chromium.org
TBR=danakj@chromium.org

Committed: https://crrev.com/7b79fb0215d08827085fa40c6c1fa027628a3c95
Cr-Commit-Position: refs/heads/master@{#383820}
==========

[commit-bot: I haz the power, 2016-03-29 21:05:13]

Patchset 3 (id:??) landed as
https://crrev.com/7b79fb0215d08827085fa40c6c1fa027628a3c95
Cr-Commit-Position: refs/heads/master@{#383820}