SkDocument base for pdf, xps, etc.

SkDocument base for pdf, xps, etc.

R=scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=9476

[scroggo, 2013-06-07 19:35:23]

lgtm

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:15: fDoc = new SkPDFDocument;
SkNEW

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:22: delete fDoc;
SkDELETE

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:36: fDevice = new SkPDFDevice(pageS, contentS,
matrix);
SkNEW_ARGS

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:58: SkSafeUnref(fCanvas);
Is there a way for fCanvas and fDevice to be non NULL here? It seems like
onEndPage is guaranteed to be called before onClose.

[epoger, 2013-06-07 19:39:04]

https://codereview.chromium.org/16660002/diff/3001/include/core/SkDocument.h
File include/core/SkDocument.h (right):

https://codereview.chromium.org/16660002/diff/3001/include/core/SkDocument.h#...
include/core/SkDocument.h:17: class SkDocument : public SkRefCnt {
Since this is going in include/core , can you please add some
commentary/documentation indicating what it's used for, its API, etc?

[reed1, 2013-06-07 19:50:40]

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:15: fDoc = new SkPDFDocument;
On 2013/06/07 19:35:23, scroggo wrote:
> SkNEW

Done.

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:22: delete fDoc;
On 2013/06/07 19:35:23, scroggo wrote:
> SkDELETE

Done.

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:36: fDevice = new SkPDFDevice(pageS, contentS,
matrix);
On 2013/06/07 19:35:23, scroggo wrote:
> SkNEW_ARGS

Done.

https://codereview.chromium.org/16660002/diff/1/src/doc/SkDocument_PDF.cpp#ne...
src/doc/SkDocument_PDF.cpp:58: SkSafeUnref(fCanvas);
On 2013/06/07 19:35:23, scroggo wrote:
> Is there a way for fCanvas and fDevice to be non NULL here? It seems like
> onEndPage is guaranteed to be called before onClose.

Agreed. Simplified.

https://codereview.chromium.org/16660002/diff/3001/include/core/SkDocument.h
File include/core/SkDocument.h (right):

https://codereview.chromium.org/16660002/diff/3001/include/core/SkDocument.h#...
include/core/SkDocument.h:17: class SkDocument : public SkRefCnt {
On 2013/06/07 19:39:04, epoger wrote:
> Since this is going in include/core , can you please add some
> commentary/documentation indicating what it's used for, its API, etc?

Done.

[reed1, 2013-06-07 20:09:01]

updated dox, that we don't take ownership of the stream

[scroggo, 2013-06-07 20:12:32]

https://codereview.chromium.org/16660002/diff/16001/include/core/SkDocument.h
File include/core/SkDocument.h (right):

https://codereview.chromium.org/16660002/diff/16001/include/core/SkDocument.h...
include/core/SkDocument.h:37: *  there is an error trying to create the doc,
returns NULL.
If got lost here.

https://codereview.chromium.org/16660002/diff/16001/src/doc/SkDocument_PDF.cpp
File src/doc/SkDocument_PDF.cpp (right):

https://codereview.chromium.org/16660002/diff/16001/src/doc/SkDocument_PDF.cp...
src/doc/SkDocument_PDF.cpp:79: SkFILEWStream* stream = SkNEW_ARGS(SkFILEWStream,
(path));
This one does need to be deleted.

[scroggo, 2013-06-07 20:29:46]

lgtm

[reed1, 2013-06-07 20:30:23]

Committed patchset #5 manually as r9476 (presubmit successful).

[wrong vandebo, 2013-06-10 16:41:11]

https://codereview.chromium.org/16660002/diff/23001/include/core/SkDocument.h
File include/core/SkDocument.h (right):

https://codereview.chromium.org/16660002/diff/23001/include/core/SkDocument.h...
include/core/SkDocument.h:33: static SkDocument* CreatePDF(const char
filename[]);
Are these factory methods and you'll have additional methods for other
multi-page output formats, i.e. XPS, etc ?  It's a bit strange to how them on
the base class otherwise.

However, based on the signatures, it looks like you'd end up with two methods
per file format.  It seems that you could just make an enum with all the file
formats and make these methods generic.  i.e.

enum DocumentType {
 kPDF,
 kEMF,
 ...
}

CreateDocument(DocumentType type, const char filename[]);

https://codereview.chromium.org/16660002/diff/23001/include/core/SkDocument.h...
include/core/SkDocument.h:45: static SkDocument* CreatePDF(SkWStream*, void
(*Done)(SkWStream*) = NULL);
PDF has a couple of document options: disable compression, disable link
annotations.  I'm not sure if these are generic enough, but it seems like this
is the place to pipe them through.

https://codereview.chromium.org/16660002/diff/23001/include/core/SkDocument.h...
include/core/SkDocument.h:53: const SkRect* content = NULL);
I tried using only a rect to specify the content area at the device level but it
didn't work out.  I don't recall if there was a deeper issue or if was just that
the device layer needed a transform and using a rect at the canvas layer would
work fine. 

With both interfaces, you still need another mechanism to enable drawing in the
margin area.  In Chrome this is used to add headers and footers.

https://codereview.chromium.org/16660002/diff/23001/include/core/SkDocument.h...
include/core/SkDocument.h:68: void close();
A feature that the PDF code has that's not representable here is to emit
multiple documents with different sets of drawn pages.  In Chrome the print
preview flow uses this to decrease latency; while creating a single PDF with all
<n> pages, it also creates <n> single page PDFs.  The single page PDFs are
streamed to the print preview page as they are ready.  A on SkDocument to
combine multiple SkDocuments or a method to get an SkDocument for a subset of
pages would preserve that functionality.

https://codereview.chromium.org/16660002/diff/23001/src/doc/SkDocument.cpp
File src/doc/SkDocument.cpp (right):

https://codereview.chromium.org/16660002/diff/23001/src/doc/SkDocument.cpp#ne...
src/doc/SkDocument.cpp:54: void SkDocument::endPage() {
It seems like this method isn't strictly needed and the method could be
generated implicitly.  I don't object to the method, just noting that it doesn't
seem necessary.