Add bitmaps and skp output to Skia port

Add bitmaps and skp output to Skia port

This is a first-cut at supporting bitmaps.

Also added a --skp option to pdfium_test to generate
a Skia picture file. The picture file can be loaded
in Skia's SampleApp, debugger, or skiaserver to
examine the generated picture.

(This also includes fixes suggested in the prior Skia CL.
My apologies for fat-fingers abandoning that one.)

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

Committed: https://pdfium.googlesource.com/pdfium/+/399be5bf559f72d4649a60320a3d802f6b21780b

[caryclark, 2016-03-09 22:33:23]

Description was changed from

==========
wip; add skia bitmap support

Working on replacing agg with Skia to draw bitmaps.

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

to

==========
wip; add skia bitmap support

Working on replacing agg with Skia to draw bitmaps.

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

[caryclark, 2016-03-09 22:33:23]

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

[caryclark, 2016-03-09 22:40:15]

Dan, this is very much work-in-progress so don't feel like you have to spend
much time on it, but I want to make sure that this is going in the direction you
want to go.

I added some simple support for a few bitmap types to draw to Skia. With that,
I've proven to myself that I can draw paths, text, and bitmaps, all of the basic
graphics on a page.

While testing PDFs on my machine, I found that I couldn't get very far finding
my own bugs, limited to printf and a PNG output.

To that end, I added output to a SKP, the Skia Picture format. In theory, this
means that there's no output bitmap allocated and vector art stays vector. 

This opens up a wide arrange of debugging tools that will help me understand why
things are drawing incorrectly or not at all. Additionally, SKP or something
like it will allow PDFium to fit in to other Skia tool-flow, like, for instance,
PDF -> SKP -> SVG.

Anyways, I'm long-winded as usual. Please take a (gentle) look.

[dsinclair, 2016-03-10 01:27:10]

On 2016/03/09 22:40:15, caryclark wrote:
> Dan, this is very much work-in-progress so don't feel like you have to spend
> much time on it, but I want to make sure that this is going in the direction
you
> want to go.
> 
> I added some simple support for a few bitmap types to draw to Skia. With that,
> I've proven to myself that I can draw paths, text, and bitmaps, all of the
basic
> graphics on a page.
> 
> While testing PDFs on my machine, I found that I couldn't get very far finding
> my own bugs, limited to printf and a PNG output.
> 
> To that end, I added output to a SKP, the Skia Picture format. In theory, this
> means that there's no output bitmap allocated and vector art stays vector. 
> 
> This opens up a wide arrange of debugging tools that will help me understand
why
> things are drawing incorrectly or not at all. Additionally, SKP or something
> like it will allow PDFium to fit in to other Skia tool-flow, like, for
instance,
> PDF -> SKP -> SVG.
> 
> Anyways, I'm long-winded as usual. Please take a (gentle) look.


Haven't looked at the CL yet, but I love the idea of having a SKP output option.
Being able to use the picture_viewer in tracing or the standalone skia one makes
it a lot simpler to find inefficiencies in what we're doing.

[dsinclair, 2016-03-10 14:38:54]

Left a few general comments. The one big issue is we can't change the signature
on FPDF_FFLDraw as it's part of our public/ API. We'll need to add a secondary
method, FPDF_FFLDraw<something> that handles a RECORDER instead of a BITMAP.

https://codereview.chromium.org/1776313002/diff/20001/core/include/fxge/fx_ge.h
File core/include/fxge/fx_ge.h (right):

https://codereview.chromium.org/1776313002/diff/20001/core/include/fxge/fx_ge...
core/include/fxge/fx_ge.h:451: void* m_recorder;
Is there a better type we could use for recorder then void*?  (Even just a using
SkiaDeviceRecorder=void*; or something)

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

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:211: // note that PDF's y-axis goes up;
Skia's y-axis goes down
In the long run, should we add a toSkia on the CFX objects which does this
conversion and return a flipped CFX object, otherwise, we're going to forget to
flip at some point.

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:388: spaint.setAntiAlias(TRUE);
If setAntiAlias takes a bool, you can leave this as true. I'd like to get rid of
most of the TRUE/FALSE, but we only change then when we've managed to drop
FX_BOOL in favour of bool (not all the FX_BOOL's are used as binary to make
things more fun, heh).

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:398: pRect->left = 0;
Does this need a todo to take the pRect into account?

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:453: uint8_t* dstStorage = nullptr;
std::unique_ptr then we can drop the if/free at the end.

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:501: SkMatrix skMatrix =
SkMatrix::MakeScale(1.f / width, 1.f / height);
nit: we try to leave 1 blank line after un-{}'d bodies.

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

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.h:150: SkPictureRecorder* m_recorder;
This should be named m_pRecorder.

