Update the SkDocument interface to allow for 1) abort won't emit pdf, 2) close can report success/f…

Add SkPDFDeviceFlatenner which extends SkPDFDevice to add support to flatten the path and the text when we have perspective.
prepare to deprecate SkPDFDevice constructor, and route gm and render_pdfs to use SkDocument::Create pdf interface instead. - controlled by a flag 
add comments where we are supposed to flatten other features (paint, shaders, ... )

Committed: http://code.google.com/p/skia/source/detail?r=11751

[edisonn, 2013-09-26 19:45:56]

Please continue the code review here, I messed up my git and I am continuing the
code review here.

Leon, do you think we can add a rect to your SkPicture::EncodeBitmap?

[bungeman-skia, 2013-09-26 19:52:38]

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

https://codereview.chromium.org/24811002/diff/1/include/core/SkDocument.h#new...
include/core/SkDocument.h:11: #include "SkPDFCallbacks.h"
core should not depend on PDF.

[scroggo, 2013-09-26 21:10:31]

On 2013/09/26 19:45:56, edisonn wrote:
> Please continue the code review here, I messed up my git and I am continuing
the
> code review here.
> 
> Leon, do you think we can add a rect to your SkPicture::EncodeBitmap?

FWIW - you can use

$ git cl patch <issue number>

to resume where you left off and keep the same code review.

[scroggo, 2013-09-26 22:05:54]

https://codereview.chromium.org/24811002/diff/1/include/pdf/SkPDFCallbacks.h
File include/pdf/SkPDFCallbacks.h (right):

https://codereview.chromium.org/24811002/diff/1/include/pdf/SkPDFCallbacks.h#...
include/pdf/SkPDFCallbacks.h:26: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
Should these be inside a namespace?

[vandebo (ex-Chrome), 2013-09-27 15:50:16]

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp#newcode628
gm/gmmain.cpp:628: canvas->setMatrix(initialTransform);
Until the old constructor is removed, I think it's best to use the old code
path. There's explicit gm support for initialtranforms because there were bugs
that only showed up with a non-default initialtranform and there are now tests
verifying that doesn't regress. Yes, you are going to remove it, but until then,
It's probably best to keep the test coverage.

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

https://codereview.chromium.org/24811002/diff/1/include/core/SkDocument.h#new...
include/core/SkDocument.h:62: *  If mediaBox is null, then we set the mediabox
to the trim rectangle.
Please document the args better.  What affect does the combination of trimSize
and mediaBox have?

https://codereview.chromium.org/24811002/diff/1/src/doc/SkCanvasSimplifier.h
File src/doc/SkCanvasSimplifier.h (right):

