Move uncoupled code out of skia/ext/platform_device.h

The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

All changes outside skia/ext/ should be purely mechanical renamings
or trimming of include lists.

R=fmalita@chromium.org
TBR=thestig@chromium.org,danakj@chromium.org,piman@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/31521a8fe53dda37a9a838402693b4d2060ec432
Cr-Commit-Position: refs/heads/master@{#362151}

[Tom Hudson, 2015-11-23 20:48:36]

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

[commit-bot: I haz the power, 2015-11-23 20:49:12]

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

[commit-bot: I haz the power, 2015-11-23 21:14:56]

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

[commit-bot: I haz the power, 2015-11-23 21:14:57]

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

[Tom Hudson, 2015-11-23 21:58:37]

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

[commit-bot: I haz the power, 2015-11-23 22:04:09]

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

[commit-bot: I haz the power, 2015-11-23 22:20:23]

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

[commit-bot: I haz the power, 2015-11-23 22:20:23]

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

[Tom Hudson, 2015-11-24 18:55:49]

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

[Tom Hudson, 2015-11-24 18:56:25]

Description was changed from

==========
WIP Move uncoupled code out of skia/ext/platform_device.h

The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

Currently blocked on crrev.com/1432443004

BUG=543755
==========

to

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

BUG=543755
==========

[Tom Hudson, 2015-11-24 18:56:30]

tomhudson@chromium.org changed reviewers:
- tomhudson@chromium.org

[commit-bot: I haz the power, 2015-11-24 18:57:01]

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

[commit-bot: I haz the power, 2015-11-24 19:14:00]

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

[Tom Hudson, 2015-11-24 19:15:43]

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

[commit-bot: I haz the power, 2015-11-24 19:16:29]

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

[Tom Hudson, 2015-11-24 21:09:13]

tomhudson@chromium.org changed reviewers:
+ fmalita@chromium.org
- tomhudson@chromium.org

[Tom Hudson, 2015-11-24 21:09:14]

Florin, PTAL at everything.

(Since all the changes outside skia/ext/ are purely mechanical renamings, I hope
to TBR)

[commit-bot: I haz the power, 2015-11-24 22:36:46]

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

[commit-bot: I haz the power, 2015-11-24 22:36:47]

Dry run: This issue passed the CQ dry run.

[f(malita), 2015-11-24 23:13:23]

looks good, just some nits.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.cc (right):

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:7: #include "base/logging.h"
Is this needed/used?

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:124: SK_API CGContextRef GetBitmapContext(const
SkCanvas& canvas) {
I don't think we need SK_API here.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:130: return NULL;
Nit: nullptr?

Nit^2: maybe

  return platform_device ? platform_device->GetBitmapContext() : nullptr; ?

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:178: SK_API SkMetaData& getMetaData(const SkCanvas&
canvas);
Nit: GetMetaData(...)?

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:178: SK_API SkMetaData& getMetaData(const SkCanvas&
canvas);
Also, I don't think we have any external users for this.  Can we relocate it to
anonymous namespace, in platform_canvas.cc?

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:185: #if defined(OS_MACOSX)
Nit: consolidate ifdefs

https://codereview.chromium.org/1462163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1462163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:38: #include
"skia/ext/platform_canvas.h"
Interesting - do you happen to know what in GraphicsContext needs access to
platform_canvas?

[Tom Hudson, 2015-11-25 15:50:35]

+Hal in case he has thoughts about how to radically simplify the PDF metafile
path that causes all the complexity here.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.cc (right):

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:7: #include "base/logging.h"
On 2015/11/24 23:13:23, f(malita) wrote:
> Is this needed/used?

Yeah, we're getting DCHECK from there. We could migrate to SkASSERT?

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:124: SK_API CGContextRef GetBitmapContext(const
SkCanvas& canvas) {
On 2015/11/24 23:13:23, f(malita) wrote:
> I don't think we need SK_API here.

I think we do, see header file for the 5 invocations outside skia/ext/.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:130: return NULL;
On 2015/11/24 23:13:23, f(malita) wrote:
> Nit: nullptr?
> 
> Nit^2: maybe
> 
>   return platform_device ? platform_device->GetBitmapContext() : nullptr; ?

Done.

Also changed other NULLs in this file to nullptr.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:178: SK_API SkMetaData& getMetaData(const SkCanvas&
canvas);
On 2015/11/24 23:13:23, f(malita) wrote:
> Nit: GetMetaData(...)?

Done.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:178: SK_API SkMetaData& getMetaData(const SkCanvas&
canvas);
On 2015/11/24 23:13:23, f(malita) wrote:
> Nit: GetMetaData(...)?

Done.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:178: SK_API SkMetaData& getMetaData(const SkCanvas&
canvas);
On 2015/11/24 23:13:23, f(malita) wrote:
> Also, I don't think we have any external users for this.  Can we relocate it
to
> anonymous namespace, in platform_canvas.cc?

printing/metafile_skia_wrapper.cc uses it twice, and GraphicsContext and
WebLocalFrameImpl from Blink as well. GraphicsContext is the only one I can
easily see wrapping; perhaps the WebLocalFrameImpl, but the Metafile is too
weird to be a straightforward refactoring.



In Blink, we see WebLocalFrameImpl storing the metadata on a GraphicsContext in
spoolSinglePage() and spoolAllPagesWithBoundaries(), and
GraphicsContext::beginRecording() setting the metadata on the (device backing
the) canvas it spawns.)

WebLocalFrameImpl is entered from PrintWebViewHelper; the linux implementation,
at least, is explicitly using MetafileSkiaWrapper to track the metafile in the
Canvas' metadata.

So it all ties together: as long as we have to track the metafile, it gets
tunnelled through WebLocalFrameImpl, GraphicsContext, and SkPictureBuilder so
that the canvases created for print results all know about the metafile
specified at the start of the printing process.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.h:185: #if defined(OS_MACOSX)
On 2015/11/24 23:13:23, f(malita) wrote:
> Nit: consolidate ifdefs

Done.

Odd that we also have some things under __APPLE__ guards in the same file, but
I'm going to guess that those are both OSX *and* iOS, and these aren't (yet?).

https://codereview.chromium.org/1462163002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp (right):

https://codereview.chromium.org/1462163002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp:38: #include
"skia/ext/platform_canvas.h"
On 2015/11/24 23:13:23, f(malita) wrote:
> Interesting - do you happen to know what in GraphicsContext needs access to
> platform_canvas?

Two calls to GetMetaData(), which is getting passed around through constructors
between GraphicsContext and SkPictureBuilder.

[Tom Hudson, 2015-11-25 17:14:29]

Description was changed from

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

All changes outside skia/ext/ should be purely mechanical renamings
or trimming of include lists.

R=fmalita@chromium.org
TBR=thestig@chromium.org,danakj@chromium.org,piman@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[f(malita), 2015-11-25 20:33:04]

Thanks, LGTM.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.cc (right):

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:7: #include "base/logging.h"
On 2015/11/25 15:50:35, Tom Hudson wrote:
> On 2015/11/24 23:13:23, f(malita) wrote:
> > Is this needed/used?
> 
> Yeah, we're getting DCHECK from there. We could migrate to SkASSERT?

Ah, got you.  DCHECK is fine.

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:124: SK_API CGContextRef GetBitmapContext(const
SkCanvas& canvas) {
On 2015/11/25 15:50:35, Tom Hudson wrote:
> On 2015/11/24 23:13:23, f(malita) wrote:
> > I don't think we need SK_API here.
> 
> I think we do, see header file for the 5 invocations outside skia/ext/.

Hmm, how about SetIsPreviewMetafile & IsPreviewMetafile above?  Those are also
used externally, but we only use SK_API for the decl.

[Tom Hudson, 2015-11-30 16:14:42]

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
File skia/ext/platform_canvas.cc (right):

https://codereview.chromium.org/1462163002/diff/100001/skia/ext/platform_canv...
skia/ext/platform_canvas.cc:124: SK_API CGContextRef GetBitmapContext(const
SkCanvas& canvas) {
On 2015/11/25 20:33:04, f(malita) wrote:
> On 2015/11/25 15:50:35, Tom Hudson wrote:
> > On 2015/11/24 23:13:23, f(malita) wrote:
> > > I don't think we need SK_API here.
> > 
> > I think we do, see header file for the 5 invocations outside skia/ext/.
> 
> Hmm, how about SetIsPreviewMetafile & IsPreviewMetafile above?  Those are also
> used externally, but we only use SK_API for the decl.

D'oh, yes. Done.

[Tom Hudson, 2015-11-30 16:14:49]

The CQ bit was checked by tomhudson@chromium.org

[Tom Hudson, 2015-11-30 16:14:49]

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/1462163002/#ps160001
(title: "Trim superfluous SK_API")

[commit-bot: I haz the power, 2015-11-30 16:15:48]

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

[commit-bot: I haz the power, 2015-11-30 17:36:57]

Description was changed from

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

All changes outside skia/ext/ should be purely mechanical renamings
or trimming of include lists.

R=fmalita@chromium.org
TBR=thestig@chromium.org,danakj@chromium.org,piman@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

All changes outside skia/ext/ should be purely mechanical renamings
or trimming of include lists.

R=fmalita@chromium.org
TBR=thestig@chromium.org,danakj@chromium.org,piman@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-11-30 17:36:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-30 17:37:37]

Description was changed from

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

All changes outside skia/ext/ should be purely mechanical renamings
or trimming of include lists.

R=fmalita@chromium.org
TBR=thestig@chromium.org,danakj@chromium.org,piman@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
The API of the metadata helpers doesn't depend on PlatformDevice.
Neither do the typedefs moved to the new platform_surface.h.
Some #includes of platform_device.h were already unnecessary.

All changes outside skia/ext/ should be purely mechanical renamings
or trimming of include lists.

R=fmalita@chromium.org
TBR=thestig@chromium.org,danakj@chromium.org,piman@chromium.org
BUG=543755
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/31521a8fe53dda37a9a838402693b4d2060ec432
Cr-Commit-Position: refs/heads/master@{#362151}
==========

[commit-bot: I haz the power, 2015-11-30 17:37:38]

Patchset 9 (id:??) landed as
https://crrev.com/31521a8fe53dda37a9a838402693b4d2060ec432
Cr-Commit-Position: refs/heads/master@{#362151}