[PDF] Add clip support and some optimizations for "complex" xfer modes.

[PDF] Add clip support and some optimizations for "complex" xfer modes.

For Clear, Src, Dst/Src-In/Out, we have to consider the current clip, and
potentially draw Dst, clipped to the inverse of the current clip before
doing the operation of interest.

For clear or src, if we haven't drawn anything, or the clip is empty, there's
nothing to be done.
For Src/Dst-In/Out, if either is empty, the result is empty.

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

[Steve VanDeBogart, 2011-05-23 06:19:49]

http://codereview.appspot.com/4538082/diff/1/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (left):

http://codereview.appspot.com/4538082/diff/1/src/pdf/SkPDFDevice.cpp#oldcode1183
src/pdf/SkPDFDevice.cpp:1183: SkASSERT(fDstFormXObject.get() == NULL);
This is removed because it is false on recursive calls now.  The other checks
should still catch leaked fDstFormXobject's though.  Added back in a follow up
CL.

[Chris Guillory, 2011-05-24 00:47:06]

I'd like to look over this one again.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1037: if (clipRegion.isEmpty() || !fContentEntries.get()
||
Nit: Perhaps fold "!fContentEntries.get() ||
!fContentEntries->fContent.getOffset()" into a member function like
isContentEmpty()

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1045: clipAndDrawFormXObject(curContent.get(),
clipStack, clipRegion, true);
Nit: After looking at this comment I think clipAndDrawFormXObject might be
better named drawFormXObjectWithClip.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1048: void
SkPDFDevice::clipAndDrawFormXObject(SkPDFFormXObject* form,
Nit: form -> object

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1072: // Draw the form with the clip as a mask.
Nit: form -> object? xobject?

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1132: if (fContentEntries.get() &&
fContentEntries->fContent.getOffset()) {
Nit: Use isContentEmpty() member function.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1205: SkClipStack clipStack =
fCurrentContentEntry->fState.fClipStack;
Nit: In another change you added a comment here. I don't see it here but just
want to make sure it gets in.

// We have to make a copy of these here because changing the current
// content into a form xobject will destroy them.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1215: if (!srcFormXObject.get()) {
Nit: Can the contents of this if be the same as the one above? Currently "if
(fCurrentContentEntry->fContent.getOffset())". Also can we have a comment here.

[Steve VanDeBogart, 2011-05-24 06:07:51]

On 2011/05/24 00:47:06, Chris Guillory wrote:
> I'd like to look over this one again.

Sure, but the other changes are pipelined on top of this one.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1037: if (clipRegion.isEmpty() || !fContentEntries.get()
||
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: Perhaps fold "!fContentEntries.get() ||
> !fContentEntries->fContent.getOffset()" into a member function like
> isContentEmpty()

Done.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1045: clipAndDrawFormXObject(curContent.get(),
clipStack, clipRegion, true);
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: After looking at this comment I think clipAndDrawFormXObject might be
> better named drawFormXObjectWithClip.

Done.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1048: void
SkPDFDevice::clipAndDrawFormXObject(SkPDFFormXObject* form,
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: form -> object

Done.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1072: // Draw the form with the clip as a mask.
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: form -> object? xobject?

Done.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1132: if (fContentEntries.get() &&
fContentEntries->fContent.getOffset()) {
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: Use isContentEmpty() member function. 

Done.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1205: SkClipStack clipStack =
fCurrentContentEntry->fState.fClipStack;
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: In another change you added a comment here. I don't see it here but just
> want to make sure it gets in.
> 
> // We have to make a copy of these here because changing the current
> // content into a form xobject will destroy them.

Moved up to this CL.

http://codereview.appspot.com/4538082/diff/2001/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1215: if (!srcFormXObject.get()) {
On 2011/05/24 00:47:06, Chris Guillory wrote:
> Nit: Can the contents of this if be the same as the one above?

If you mean, can the conditional be the same, then no.  The call to
drawFormXObjectWithClip changes what the first conditional tested.

> Currently "if (fCurrentContentEntry->fContent.getOffset())".

I changed this to !isContentEmpty() to make it more descriptive.

> Also can we have a comment here.

Done.

[Chris Guillory, 2011-05-24 09:27:51]

LGTM.

http://codereview.appspot.com/4538082/diff/2002/src/pdf/SkPDFDevice.cpp
File src/pdf/SkPDFDevice.cpp (right):

http://codereview.appspot.com/4538082/diff/2002/src/pdf/SkPDFDevice.cpp#newco...
src/pdf/SkPDFDevice.cpp:1217: // Now that we've redrawn dst except what's in the
clip, if there's
Nits/Optional: Now that we've -> We've
except what's in -> with
source -> src