Can this be put in std::unique_ptr's so we don't have to worry about cleaning it
up?

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfformfil...
File fpdfsdk/src/fpdfformfill.cpp (right):

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfformfil...
fpdfsdk/src/fpdfformfill.cpp:292: void* recorder,
This is part of the public API, so we can't change. You could add a second call
to the api and just have the old one call the new with a nullptr.

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfview.cpp
File fpdfsdk/src/fpdfview.cpp (right):

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfview.cp...
fpdfsdk/src/fpdfview.cpp:678: CRenderContext* pContext = new CRenderContext;
std::unique_ptr?

https://codereview.chromium.org/1776313002/diff/20001/public/fpdf_formfill.h
File public/fpdf_formfill.h (right):

https://codereview.chromium.org/1776313002/diff/20001/public/fpdf_formfill.h#...
public/fpdf_formfill.h:1503: void* recorder,
Can't modify this as it's public API.

If we add a second method for this, we should probably add a FPDF_RECORDER
typedef to be consistent with BITMAP and FORMHANDLE.

https://codereview.chromium.org/1776313002/diff/20001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/1776313002/diff/20001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:216: fprintf(stderr, "Filname %s is too long\n",
filename);
nit: filename

https://codereview.chromium.org/1776313002/diff/20001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:473: void* recorder = FPDF_RenderPageSkp(page, width,
height);
unique_ptr

[caryclark, 2016-03-10 20:44:55]

https://codereview.chromium.org/1776313002/diff/20001/core/include/fxge/fx_ge.h
File core/include/fxge/fx_ge.h (right):

https://codereview.chromium.org/1776313002/diff/20001/core/include/fxge/fx_ge...
core/include/fxge/fx_ge.h:451: void* m_recorder;
On 2016/03/10 14:38:54, dsinclair wrote:
> Is there a better type we could use for recorder then void*?  (Even just a
using
> SkiaDeviceRecorder=void*; or something)

replaced void* with FPDF_RECORDER in C interfaces and SkPictureRecorder* in
Skia-centric code. (Done)

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

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:211: // note that PDF's y-axis goes up;
Skia's y-axis goes down
On 2016/03/10 14:38:54, dsinclair wrote:
> In the long run, should we add a toSkia on the CFX objects which does this
> conversion and return a flipped CFX object, otherwise, we're going to forget
to
> flip at some point.

It's not so clear to me what the correct solution is here. Would it be OK with
you if some more prototyping is complete before deciding this?

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:388: spaint.setAntiAlias(TRUE);
On 2016/03/10 14:38:54, dsinclair wrote:
> If setAntiAlias takes a bool, you can leave this as true. I'd like to get rid
of
> most of the TRUE/FALSE, but we only change then when we've managed to drop
> FX_BOOL in favour of bool (not all the FX_BOOL's are used as binary to make
> things more fun, heh).

Done.

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:398: pRect->left = 0;
On 2016/03/10 14:38:54, dsinclair wrote:
> Does this need a todo to take the pRect into account?

Done.

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:453: uint8_t* dstStorage = nullptr;
On 2016/03/10 14:38:54, dsinclair wrote:
> std::unique_ptr then we can drop the if/free at the end.

Done.

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:501: SkMatrix skMatrix =
SkMatrix::MakeScale(1.f / width, 1.f / height);
On 2016/03/10 14:38:54, dsinclair wrote:
> nit: we try to leave 1 blank line after un-{}'d bodies.

Done.

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

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.h:150: SkPictureRecorder* m_recorder;
On 2016/03/10 14:38:54, dsinclair wrote:
> This should be named m_pRecorder.
> 
> Can this be put in std::unique_ptr's so we don't have to worry about cleaning
it
> up?

The lifetime of the recorder is similar to a bitmap -- as PDFium is structured,
the bitmap isn't owned by the device or driver, but by the client caller. I put
a unique_ptr in pdfium_test.cc to mimic this structure. (done)

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfformfil...
File fpdfsdk/src/fpdfformfill.cpp (right):

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfformfil...
fpdfsdk/src/fpdfformfill.cpp:292: void* recorder,
On 2016/03/10 14:38:54, dsinclair wrote:
> This is part of the public API, so we can't change. You could add a second
call
> to the api and just have the old one call the new with a nullptr.

Done.

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfview.cpp
File fpdfsdk/src/fpdfview.cpp (right):