https://codereview.chromium.org/24811002/diff/1/src/doc/SkCanvasSimplifier.h#...
src/doc/SkCanvasSimplifier.h:27: class SkCanvasSimplifier : public SkCanvas {
There's probably a better name for this: SkPerspectiveFlatteningCanvas ?

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:873: // TODO(edisonn): should we pass here the DCT
encoder of the destination device?
I would say so.

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:875: SkCanvasSimplifier canvas(&pattern);
This doesn't help you here.  I think this method will require explicit support
for perspective.  Calculation of the bbox needs to take the perspective into
consideration.

If you look below, canvas is used only with affine transforms - there's no way
that the canvas will see perspective transforms.  Above on line 844/5 is where a
perspective transform might come into the picture, but right now it would just
be passed through to the FormXObject matrix property and won't be handled
properly.

https://codereview.chromium.org/24811002/diff/1/tools/PdfRenderer.cpp
File tools/PdfRenderer.cpp (right):

https://codereview.chromium.org/24811002/diff/1/tools/PdfRenderer.cpp#newcode58
tools/PdfRenderer.cpp:58: // TODO(edisonn): SkDocument should report failure
somehow!
Didn't you change close to return a bool, or does this comment mean something
else?

[edisonn, 2013-10-04 19:40:37]

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp#newcode628
gm/gmmain.cpp:628: canvas->setMatrix(initialTransform);
On 2013/09/27 15:50:17, vandebo wrote:
> Until the old constructor is removed, I think it's best to use the old code
> path. There's explicit gm support for initialtranforms because there were bugs
> that only showed up with a non-default initialtranform and there are now tests
> verifying that doesn't regress. Yes, you are going to remove it, but until
then,
> It's probably best to keep the test coverage.

Now SkPDFDeviceFlatenable will call the old constructor

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

https://codereview.chromium.org/24811002/diff/1/include/core/SkDocument.h#new...
include/core/SkDocument.h:11: #include "SkPDFCallbacks.h"
On 2013/09/26 19:52:39, bungeman1 wrote:
> core should not depend on PDF.

Done.

https://codereview.chromium.org/24811002/diff/1/include/core/SkDocument.h#new...
include/core/SkDocument.h:62: *  If mediaBox is null, then we set the mediabox
to the trim rectangle.
On 2013/09/27 15:50:17, vandebo wrote:
> Please document the args better.  What affect does the combination of trimSize
> and mediaBox have?

Done.

https://codereview.chromium.org/24811002/diff/1/src/doc/SkCanvasSimplifier.h
File src/doc/SkCanvasSimplifier.h (right):

https://codereview.chromium.org/24811002/diff/1/src/doc/SkCanvasSimplifier.h#...
src/doc/SkCanvasSimplifier.h:27: class SkCanvasSimplifier : public SkCanvas {
On 2013/09/27 15:50:17, vandebo wrote:
> There's probably a better name for this: SkPerspectiveFlatteningCanvas ?

removed

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:875: SkCanvasSimplifier canvas(&pattern);
On 2013/09/27 15:50:17, vandebo wrote:
> This doesn't help you here.  I think this method will require explicit support
> for perspective.  Calculation of the bbox needs to take the perspective into
> consideration.
> 
> If you look below, canvas is used only with affine transforms - there's no way
> that the canvas will see perspective transforms.  Above on line 844/5 is where
a
> perspective transform might come into the picture, but right now it would just
> be passed through to the FormXObject matrix property and won't be handled
> properly.

Done.

https://codereview.chromium.org/24811002/diff/1/tools/PdfRenderer.cpp
File tools/PdfRenderer.cpp (right):

https://codereview.chromium.org/24811002/diff/1/tools/PdfRenderer.cpp#newcode58
tools/PdfRenderer.cpp:58: // TODO(edisonn): SkDocument should report failure
somehow!
On 2013/09/27 15:50:17, vandebo wrote:
> Didn't you change close to return a bool, or does this comment mean something
> else?

Done.

[reed1, 2013-10-07 13:24:54]

https://codereview.chromium.org/24811002/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/24001/gm/gmmain.cpp#newcode626
gm/gmmain.cpp:626: SkAutoTUnref<SkDocument> pdfDoc(SkDocument::CreatePDF(&pdf,
NULL, encode_to_dct_data));
Can we move the SUPPORT_PDF check to inside CreatePDF(), so that the caller just
needs to do a NULL check on the returned document?

https://codereview.chromium.org/24811002/diff/24001/gm/gmmain.cpp#newcode629
gm/gmmain.cpp:629: canvas->setMatrix(initialTransform);
can this be concat instead of setMatrix? The latter is very unfriendly, since it
completely ignores the state of the canvas as returned by beginPage.

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

https://codereview.chromium.org/24811002/diff/24001/include/core/SkDocument.h...
include/core/SkDocument.h:56: SkPicture::EncodeBitmap encoder = NULL);
I wonder, if we end up with more options in the future, if we want to allow
these to be set after construction. e.g. setBitmapEncoder(...), or is it vital
to keep all of them in the constructor (i.e. known a factory time)?

Not requesting a change to this CL, just wondering.

https://codereview.chromium.org/24811002/diff/24001/include/core/SkDocument.h...
include/core/SkDocument.h:68: SkCanvas* beginPage(const SkSize& trimSize,
I need a better (easier) picture of what these sizes/rects mean. trimSize may be
correct, but its a funny word given we're creating a page. Why isn't it
pageSize?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:40: class Reporter;
Why is this declared here?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:44: class PdfRenderer;
Why is this declared here?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:51: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
Is this different form the EncodeProc type added to SkDocument.h?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:77: // TODO(vandebo): The sizes should be SkSize and
not SkISize.
lets remove the TODO if this is deprecated

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:334: class SkPDFDeviceFlattener : public SkPDFDevice {
Does this class need to be in this header, or can it be private to the impl?

https://codereview.chromium.org/24811002/diff/24001/src/doc/SkDocument.cpp
File src/doc/SkDocument.cpp (left):

https://codereview.chromium.org/24811002/diff/24001/src/doc/SkDocument.cpp#ol...
src/doc/SkDocument.cpp:33: if (!inner.intersect(outer)) {
Why do we not check for valid mediaBox anymore?

https://codereview.chromium.org/24811002/diff/24001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/24811002/diff/24001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:2101: SkMatrix translate(const SkRect& content) {
I see the need for a function, but can we give it a more meaningful name? Or at
least a brief comment?

[edisonn, 2013-10-07 19:29:06]

https://codereview.chromium.org/24811002/diff/24001/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/24001/gm/gmmain.cpp#newcode626
gm/gmmain.cpp:626: SkAutoTUnref<SkDocument> pdfDoc(SkDocument::CreatePDF(&pdf,
NULL, encode_to_dct_data));
On 2013/10/07 13:24:54, reed1 wrote:
> Can we move the SUPPORT_PDF check to inside CreatePDF(), so that the caller
just
> needs to do a NULL check on the returned document?

not really. Sk_SUPPORT_PDF is defined for dependent projects, not for pdf lib
itself.

https://codereview.chromium.org/24811002/diff/24001/gm/gmmain.cpp#newcode629
gm/gmmain.cpp:629: canvas->setMatrix(initialTransform);
On 2013/10/07 13:24:54, reed1 wrote:
> can this be concat instead of setMatrix? The latter is very unfriendly, since
it
> completely ignores the state of the canvas as returned by beginPage.

Done.

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

https://codereview.chromium.org/24811002/diff/24001/include/core/SkDocument.h...
include/core/SkDocument.h:56: SkPicture::EncodeBitmap encoder = NULL);
IMHO, since this is a function pointer, the caller can dynamically change the
behavior of the function, without having to explicitly set a new encoder. But I
have no preference, either way.

On 2013/10/07 13:24:54, reed1 wrote:
> I wonder, if we end up with more options in the future, if we want to allow
> these to be set after construction. e.g. setBitmapEncoder(...), or is it vital
> to keep all of them in the constructor (i.e. known a factory time)?
> 
> Not requesting a change to this CL, just wondering.

https://codereview.chromium.org/24811002/diff/24001/include/core/SkDocument.h...
include/core/SkDocument.h:68: SkCanvas* beginPage(const SkSize& trimSize,
A good reason would be to maintain consistency with the XPS code. Ben, care to
comment?


On 2013/10/07 13:24:54, reed1 wrote:
> I need a better (easier) picture of what these sizes/rects mean. trimSize may
be
> correct, but its a funny word given we're creating a page. Why isn't it
> pageSize?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:40: class Reporter;
On 2013/10/07 13:24:54, reed1 wrote:
> Why is this declared here?

test code, removed

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:44: class PdfRenderer;
On 2013/10/07 13:24:54, reed1 wrote:
> Why is this declared here?

test code, removed

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:51: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
On 2013/10/07 13:24:54, reed1 wrote:
> Is this different form the EncodeProc type added to SkDocument.h?

Unused, removed

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:77: // TODO(vandebo): The sizes should be SkSize and
not SkISize.
On 2013/10/07 13:24:54, reed1 wrote:
> lets remove the TODO if this is deprecated

Done.

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:334: class SkPDFDeviceFlattener : public SkPDFDevice {
On 2013/10/07 13:24:54, reed1 wrote:
> Does this class need to be in this header, or can it be private to the impl?

I could put all this logic inside PDF, but long term, I think PDF and XPS should
inherit the same base class. For now The flattener is based on PDF, but then I
would move the flattener to be inherited by pdf instead, and then maybe add XPS.

In this way I keep most of the logic contained in a single layer.

I would not move this to it's own file, for now.

This class has a private constructor, and only SkDocument::CreatePDF can create
one.

what do you mean by "private to the impl"?

https://codereview.chromium.org/24811002/diff/24001/src/doc/SkDocument.cpp
File src/doc/SkDocument.cpp (left):

https://codereview.chromium.org/24811002/diff/24001/src/doc/SkDocument.cpp#ol...
src/doc/SkDocument.cpp:33: if (!inner.intersect(outer)) {
On 2013/10/07 13:24:54, reed1 wrote:
> Why do we not check for valid mediaBox anymore?

Done.

https://codereview.chromium.org/24811002/diff/24001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

https://codereview.chromium.org/24811002/diff/24001/src/pdf/SkPDFDevice.cpp#n...
src/pdf/SkPDFDevice.cpp:2101: SkMatrix translate(const SkRect& content) {
On 2013/10/07 13:24:54, reed1 wrote:
> I see the need for a function, but can we give it a more meaningful name? Or
at
> least a brief comment?

Done.

[reed1, 2013-10-07 19:39:42]

trimSIze is the size of the content? why not call it content-size?

mediaBox is the size of the page? so it is larger than the trimSize? This seems
a bit surprising as a specification pattern. Why not

SkSize: pageSize
SkRect: contentAreaWithinPage

https://codereview.chromium.org/24811002/diff/39001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/39001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:323: class SkPDFDeviceFlattener : public SkPDFDevice {
If there is no need to make this public, I think it should be private. Making a
sharable private header is easy (and common in skia).

[vandebo (ex-Chrome), 2013-10-07 22:34:10]

Comments that never got sent.

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:875: SkCanvasSimplifier canvas(&pattern);
On 2013/10/04 19:40:37, edisonn wrote:
> On 2013/09/27 15:50:17, vandebo wrote:
> > This doesn't help you here.  I think this method will require explicit
support
> > for perspective.  Calculation of the bbox needs to take the perspective into
> > consideration.
> > 
> > If you look below, canvas is used only with affine transforms - there's no
way
> > that the canvas will see perspective transforms.  Above on line 844/5 is
where
> a
> > perspective transform might come into the picture, but right now it would
just
> > be passed through to the FormXObject matrix property and won't be handled
> > properly.
> 
> Done.

Done? how so?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:51: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
remove?

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:334: class SkPDFDeviceFlattener : public SkPDFDevice {
Flattener is a very generic name.  Is this meant to be SkPDFDevice with
perspective support?  SkPDFPerspectiveDevice ?

Why do it as a separate device?

[edisonn, 2013-10-08 17:55:30]

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/24811002/diff/1/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:875: SkCanvasSimplifier canvas(&pattern);
On 2013/10/07 22:34:10, vandebo wrote:
> On 2013/10/04 19:40:37, edisonn wrote:
> > On 2013/09/27 15:50:17, vandebo wrote:
> > > This doesn't help you here.  I think this method will require explicit
> support
> > > for perspective.  Calculation of the bbox needs to take the perspective
into
> > > consideration.
> > > 
> > > If you look below, canvas is used only with affine transforms - there's no
> way
> > > that the canvas will see perspective transforms.  Above on line 844/5 is
> where
> > a
> > > perspective transform might come into the picture, but right now it would
> just
> > > be passed through to the FormXObject matrix property and won't be handled
> > > properly.
> > 
> > Done.
> 
> Done? how so?
Sorry, is being done in another CL, as it is not related to this work item. I
will leave the comment though, by the time I will check it in.

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:51: typedef bool (*EncodeToDCTStream)(SkWStream*
stream, const SkBitmap& bitmap, const SkIRect& rect);
On 2013/10/07 22:34:10, vandebo wrote:
> remove?

removed

https://codereview.chromium.org/24811002/diff/24001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:334: class SkPDFDeviceFlattener : public SkPDFDevice {
On 2013/10/07 22:34:10, vandebo wrote:
> Flattener is a very generic name.  Is this meant to be SkPDFDevice with
> perspective support?  SkPDFPerspectiveDevice ?
This class will flatten other properties unsuported, not only perspective.
> 
> Why do it as a separate device?
The flatten layer should be be separated, so long term XPS and PDF device can
reuse them easily. Probably, long term XPS and PDF should inherit the same
device type. If we mix the flattening code in PDFDevice, then we will end up
with duplicated code in XPS and PDF, and it will be harder to refactor them.

https://codereview.chromium.org/24811002/diff/39001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/39001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:323: class SkPDFDeviceFlattener : public SkPDFDevice {
On 2013/10/07 19:39:42, reed1 wrote:
> If there is no need to make this public, I think it should be private. Making
a
> sharable private header is easy (and common in skia).

Done.

[vandebo (ex-Chrome), 2013-10-08 23:37:05]

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp#newcode628
gm/gmmain.cpp:628: canvas->setMatrix(initialTransform);
On 2013/10/04 19:40:37, edisonn wrote:
> On 2013/09/27 15:50:17, vandebo wrote:
> > Until the old constructor is removed, I think it's best to use the old code
> > path. There's explicit gm support for initialtranforms because there were
bugs
> > that only showed up with a non-default initialtranform and there are now
tests
> > verifying that doesn't regress. Yes, you are going to remove it, but until
> then,
> > It's probably best to keep the test coverage.
> 
> Now SkPDFDeviceFlatenable will call the old constructor

It doesn't.

https://codereview.chromium.org/24811002/diff/46001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/24811002/diff/46001/gm/gm_error.h#newcode50
gm/gm_error.h:50: return "RenderPdfFailed";
This isn't RenderPDF, this is GeneratePDF

https://codereview.chromium.org/24811002/diff/46001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/46001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:25: class CanvasTestStep;
Extra

https://codereview.chromium.org/24811002/diff/46001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:319: friend class SkDocument_PDF;
nit: omit the friends until they are needed

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

https://codereview.chromium.org/24811002/diff/46001/src/doc/SkDocument.cpp#ne...
src/doc/SkDocument.cpp:25: if (width <= 0 || height <= 0) {
nit: extra spaces

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.cpp (right):

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:26: void SkPDFDeviceFlattener::drawPaint(const
SkDraw& d, const SkPaint& paint) {
Omit virtual methods that just call the parent.

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:33: if (mustFlatten(d)) {
consider using this pattern instead:

if(!mustFlatten(d)) {
    INHERITED::foo(args);
    return;
}

This lets you do the rest of the work without having to worry about that case
and gives you another level of indent to work with.

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.h (right):

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.h:13: class SkPDFDeviceFlattener : public
SkPDFDevice {
There's probably a better name for this: SkPerspectiveFlattener?

[edisonn, 2013-10-09 13:25:55]

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp#newcode628
gm/gmmain.cpp:628: canvas->setMatrix(initialTransform);
On 2013/10/08 23:37:06, vandebo wrote:
> On 2013/10/04 19:40:37, edisonn wrote:
> > On 2013/09/27 15:50:17, vandebo wrote:
> > > Until the old constructor is removed, I think it's best to use the old
code
> > > path. There's explicit gm support for initialtranforms because there were
> bugs
> > > that only showed up with a non-default initialtranform and there are now
> tests
> > > verifying that doesn't regress. Yes, you are going to remove it, but until
> > then,
> > > It's probably best to keep the test coverage.
> > 
> > Now SkPDFDeviceFlatenable will call the old constructor
> 
> It doesn't.

Correct, I do not pass now the initialTransform.

But, if there were bugs, we should have had test cases for them, instead of
relying on gms that can actually change without pdf's control. Do you have at
least a list of gms that hit those interesting bugs?

Another option, is to pseudo-randomly use one or the other, so we cover both. I
am not sure what the best course is in this case, as both should be tested

https://codereview.chromium.org/24811002/diff/46001/gm/gm_error.h
File gm/gm_error.h (right):

https://codereview.chromium.org/24811002/diff/46001/gm/gm_error.h#newcode50
gm/gm_error.h:50: return "RenderPdfFailed";
On 2013/10/08 23:37:06, vandebo wrote:
> This isn't RenderPDF, this is GeneratePDF

Done.

https://codereview.chromium.org/24811002/diff/46001/include/pdf/SkPDFDevice.h
File include/pdf/SkPDFDevice.h (right):

https://codereview.chromium.org/24811002/diff/46001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:25: class CanvasTestStep;
On 2013/10/08 23:37:06, vandebo wrote:
> Extra

removed

https://codereview.chromium.org/24811002/diff/46001/include/pdf/SkPDFDevice.h...
include/pdf/SkPDFDevice.h:319: friend class SkDocument_PDF;
On 2013/10/08 23:37:06, vandebo wrote:
> nit: omit the friends until they are needed

Done.

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.cpp (right):

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:33: if (mustFlatten(d)) {
On 2013/10/08 23:37:06, vandebo wrote:
> consider using this pattern instead:
> 
> if(!mustFlatten(d)) {
>     INHERITED::foo(args);
>     return;
> }
> 
> This lets you do the rest of the work without having to worry about that case
> and gives you another level of indent to work with.

Done.

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.h (right):

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.h:13: class SkPDFDeviceFlattener : public
SkPDFDevice {
On 2013/10/08 23:37:06, vandebo wrote:
> There's probably a better name for this: SkPerspectiveFlattener?

As said before, this class will do more than flatten perspective. Perspective is
the first one implemented, bit there other features that require flattening.

[reed1, 2013-10-09 13:33:47]

This CL is 12 days old. Please focus on core attributes so we know how to reach
closure one way or the other. If there are 2nd-order concerns, perhaps they can
be deferred to a 2nd CL?

[vandebo (ex-Chrome), 2013-10-09 16:52:31]

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp#newcode628
gm/gmmain.cpp:628: canvas->setMatrix(initialTransform);
On 2013/10/09 13:25:55, edisonn wrote:
> On 2013/10/08 23:37:06, vandebo wrote:
> > On 2013/10/04 19:40:37, edisonn wrote:
> > > On 2013/09/27 15:50:17, vandebo wrote:
> > > > Until the old constructor is removed, I think it's best to use the old
> code
> > > > path. There's explicit gm support for initialtranforms because there
were
> > bugs
> > > > that only showed up with a non-default initialtranform and there are now
> > tests
> > > > verifying that doesn't regress. Yes, you are going to remove it, but
until
> > > then,
> > > > It's probably best to keep the test coverage.
> > > 
> > > Now SkPDFDeviceFlatenable will call the old constructor
> > 
> > It doesn't.
> 
> Correct, I do not pass now the initialTransform.
> 
> But, if there were bugs, we should have had test cases for them, instead of
> relying on gms that can actually change without pdf's control. Do you have at
> least a list of gms that hit those interesting bugs?

I added a gm (and the ability for the gm to specify the initial transform) when
fixing the bug.  The gm is the test.  Grepping for usage, it's 
shaderbounds_linear in shaderbounds.cpp.

> Another option, is to pseudo-randomly use one or the other, so we cover both.
I
> am not sure what the best course is in this case, as both should be tested

A fast, but dirty way to make progress is to use the old code path if there is
an initial transform specified by the gm - that's only one gm right now.  All of
the initial transform code can eventually be removed when Blink is made to work
without it.

If that doesn't work for you, call me and we'll hash it out.

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.h (right):

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.h:13: class SkPDFDeviceFlattener : public
SkPDFDevice {
On 2013/10/09 13:25:55, edisonn wrote:
> On 2013/10/08 23:37:06, vandebo wrote:
> > There's probably a better name for this: SkPerspectiveFlattener?
> 
> As said before, this class will do more than flatten perspective. Perspective
is
> the first one implemented, bit there other features that require flattening.

I must have missed that comment.  This is a very generic name and it's not clear
what it does. Please add a comment describing what it currently does and a TODO
to consider a better name once we know everything it will do.

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.cpp (right):

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:33: 
I think shaders can affect drawPoints too... lines, polygons...

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:57: if (paintFlatten.getShader()) {
Consider pulling this block of code into a function, it's used twice and may be
used more in the future.

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:77: path.transform(*prePathMatrix);
The comment in SkDevice.h above drawPath implies that something more may need to
be done here if paint has geometry effects, but I don't understand it.

Mike?

[reed1, 2013-10-09 17:03:54]

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.cpp (right):

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:75: path.addPath(origpath);
if your intention is to always make a copy of the path, just calling the
copy-constructor is probably clearer/slightly-faster.

SkPath path(origpath);

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:77: path.transform(*prePathMatrix);
On 2013/10/09 16:52:32, vandebo wrote:
> The comment in SkDevice.h above drawPath implies that something more may need
to
> be done here if paint has geometry effects, but I don't understand it.
> 
> Mike?

Before you can call transform (on line 79) you have to have applied any
stroking, etc. This can easily be done by just calling paint.getFillPath(). The
returned path will have all geometric effects applied, and can be now be
transformed by the CTM.

If that is done (calling getFillPath), then the paint needs to have those
effects removed before calling back into the INHERITED::drawPath (so they don't
get applied twice).

[edisonn, 2013-10-10 18:35:14]

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp
File gm/gmmain.cpp (right):

https://codereview.chromium.org/24811002/diff/1/gm/gmmain.cpp#newcode628
gm/gmmain.cpp:628: canvas->setMatrix(initialTransform);
On 2013/10/09 16:52:32, vandebo wrote:
> On 2013/10/09 13:25:55, edisonn wrote:
> > On 2013/10/08 23:37:06, vandebo wrote:
> > > On 2013/10/04 19:40:37, edisonn wrote:
> > > > On 2013/09/27 15:50:17, vandebo wrote:
> > > > > Until the old constructor is removed, I think it's best to use the old
> > code
> > > > > path. There's explicit gm support for initialtranforms because there
> were
> > > bugs
> > > > > that only showed up with a non-default initialtranform and there are
now
> > > tests
> > > > > verifying that doesn't regress. Yes, you are going to remove it, but
> until
> > > > then,
> > > > > It's probably best to keep the test coverage.
> > > > 
> > > > Now SkPDFDeviceFlatenable will call the old constructor
> > > 
> > > It doesn't.
> > 
> > Correct, I do not pass now the initialTransform.
> > 
> > But, if there were bugs, we should have had test cases for them, instead of
> > relying on gms that can actually change without pdf's control. Do you have
at
> > least a list of gms that hit those interesting bugs?
> 
> I added a gm (and the ability for the gm to specify the initial transform)
when
> fixing the bug.  The gm is the test.  Grepping for usage, it's 
> shaderbounds_linear in shaderbounds.cpp.
> 
> > Another option, is to pseudo-randomly use one or the other, so we cover
both.
> I
> > am not sure what the best course is in this case, as both should be tested
> 
> A fast, but dirty way to make progress is to use the old code path if there is
> an initial transform specified by the gm - that's only one gm right now.  All
of
> the initial transform code can eventually be removed when Blink is made to
work
> without it.
> 
> If that doesn't work for you, call me and we'll hash it out.
added a new flag to specify if SkDocument will be used. It will be enabled in
android gms for example, while in the rest of gms we will use old way

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.h (right):

https://codereview.chromium.org/24811002/diff/46001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.h:13: class SkPDFDeviceFlattener : public
SkPDFDevice {
On 2013/10/09 16:52:32, vandebo wrote:
> On 2013/10/09 13:25:55, edisonn wrote:
> > On 2013/10/08 23:37:06, vandebo wrote:
> > > There's probably a better name for this: SkPerspectiveFlattener?
> > 
> > As said before, this class will do more than flatten perspective.
Perspective
> is
> > the first one implemented, bit there other features that require flattening.
> 
> I must have missed that comment.  This is a very generic name and it's not
clear
> what it does. Please add a comment describing what it currently does and a
TODO
> to consider a better name once we know everything it will do.

Done.

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
File src/pdf/SkPDFDeviceFlattener.cpp (right):

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:33: 
On 2013/10/09 16:52:32, vandebo wrote:
> I think shaders can affect drawPoints too... lines, polygons...

Done.

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:57: if (paintFlatten.getShader()) {
On 2013/10/09 16:52:32, vandebo wrote:
> Consider pulling this block of code into a function, it's used twice and may
be
> used more in the future.

Done.

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:75: path.addPath(origpath);
On 2013/10/09 17:03:54, reed1 wrote:
> if your intention is to always make a copy of the path, just calling the
> copy-constructor is probably clearer/slightly-faster.
> 
> SkPath path(origpath);

Done - using pathIsMutable

https://codereview.chromium.org/24811002/diff/57001/src/pdf/SkPDFDeviceFlatte...
src/pdf/SkPDFDeviceFlattener.cpp:77: path.transform(*prePathMatrix);
On 2013/10/09 17:03:54, reed1 wrote:
> On 2013/10/09 16:52:32, vandebo wrote:
> > The comment in SkDevice.h above drawPath implies that something more may
need
> to
> > be done here if paint has geometry effects, but I don't understand it.
> > 
> > Mike?
> 
> Before you can call transform (on line 79) you have to have applied any
> stroking, etc. This can easily be done by just calling paint.getFillPath().
The
> returned path will have all geometric effects applied, and can be now be
> transformed by the CTM.
> 
> If that is done (calling getFillPath), then the paint needs to have those
> effects removed before calling back into the INHERITED::drawPath (so they
don't
> get applied twice).

Done.

[vandebo (ex-Chrome), 2013-10-10 23:02:21]

LGTM

[edisonn, 2013-10-11 14:24:18]

Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

Mike/Brian, can you approve the CL too?
The public API change is in SkDraw - it makes public 2 drawFoo_AsPath, I think
we should be fine, since drawFoo directly calls drawFoo_AsPath.

[reed1, 2013-10-14 13:16:01]

lgtm

[commit-bot: I haz the power, 2013-10-14 13:31:54]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/edisonn@google.com/24811002/81001

[commit-bot: I haz the power, 2013-10-14 13:42:18]

Change committed as 11751