get skia port working again

Small change to enable skia-backed version of pdfium.

This only directs paths to use Skia. Text and bitmaps
still use antigrain.

Clipping is disabled -- still figuring out pdfium's model
for saving and restoring clips.

Deleting the Skia canvas is disabled -- there's some build problem
where the pdfium and skia libraries are built with slightly
different options that I haven't tracked down. This is also why
for the moment SkDebugf is defined locally.

With this patch and pdf_use_skia=1 a modestly complicated PDF draws
(mostly) correctly with pdfium_test.

R=dsinclair@chromium.org, tsepez@chromium.org, dsinclair
BUG=

Committed: https://pdfium.googlesource.com/pdfium/+/979a36af81e6306c1e8b1717a6946dabc757d07d

[caryclark, 2016-03-03 18:10:51]

Description was changed from

==========
wip; play around with enabling skia

BUG=
==========

to

==========
Small change to enable skia-backed version of pdfium.

This only directs paths to use Skia. Text and bitmaps still use antigrain.

Clipping is disabled -- still figuring out pdfium's model for saving and
restoring clips.

Deleting the Skia canvas is disabled -- there's some build problem where the
pdfium and skia libraries are built with slightly different options that I
haven't tracked down. This is also why for the moment SkDebugf is defined
locally.

With this patch and pdf_use_skia=1 a modestly complicated PDF draws (mostly)
correctly with pdfium_test.

R=dsinclair
BUG=
==========

[caryclark, 2016-03-03 18:10:51]

caryclark@google.com changed reviewers:
+ dsinclair@chromium.org

[caryclark, 2016-03-03 18:11:29]

Description was changed from

==========
Small change to enable skia-backed version of pdfium.

This only directs paths to use Skia. Text and bitmaps still use antigrain.

Clipping is disabled -- still figuring out pdfium's model for saving and
restoring clips.

Deleting the Skia canvas is disabled -- there's some build problem where the
pdfium and skia libraries are built with slightly different options that I
haven't tracked down. This is also why for the moment SkDebugf is defined
locally.

With this patch and pdf_use_skia=1 a modestly complicated PDF draws (mostly)
correctly with pdfium_test.

R=dsinclair
BUG=
==========

to

==========
Small change to enable skia-backed version of pdfium.

This only directs paths to use Skia. Text and bitmaps 
still use antigrain.

Clipping is disabled -- still figuring out pdfium's model 
for saving and restoring clips.

Deleting the Skia canvas is disabled -- there's some build problem 
where the pdfium and skia libraries are built with slightly 
different options that I haven't tracked down. This is also why 
for the moment SkDebugf is defined locally.

With this patch and pdf_use_skia=1 a modestly complicated PDF draws 
(mostly) correctly with pdfium_test.

R=dsinclair
BUG=
==========

[caryclark, 2016-03-03 18:13:24]

Dan, here's a very early first cut to give you an idea of which direction I'm
heading. Any feedback, positive or negative, is appreciated.

[dsinclair, 2016-03-03 18:15:18]

dsinclair@chromium.org changed reviewers:
+ thestig@chromium.org, tsepez@chromium.org

[dsinclair, 2016-03-03 18:15:18]

Adding a few more reviewers who've been doing this longer then me.

[dsinclair, 2016-03-03 18:40:12]

This is all makes sense to me. Seems like it removes a lot of crufty code as
well.

The random comments about bugs fixed are a little worrying as we don't have the
PDFs, but not much we can do about that.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.cpp (left):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:153: // Therefore, we have to extend the
line by 0.4 pixel here.
I'm not sure I understand this comment, the line was extended because the length
was 0?