https://codereview.chromium.org/1776313002/diff/20001/fpdfsdk/src/fpdfview.cp...
fpdfsdk/src/fpdfview.cpp:678: CRenderContext* pContext = new CRenderContext;
On 2016/03/10 14:38:54, dsinclair wrote:
> std::unique_ptr?

Done.

https://codereview.chromium.org/1776313002/diff/20001/public/fpdf_formfill.h
File public/fpdf_formfill.h (right):

https://codereview.chromium.org/1776313002/diff/20001/public/fpdf_formfill.h#...
public/fpdf_formfill.h:1503: void* recorder,
On 2016/03/10 14:38:54, dsinclair wrote:
> Can't modify this as it's public API.
> 
> If we add a second method for this, we should probably add a FPDF_RECORDER
> typedef to be consistent with BITMAP and FORMHANDLE.

Done.

https://codereview.chromium.org/1776313002/diff/20001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/1776313002/diff/20001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:216: fprintf(stderr, "Filname %s is too long\n",
filename);
On 2016/03/10 14:38:54, dsinclair wrote:
> nit: filename

Done.

https://codereview.chromium.org/1776313002/diff/20001/samples/pdfium_test.cc#...
samples/pdfium_test.cc:473: void* recorder = FPDF_RenderPageSkp(page, width,
height);
On 2016/03/10 14:38:54, dsinclair wrote:
> unique_ptr

Done.

[dsinclair, 2016-03-10 20:47:12]

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

https://codereview.chromium.org/1776313002/diff/20001/core/src/fxge/skia/fx_s...
core/src/fxge/skia/fx_skia_device.cpp:211: // note that PDF's y-axis goes up;
Skia's y-axis goes down
On 2016/03/10 20:44:55, caryclark wrote:
> On 2016/03/10 14:38:54, dsinclair wrote:
> > In the long run, should we add a toSkia on the CFX objects which does this
> > conversion and return a flipped CFX object, otherwise, we're going to forget
> to
> > flip at some point.
> 
> It's not so clear to me what the correct solution is here. Would it be OK with
> you if some more prototyping is complete before deciding this?


Figuring it out later is fine, just something we should keep in mind for the
long term.

[caryclark, 2016-03-10 22:11:27]

Description was changed from

==========
wip; add skia bitmap support

Working on replacing agg with Skia to draw bitmaps.

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

to

==========
add bitmaps and skp output to Skia port

Working on replacing agg with Skia to draw bitmaps.

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

[caryclark, 2016-03-10 22:14:48]

Description was changed from

==========
add bitmaps and skp output to Skia port

Working on replacing agg with Skia to draw bitmaps.

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

to

==========
Add bitmaps and skp output to Skia port

This is a first-cut at supporting bitmaps.

Also added a --skp option to pdfium_test to generate
a Skia picture file. The picture file can be loaded
in Skia's SampleApp, debugger, or skiaserver to
examine the generated picture.

(This also includes fixes suggested in the prior Skia CL.
My apologies for fat-fingers abandoning that one.)

R=dsinclair
==========

[caryclark, 2016-03-10 22:17:27]

Description was changed from

==========
Add bitmaps and skp output to Skia port

This is a first-cut at supporting bitmaps.

Also added a --skp option to pdfium_test to generate
a Skia picture file. The picture file can be loaded
in Skia's SampleApp, debugger, or skiaserver to
examine the generated picture.

(This also includes fixes suggested in the prior Skia CL.
My apologies for fat-fingers abandoning that one.)

R=dsinclair
==========

to

==========
Add bitmaps and skp output to Skia port

This is a first-cut at supporting bitmaps.

Also added a --skp option to pdfium_test to generate
a Skia picture file. The picture file can be loaded
in Skia's SampleApp, debugger, or skiaserver to
examine the generated picture.

(This also includes fixes suggested in the prior Skia CL.
My apologies for fat-fingers abandoning that one.)

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

[caryclark, 2016-03-10 22:17:27]

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

[caryclark, 2016-03-10 22:17:54]

ready for more formal review

[dsinclair, 2016-03-10 22:23:45]

I'm heading out for the weekend, I'll try to take a look but it maybe Monday.
(Unless tsepez@ gets to it before me)

[Tom Sepez, 2016-03-10 22:55:32]

https://codereview.chromium.org/1776313002/diff/160001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1776313002/diff/160001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.cpp:454: std::unique_ptr<uint8_t[]>
dstStorage((uint8_t*) nullptr);
Can't you just declare this as 
  std::unique_ptr<uint8_t[]> dstStorage;

https://codereview.chromium.org/1776313002/diff/160001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.cpp:461: uint8_t* dst8Pixels =
dstStorage.get();
are we sure that width*height can't overflow?

