[PDF] Fix bugs preventing gm from running with PDF support.

[PDF] Fix bugs preventing gm from running with PDF support.

Fix copy-paste ref counting bug.
Change NOT_IMPLEMENTED macro to only assert in debug mode.

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

[reed1, 2011-01-31 22:24:50]

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

http://codereview.appspot.com/4080048/diff/1/src/pdf/SkPDFDevice.cpp#newcode39
src/pdf/SkPDFDevice.cpp:39: SkASSERT(!SK_DEBUG || !assert);                     
  \
SK_DEBUG is not defined to be 1 or 0, but is just defined or not. Is !SK_DEBUG
well defined if SK_DEBUG is define or not, but with no value?

An alternative pattern we've used is

SkDEBUGCODE(SkASSERT(!assert);)

-- note there is no trailing ;

http://codereview.appspot.com/4080048/diff/1/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

http://codereview.appspot.com/4080048/diff/1/src/pdf/SkPDFImage.cpp#newcode232
src/pdf/SkPDFImage.cpp:232: countValue->unref();  // SkRefPtr and new both took
a reference.
1. We should document that SkPDFArray.append() increments the reference count of
its argument.
2. If you changed that signature to also return its argument (as we do in calls
like SkPaint::setShader() etc.) then this pattern simplifies to

result->append(new SkPDFInt(...))->unref();

with no need for a named intermediary, or SkRefPtr<>. Just a suggestion.

[Steve VanDeBogart, 2011-01-31 22:44:24]

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

http://codereview.appspot.com/4080048/diff/1/src/pdf/SkPDFDevice.cpp#newcode39
src/pdf/SkPDFDevice.cpp:39: SkASSERT(!SK_DEBUG || !assert);                     
  \
On 2011/01/31 22:24:52, reed1 wrote:
> SK_DEBUG is not defined to be 1 or 0, but is just defined or not. Is !SK_DEBUG
> well defined if SK_DEBUG is define or not, but with no value?
> 
> An alternative pattern we've used is
> 
> SkDEBUGCODE(SkASSERT(!assert);)
> 
> -- note there is no trailing ;

It worked when I tried it, but your suggesting is better.  Fixed.

http://codereview.appspot.com/4080048/diff/1/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

http://codereview.appspot.com/4080048/diff/1/src/pdf/SkPDFImage.cpp#newcode232
src/pdf/SkPDFImage.cpp:232: countValue->unref();  // SkRefPtr and new both took
a reference.
On 2011/01/31 22:24:52, reed1 wrote:
> 1. We should document that SkPDFArray.append() increments the reference count
of
> its argument.
> 2. If you changed that signature to also return its argument (as we do in
calls
> like SkPaint::setShader() etc.) then this pattern simplifies to
> 
> result->append(new SkPDFInt(...))->unref();
> 
> with no need for a named intermediary, or SkRefPtr<>. Just a suggestion.

Most of the objects that inherit from SkPDFObject increment the ref count of
their arguments and could benefit from the return your argument pattern.  When I
wrote these classes, I didn't know how much they were going to get used.  If
it's ok with you, I'll do this conversion in a subsequent CL.

[reed1, 2011-02-01 00:51:02]

LGTM