Do we need to do anything around this?

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:254: smatrix.setAll(pObject2Device->a,
pObject2Device->c, pObject2Device->e,
Do we need to worry about, what I'm guessing is object to device space
transform?

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:126: #if 0  // fix me : mismatch on
allocator ?
Once we figure out the allocator issue, can we std::unique_ptr<SkCanvas> instead
of SkCanvas*?

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:191: // fix me : don't clip quite yet
nit: fix me should be TODO(caryclark)

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.h:141: void PaintStroke(SkPaint& spaint, const
CFX_GraphStateData* pGraphState);
I believe this should be a SkPaint*

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.h:144: SkCanvas* m_canvas;
Can this be private?

https://codereview.chromium.org/1761453002/diff/40001/pdfium.gyp
File pdfium.gyp (right):

https://codereview.chromium.org/1761453002/diff/40001/pdfium.gyp#newcode563
pdfium.gyp:563: 'include_dirs': [
These should probably be removed and we do third_party/skia/include/... in the
code.

https://codereview.chromium.org/1761453002/diff/40001/pdfium.gyp#newcode567
pdfium.gyp:567: '<(DEPTH)/third_party/skia/include/images',
Out of curiosity, did you check the skia dir out manually? I don't seem to have
one in my checkout? Do we need to add skia to DEPS?

[Tom Sepez, 2016-03-03 19:18:20]

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:25: FX_FLOAT x = pPoints[i].m_PointX, y =
pPoints[i].m_PointY;
nit: one per line.

[caryclark, 2016-03-03 21:03:44]

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.cpp (left):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:153: // Therefore, we have to extend the
line by 0.4 pixel here.
On 2016/03/03 18:40:11, dsinclair wrote:
> I'm not sure I understand this comment, the line was extended because the
length
> was 0?
> 
> Do we need to do anything around this?

An old Skia bug ignored zero-length line segments even if a square or round cap
gave it substance. That bug has since been fixed, eliminating the need for this
workaround.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:254: smatrix.setAll(pObject2Device->a,
pObject2Device->c, pObject2Device->e,
On 2016/03/03 18:40:11, dsinclair wrote:
> Do we need to worry about, what I'm guessing is object to device space
> transform?

There is likely missing code, but the transform should be on the canvas, not on
the geometry as coded here.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:25: FX_FLOAT x = pPoints[i].m_PointX, y =
pPoints[i].m_PointY;
On 2016/03/03 19:18:20, Tom Sepez wrote:
> nit: one per line.

Done.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:126: #if 0  // fix me : mismatch on
allocator ?
On 2016/03/03 18:40:11, dsinclair wrote:
> Once we figure out the allocator issue, can we std::unique_ptr<SkCanvas>
instead
> of SkCanvas*?

I don't know. SkCanvas is a reference counted object, and it's too early for me
to understand its lifetime. I would prefer to have SkCanvas as a stack object
and follow the Chromium model -- I only use a pointer here as a crude workaround
until the build issues are resolved.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:191: // fix me : don't clip quite yet
On 2016/03/03 18:40:11, dsinclair wrote:
> nit: fix me should be TODO(caryclark)

Done.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.h:141: void PaintStroke(SkPaint& spaint, const
CFX_GraphStateData* pGraphState);
On 2016/03/03 18:40:11, dsinclair wrote:
> I believe this should be a SkPaint*

Done.

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.h:144: SkCanvas* m_canvas;
On 2016/03/03 18:40:11, dsinclair wrote:
> Can this be private?

Done.

https://codereview.chromium.org/1761453002/diff/40001/pdfium.gyp
File pdfium.gyp (right):

https://codereview.chromium.org/1761453002/diff/40001/pdfium.gyp#newcode563
pdfium.gyp:563: 'include_dirs': [
On 2016/03/03 18:40:11, dsinclair wrote:
> These should probably be removed and we do third_party/skia/include/... in the
> code.

Done.

https://codereview.chromium.org/1761453002/diff/40001/pdfium.gyp#newcode567
pdfium.gyp:567: '<(DEPTH)/third_party/skia/include/images',
On 2016/03/03 18:40:11, dsinclair wrote:
> Out of curiosity, did you check the skia dir out manually? I don't seem to
have
> one in my checkout? Do we need to add skia to DEPS?

I am doing the build inside a Chromium tree for now. I couldn't figure out a way
to easily do it in a standalone PDFium tree.

To test, I use:
GYP_DEFINES="pdf_use_skia=1" ./build/gyp_chromium -Dcomponent=shared_library
ninja -C out/Debug pdfium_test 
./out/Debug/pdfium_test --png /some/local/pdf/file

An aside: I have the new/delete mismatch with non-shared library builds as well.

[dsinclair, 2016-03-03 21:15:18]

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1761453002/diff/40001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:126: #if 0  // fix me : mismatch on
allocator ?
On 2016/03/03 21:03:44, caryclark wrote:
> On 2016/03/03 18:40:11, dsinclair wrote:
> > Once we figure out the allocator issue, can we std::unique_ptr<SkCanvas>
> instead
> > of SkCanvas*?
> 
> I don't know. SkCanvas is a reference counted object, and it's too early for
me
> to understand its lifetime. I would prefer to have SkCanvas as a stack object
> and follow the Chromium model -- I only use a pointer here as a crude
workaround
> until the build issues are resolved.


Ah, makes sense. Thanks for the info.

https://codereview.chromium.org/1761453002/diff/60001/pdfium.gyp
File pdfium.gyp (left):

https://codereview.chromium.org/1761453002/diff/60001/pdfium.gyp#oldcode558
pdfium.gyp:558: 'core/src/fxge/skia/fx_skia_blitter_new.cpp',
You'll need to do this in BUILD.gn as well.

https://codereview.chromium.org/1761453002/diff/60001/pdfium.gyp#oldcode565
pdfium.gyp:565: '<(DEPTH)/third_party/skia/include/config',
I'm guessing you're using a GN build, otherwise, how does it find the skia
headers now since the paths aren't updated?

[caryclark, 2016-03-03 21:32:59]

Thanks for egging me to try out the GN build. It's much, much, faster than gyp.

https://codereview.chromium.org/1761453002/diff/60001/pdfium.gyp
File pdfium.gyp (left):

https://codereview.chromium.org/1761453002/diff/60001/pdfium.gyp#oldcode558
pdfium.gyp:558: 'core/src/fxge/skia/fx_skia_blitter_new.cpp',
On 2016/03/03 21:15:17, dsinclair wrote:
> You'll need to do this in BUILD.gn as well.

Done.

https://codereview.chromium.org/1761453002/diff/60001/pdfium.gyp#oldcode565
pdfium.gyp:565: '<(DEPTH)/third_party/skia/include/config',
On 2016/03/03 21:15:17, dsinclair wrote:
> I'm guessing you're using a GN build, otherwise, how does it find the skia
> headers now since the paths aren't updated?

Nope, using a gyp build; don't know how it works.
I'll try GN as well.

[dsinclair, 2016-03-03 21:34:53]

lgtm

[Tom Sepez, 2016-03-03 21:45:15]

On 2016/03/03 21:34:53, dsinclair wrote:
> lgtm

Deferring to dan's LGTM.

[caryclark, 2016-03-04 19:35:04]

Description was changed from

==========
Small change to enable skia-backed version of pdfium.

This only directs paths to use Skia. Text and bitmaps 
still use antigrain.

Clipping is disabled -- still figuring out pdfium's model 
for saving and restoring clips.

Deleting the Skia canvas is disabled -- there's some build problem 
where the pdfium and skia libraries are built with slightly 
different options that I haven't tracked down. This is also why 
for the moment SkDebugf is defined locally.

With this patch and pdf_use_skia=1 a modestly complicated PDF draws 
(mostly) correctly with pdfium_test.

R=dsinclair
BUG=
==========

to

==========
Small change to enable skia-backed version of pdfium.

This only directs paths to use Skia. Text and bitmaps
still use antigrain.

Clipping is disabled -- still figuring out pdfium's model
for saving and restoring clips.

Deleting the Skia canvas is disabled -- there's some build problem
where the pdfium and skia libraries are built with slightly
different options that I haven't tracked down. This is also why
for the moment SkDebugf is defined locally.

With this patch and pdf_use_skia=1 a modestly complicated PDF draws
(mostly) correctly with pdfium_test.

R=dsinclair@chromium.org, tsepez@chromium.org, dsinclair
BUG=

Committed:
https://pdfium.googlesource.com/pdfium/+/979a36af81e6306c1e8b1717a6946dabc757...
==========

[caryclark, 2016-03-04 19:35:06]

Committed patchset #5 (id:80001) manually as
979a36af81e6306c1e8b1717a6946dabc757d07d (presubmit successful).