https://codereview.chromium.org/1776313002/diff/160001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/1776313002/diff/160001/samples/pdfium_test.cc...
samples/pdfium_test.cc:34: #ifdef _SKIA_SUPPORT_
Technicaly, identifiers beginning with an underscore and a CAP letter are
reserved for the implementation.  Is this a new #define, or something
pre-exisiting?

https://codereview.chromium.org/1776313002/diff/160001/samples/pdfium_test.cc...
samples/pdfium_test.cc:681: "  --ppm - write page images
<pdf-name>.<page-number>.ppm\n"
Why duplicated ppm line in both halves of the ifdef? Can we just move the
semicolon to a line of its own?

[caryclark, 2016-03-11 12:40:34]

https://codereview.chromium.org/1776313002/diff/160001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1776313002/diff/160001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.cpp:454: std::unique_ptr<uint8_t[]>
dstStorage((uint8_t*) nullptr);
On 2016/03/10 22:55:32, Tom Sepez wrote:
> Can't you just declare this as 
>   std::unique_ptr<uint8_t[]> dstStorage;

Done.

https://codereview.chromium.org/1776313002/diff/160001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.cpp:461: uint8_t* dst8Pixels =
dstStorage.get();
On 2016/03/10 22:55:32, Tom Sepez wrote:
> are we sure that width*height can't overflow?

Done.

https://codereview.chromium.org/1776313002/diff/160001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/1776313002/diff/160001/samples/pdfium_test.cc...
samples/pdfium_test.cc:34: #ifdef _SKIA_SUPPORT_
On 2016/03/10 22:55:32, Tom Sepez wrote:
> Technicaly, identifiers beginning with an underscore and a CAP letter are
> reserved for the implementation.  Is this a new #define, or something
> pre-exisiting?

Changed the define so that it is clear what is specific to the app and what is
is specific to the impementation (done)

https://codereview.chromium.org/1776313002/diff/160001/samples/pdfium_test.cc...
samples/pdfium_test.cc:681: "  --ppm - write page images
<pdf-name>.<page-number>.ppm\n"
On 2016/03/10 22:55:32, Tom Sepez wrote:
> Why duplicated ppm line in both halves of the ifdef? Can we just move the
> semicolon to a line of its own?

The style checker complained when a single semicolon was on a line by itself. I
figured out how to get around this by adding an empty string. (done)

[Tom Sepez, 2016-03-11 17:38:46]

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/agg/fx_a...
File core/src/fxge/agg/fx_agg_driver.h (right):

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/agg/fx_a...
core/src/fxge/agg/fx_agg_driver.h:136: virtual uint8_t* GetBuffer() const;
Nit: Pre-existing condition, but why is this virtual?  Its not an override --
does anything subclass CFX_AggDeviceDriver?

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.cpp:460: dstStorage.reset(new
uint8_t[(int64_t) width * height]);
Unfortunately, it's almost impossible to get this right in C++ across all
platforms.

This isn't safe on a 32-bit platform with a 32-bit size_t.  As written, We'd
perform the multiplication in 64-bits, but then pass this to: 
  operator new[](size_t size){}

and truncate to 32 bits.  We may allocate too little, and then step past the end
of it in the code below before the src_row calculation overflows, even. The
pathological case is something like 65535x65538 where the result in 32-bits is
65534, less than even one scan line in size.

For raw buffers such as these, we added some malloc-like helpers that avoid
these overflows to PDFium.  See FX_Alloc2d() in core/include/fxcrt/fx_memory.h.

To use this with a unique_ptr, you'll need the helper FXFreeDeleter in
pdfium/core/include/fxcrt/fx_basic.h,
e.g.

std::unique_ptr<uint8_t, FxFreeDeleter> dstStorage;

then

dstStorage.reset(FX_Alloc2D(uint8_t, width, height));

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.h:150: SkPictureRecorder* m_pRecorder;
nit: can this be SkPictureRecorder* const m_pRecorder;

https://codereview.chromium.org/1776313002/diff/220001/fpdfsdk/src/fpdfformfi...
File fpdfsdk/src/fpdfformfill.cpp (right):

https://codereview.chromium.org/1776313002/diff/220001/fpdfsdk/src/fpdfformfi...
fpdfsdk/src/fpdfformfill.cpp:341:
pDevice->AttachRecorder((SkPictureRecorder*)recorder);
nit: prefer static_cast<>

[caryclark, 2016-03-13 01:02:38]

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/agg/fx_a...
File core/src/fxge/agg/fx_agg_driver.h (right):

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/agg/fx_a...
core/src/fxge/agg/fx_agg_driver.h:136: virtual uint8_t* GetBuffer() const;
On 2016/03/11 17:38:45, Tom Sepez wrote:
> Nit: Pre-existing condition, but why is this virtual?  Its not an override --
> does anything subclass CFX_AggDeviceDriver?

I have no idea. Over time, I hope to completely remove all agg from the Skia
driver implementation, so I bet this will change. I'll keep this in mind as I
continue to refactor the Skia driver code.

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.cpp (right):

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.cpp:460: dstStorage.reset(new
uint8_t[(int64_t) width * height]);
On 2016/03/11 17:38:45, Tom Sepez wrote:
> Unfortunately, it's almost impossible to get this right in C++ across all
> platforms.
> 
> This isn't safe on a 32-bit platform with a 32-bit size_t.  As written, We'd
> perform the multiplication in 64-bits, but then pass this to: 
>   operator new[](size_t size){}
> 
> and truncate to 32 bits.  We may allocate too little, and then step past the
end
> of it in the code below before the src_row calculation overflows, even. The
> pathological case is something like 65535x65538 where the result in 32-bits is
> 65534, less than even one scan line in size.
> 
> For raw buffers such as these, we added some malloc-like helpers that avoid
> these overflows to PDFium.  See FX_Alloc2d() in
core/include/fxcrt/fx_memory.h.
> 
> To use this with a unique_ptr, you'll need the helper FXFreeDeleter in
> pdfium/core/include/fxcrt/fx_basic.h,
> e.g.
> 
> std::unique_ptr<uint8_t, FxFreeDeleter> dstStorage;
> 
> then
> 
> dstStorage.reset(FX_Alloc2D(uint8_t, width, height));
> 
> 
> 
> 

Done.

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/1776313002/diff/220001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.h:150: SkPictureRecorder* m_pRecorder;
On 2016/03/11 17:38:46, Tom Sepez wrote:
> nit: can this be SkPictureRecorder* const m_pRecorder;

Done.

https://codereview.chromium.org/1776313002/diff/220001/fpdfsdk/src/fpdfformfi...
File fpdfsdk/src/fpdfformfill.cpp (right):

https://codereview.chromium.org/1776313002/diff/220001/fpdfsdk/src/fpdfformfi...
fpdfsdk/src/fpdfformfill.cpp:341:
pDevice->AttachRecorder((SkPictureRecorder*)recorder);
On 2016/03/11 17:38:46, Tom Sepez wrote:
> nit: prefer static_cast<>

Done.

[dsinclair, 2016-03-14 13:53:41]

https://codereview.chromium.org/1776313002/diff/240001/core/src/fxge/skia/fx_...
File core/src/fxge/skia/fx_skia_device.h (right):

https://codereview.chromium.org/1776313002/diff/240001/core/src/fxge/skia/fx_...
core/src/fxge/skia/fx_skia_device.h:23: CFX_SkiaDeviceDriver(SkPictureRecorder*
recorder);
nit: explicit

https://codereview.chromium.org/1776313002/diff/240001/samples/pdfium_test.cc
File samples/pdfium_test.cc (right):

https://codereview.chromium.org/1776313002/diff/240001/samples/pdfium_test.cc...
samples/pdfium_test.cc:476: (SkPictureRecorder*)FPDF_RenderPageSkp(page, width,
height));
nit: static_cast<>()

[dsinclair, 2016-03-14 13:54:08]

lgtm

[Tom Sepez, 2016-03-14 17:58:31]

lgtm

[caryclark, 2016-03-14 20:51:33]

Description was changed from

==========
Add bitmaps and skp output to Skia port

This is a first-cut at supporting bitmaps.

Also added a --skp option to pdfium_test to generate
a Skia picture file. The picture file can be loaded
in Skia's SampleApp, debugger, or skiaserver to
examine the generated picture.

(This also includes fixes suggested in the prior Skia CL.
My apologies for fat-fingers abandoning that one.)

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

to

==========
Add bitmaps and skp output to Skia port

This is a first-cut at supporting bitmaps.

Also added a --skp option to pdfium_test to generate
a Skia picture file. The picture file can be loaded
in Skia's SampleApp, debugger, or skiaserver to
examine the generated picture.

(This also includes fixes suggested in the prior Skia CL.
My apologies for fat-fingers abandoning that one.)

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

Committed:
https://pdfium.googlesource.com/pdfium/+/399be5bf559f72d4649a60320a3d802f6b21...
==========

[caryclark, 2016-03-14 20:51:35]

Committed patchset #13 (id:240001) manually as
399be5bf559f72d4649a60320a3d802f6b21780b (presubmit successful).