Initial PDF backend commit: directories, build rules, primitives

Initial PDF backend commit: directories, build rules, primitives

This change establishes and tests the building blocks of the PDF file format.
For now, PDF code is not compiled by default.

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

[James Hawkins, 2010-08-25 06:31:58]

Mostly nits, API looks good so far.

http://codereview.appspot.com/1950044/diff/1/4
File include/pdf/SkPDFCatalog.h (right):

http://codereview.appspot.com/1950044/diff/1/4#newcode28
include/pdf/SkPDFCatalog.h:28: PDF stream, index all the objects in the file by
offset.
s/index/indexes/

http://codereview.appspot.com/1950044/diff/1/4#newcode45
include/pdf/SkPDFCatalog.h:45: void addObject(SkPDFObject* obj, bool firstPage);
Consider renaming the variable to onFirstPage. This one made me think for a bit.

http://codereview.appspot.com/1950044/diff/1/4#newcode48
include/pdf/SkPDFCatalog.h:48: *  @param obj        The object of interest
Period at end of sentence.

http://codereview.appspot.com/1950044/diff/1/5
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/1950044/diff/1/5#newcode27
include/pdf/SkPDFStream.h:27: An stream object in a PDF.
s/An/A/

http://codereview.appspot.com/1950044/diff/1/5#newcode53
include/pdf/SkPDFStream.h:53: SkMemoryStream* fData;
Does skia/android have scoped_ptr or a variation?

http://codereview.appspot.com/1950044/diff/1/6
File include/pdf/SkPDFTypes.h (right):

http://codereview.appspot.com/1950044/diff/1/6#newcode45
include/pdf/SkPDFTypes.h:45: *  @param indirect If true, output an object
identifier with the object
Period at end of sentence.

http://codereview.appspot.com/1950044/diff/1/6#newcode55
include/pdf/SkPDFTypes.h:55: *  @param indirect If true, output an object
identifier with the object
Period at end of sentence.

http://codereview.appspot.com/1950044/diff/1/6#newcode114
include/pdf/SkPDFTypes.h:114: An real number object in a PDF.
s/An/A/ here and a few lines down.

http://codereview.appspot.com/1950044/diff/1/6#newcode134
include/pdf/SkPDFTypes.h:134: An string object in a PDF.
s/An/A/

http://codereview.appspot.com/1950044/diff/1/6#newcode160
include/pdf/SkPDFTypes.h:160: An name object in a PDF.
s/An/A/

http://codereview.appspot.com/1950044/diff/1/6#newcode232
include/pdf/SkPDFTypes.h:232: An dictionary object in a PDF.
s/An/A/

http://codereview.appspot.com/1950044/diff/1/9
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/1950044/diff/1/9#newcode89
src/pdf/SkPDFTypes.cpp:89: // 7-bit clean is a heuristic to decide what format
string to use.
Might want to comment on the different format strings.

http://codereview.appspot.com/1950044/diff/1/9#newcode90
src/pdf/SkPDFTypes.cpp:90: bool SevenBitClean = true;
I'm not entirely familiar with skia/android style, but should this be
sevenBitClean?

http://codereview.appspot.com/1950044/diff/1/11
File tests/PDFPrimitivesTest.cpp (right):

http://codereview.appspot.com/1950044/diff/1/11#newcode1
tests/PDFPrimitivesTest.cpp:1: #include <string>
No license for test files?

[Steve VanDeBogart, 2010-08-25 20:17:33]

http://codereview.appspot.com/1950044/diff/1/4
File include/pdf/SkPDFCatalog.h (right):

http://codereview.appspot.com/1950044/diff/1/4#newcode28
include/pdf/SkPDFCatalog.h:28: PDF stream, index all the objects in the file by
offset.
On 2010/08/25 06:31:59, James Hawkins wrote:
> s/index/indexes/

Done.

http://codereview.appspot.com/1950044/diff/1/4#newcode45
include/pdf/SkPDFCatalog.h:45: void addObject(SkPDFObject* obj, bool firstPage);
On 2010/08/25 06:31:59, James Hawkins wrote:
> Consider renaming the variable to onFirstPage. This one made me think for a
bit.

Done.

http://codereview.appspot.com/1950044/diff/1/4#newcode48
include/pdf/SkPDFCatalog.h:48: *  @param obj        The object of interest
On 2010/08/25 06:31:59, James Hawkins wrote:
> Period at end of sentence.

Done.

http://codereview.appspot.com/1950044/diff/1/5
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/1950044/diff/1/5#newcode27
include/pdf/SkPDFStream.h:27: An stream object in a PDF.
On 2010/08/25 06:31:59, James Hawkins wrote:
> s/An/A/

Done.

http://codereview.appspot.com/1950044/diff/1/5#newcode53
include/pdf/SkPDFStream.h:53: SkMemoryStream* fData;
On 2010/08/25 06:31:59, James Hawkins wrote:
> Does skia/android have scoped_ptr or a variation?

Good call.  Took a couple tries to find it, but Done.

http://codereview.appspot.com/1950044/diff/1/6
File include/pdf/SkPDFTypes.h (right):

http://codereview.appspot.com/1950044/diff/1/6#newcode45
include/pdf/SkPDFTypes.h:45: *  @param indirect If true, output an object
identifier with the object
On 2010/08/25 06:31:59, James Hawkins wrote:
> Period at end of sentence.

Done.

http://codereview.appspot.com/1950044/diff/1/6#newcode55
include/pdf/SkPDFTypes.h:55: *  @param indirect If true, output an object
identifier with the object
On 2010/08/25 06:31:59, James Hawkins wrote:
> Period at end of sentence.

Done.

http://codereview.appspot.com/1950044/diff/1/6#newcode114
include/pdf/SkPDFTypes.h:114: An real number object in a PDF.
On 2010/08/25 06:31:59, James Hawkins wrote:
> s/An/A/ here and a few lines down.

Done.

http://codereview.appspot.com/1950044/diff/1/6#newcode134
include/pdf/SkPDFTypes.h:134: An string object in a PDF.
On 2010/08/25 06:31:59, James Hawkins wrote:
> s/An/A/

Done.

http://codereview.appspot.com/1950044/diff/1/6#newcode160
include/pdf/SkPDFTypes.h:160: An name object in a PDF.
On 2010/08/25 06:31:59, James Hawkins wrote:
> s/An/A/

Done.

http://codereview.appspot.com/1950044/diff/1/6#newcode232
include/pdf/SkPDFTypes.h:232: An dictionary object in a PDF.
On 2010/08/25 06:31:59, James Hawkins wrote:
> s/An/A/

Done.

http://codereview.appspot.com/1950044/diff/1/9
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/1950044/diff/1/9#newcode89
src/pdf/SkPDFTypes.cpp:89: // 7-bit clean is a heuristic to decide what format
string to use.
On 2010/08/25 06:31:59, James Hawkins wrote:
> Might want to comment on the different format strings.

Done.

http://codereview.appspot.com/1950044/diff/1/9#newcode90
src/pdf/SkPDFTypes.cpp:90: bool SevenBitClean = true;
On 2010/08/25 06:31:59, James Hawkins wrote:
> I'm not entirely familiar with skia/android style, but should this be
> sevenBitClean?

Done.

http://codereview.appspot.com/1950044/diff/1/11
File tests/PDFPrimitivesTest.cpp (right):

http://codereview.appspot.com/1950044/diff/1/11#newcode1
tests/PDFPrimitivesTest.cpp:1: #include <string>
On 2010/08/25 06:31:59, James Hawkins wrote:
> No license for test files?

Hmm, indeed.  None of the other test files have licenses.

[James Hawkins, 2010-08-25 20:27:21]

LGTM, but definitely want agl's opinion as well.

[agl, 2010-08-25 23:06:14]

LGTM, but see below.

http://codereview.appspot.com/1950044/diff/9001/4005
File include/pdf/SkPDFCatalog.h (right):

http://codereview.appspot.com/1950044/diff/9001/4005#newcode52
include/pdf/SkPDFCatalog.h:52: void emitObjectNumber(SkPDFObject* obj,
SkWStream* stream, bool definition);
I think |stream| should be the first argument as it's the destination (as in
|fprintf| or |write|).

Later: I see you sometimes have the stream as the last argument to some
functions. That's ok too.

http://codereview.appspot.com/1950044/diff/9001/4005#newcode69
include/pdf/SkPDFCatalog.h:69: int fObjNum;
unsigned or uint32_t

http://codereview.appspot.com/1950044/diff/9001/4005#newcode73
include/pdf/SkPDFCatalog.h:73: typedef std::map<SkRefPtr<SkPDFObject>, struct
CatalogInfo> CatalogMap;
I'm not a huge fan of having RefPtrs as keys in a map. I think it'll lead to a
lot of reference churn and the operator< on the SkRefPtr causes one to pause.

It seems that you could have a CatalogInfo pointer inside an SkPDFObject. (Maybe
it would have to be a void* for encapsulation.) But, that way, you could have a
simple std::vector of SkPDFObject*. You would add a reference when putting
objects into the vector and decrement the reference in ~SkPDFCatalog().

It's up to you, but I think it's worth considering.

http://codereview.appspot.com/1950044/diff/9001/4010
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/1950044/diff/9001/4010#newcode36
src/pdf/SkPDFTypes.cpp:36: this->getOutputSize(NULL, false) +
strlen("\nendobj\n");
GCC is smart enough to make strlen("foo") a constant: I checked.

[Steve VanDeBogart, 2010-08-26 23:54:24]

Thanks for the review.

http://codereview.appspot.com/1950044/diff/9001/4005
File include/pdf/SkPDFCatalog.h (right):

http://codereview.appspot.com/1950044/diff/9001/4005#newcode52
include/pdf/SkPDFCatalog.h:52: void emitObjectNumber(SkPDFObject* obj,
SkWStream* stream, bool definition);
On 2010/08/25 23:06:14, agl wrote:
> I think |stream| should be the first argument as it's the destination (as in
> |fprintf| or |write|).
> 
> Later: I see you sometimes have the stream as the last argument to some
> functions. That's ok too.

Now that you mention it, I do like the notion of the stream first. Changed
everywhere.

http://codereview.appspot.com/1950044/diff/9001/4005#newcode69
include/pdf/SkPDFCatalog.h:69: int fObjNum;
On 2010/08/25 23:06:14, agl wrote:
> unsigned or uint32_t

Done.

http://codereview.appspot.com/1950044/diff/9001/4005#newcode73
include/pdf/SkPDFCatalog.h:73: typedef std::map<SkRefPtr<SkPDFObject>, struct
CatalogInfo> CatalogMap;
On 2010/08/25 23:06:14, agl wrote:
> I'm not a huge fan of having RefPtrs as keys in a map. I think it'll lead to a
> lot of reference churn and the operator< on the SkRefPtr causes one to pause.
> 
> It seems that you could have a CatalogInfo pointer inside an SkPDFObject.
(Maybe
> it would have to be a void* for encapsulation.) But, that way, you could have
a
> simple std::vector of SkPDFObject*. You would add a reference when putting
> objects into the vector and decrement the reference in ~SkPDFCatalog().
> 
> It's up to you, but I think it's worth considering.

The CatalogInfo is not part of SkPDFObject for a a specific reason (Because of
the linearized PDF format, generating a PDF has to be a two pass process.  When
changing page layout in print previous we'd like to to update the preview for
the user ASAP.  With long PDFs that would be slow, so we will be able to
generate multiple PDF's, one with all the pages, the others with one page per
PDF.  On demand we'll output the one page per PDF files for preview to display,
or when they are done with preview, we'll output the single big PDF.  With the
current structure, a PDF object can be added to to both the long PDF and the per
page PDF at the same time.)

I still could do typedef std::map<SkPDFObject *, struct CatalogInfo> CatalogMap.
 Because of the access pattern I expect, I think the amount of reference churn
to be low: An insert per object, several lookups and no deletes (just
destruction).  So automatic memory reference wins in this case unless a lookup
will induce a ref/unref, but I don't think it will.  Adding the operator< on
SkRefPtr did give me pause, but it's a pass through function, so seems ok.

http://codereview.appspot.com/1950044/diff/9001/4010
File src/pdf/SkPDFTypes.cpp (right):

http://codereview.appspot.com/1950044/diff/9001/4010#newcode36
src/pdf/SkPDFTypes.cpp:36: this->getOutputSize(NULL, false) +
strlen("\nendobj\n");
On 2010/08/25 23:06:14, agl wrote:
> GCC is smart enough to make strlen("foo") a constant: I checked.

Great, I was pretty sure it was.

[agl, 2010-08-27 14:51:08]

LGTM

[reed, 2010-09-02 15:01:35]

Looks great. I'm unsure about the operator< inside SkRefCnt, but over all a very
clean job.

I see you're using some std:: collections that have equivalents in Skia. I
presume you think any clients of this library will have STL (previously Skia had
no STL dependency).

http://codereview.appspot.com/1950044/diff/17001/18002
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/1950044/diff/17001/18002#newcode178
include/core/SkRefCnt.h:178: bool operator<(const SkRefPtr& rp) const { return
fObj < rp.fObj; }
These are just comparing the pointers, not the actual objects? That is not at
all what I would expect at a call site.

Perhaps you are using this just as way to order objects arbitrarily for maps or
sorts, which is fine, but I'm uneasy with a public operator overload that
doesn't necessarily behave as expected.

Can this be moved into a (private) friend function?

http://codereview.appspot.com/1950044/diff/17001/18005
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/1950044/diff/17001/18005#newcode37
include/pdf/SkPDFStream.h:37: *  ownership of the passed stream.
Is this ownership rule necessary? More often than not Skia's convention is to
just increment the ownership of the stream, rather than "transfer".

http://codereview.appspot.com/1950044/diff/17001/18005#newcode39
include/pdf/SkPDFStream.h:39: explicit SkPDFStream(SkMemoryStream* stream);
Is it necessary for stream to be a memorystream?

[reed, 2010-09-02 15:40:21]

I see that AGL also commented on the operator<

Lets definitely try something different, so we don't add this to the public
class.

[Steve VanDeBogart, 2010-09-03 02:18:55]

On 2010/09/02 15:01:35, reed wrote:
> Looks great. I'm unsure about the operator< inside SkRefCnt, but over all a
very
> clean job.
> 
> I see you're using some std:: collections that have equivalents in Skia. I
> presume you think any clients of this library will have STL (previously Skia
had
> no STL dependency).

I didn't even notice that STL wasn't used (I saw uses of std::string as
assumed).  I don't want to introduce that dependency without a good reason. 
Uses of STL containers removed in favor of existing classes.

http://codereview.appspot.com/1950044/diff/17001/18002
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/1950044/diff/17001/18002#newcode178
include/core/SkRefCnt.h:178: bool operator<(const SkRefPtr& rp) const { return
fObj < rp.fObj; }
On 2010/09/02 15:01:35, reed wrote:
> These are just comparing the pointers, not the actual objects? That is not at
> all what I would expect at a call site.
> 
> Perhaps you are using this just as way to order objects arbitrarily for maps
or
> sorts, which is fine, but I'm uneasy with a public operator overload that
> doesn't necessarily behave as expected.
> 
> Can this be moved into a (private) friend function?

Removed.

http://codereview.appspot.com/1950044/diff/17001/18005
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/1950044/diff/17001/18005#newcode37
include/pdf/SkPDFStream.h:37: *  ownership of the passed stream.
On 2010/09/02 15:01:35, reed wrote:
> Is this ownership rule necessary? More often than not Skia's convention is to
> just increment the ownership of the stream, rather than "transfer".

For some reason, when I first looked, I didn't see that SkStream was reference
counted.  Done.

http://codereview.appspot.com/1950044/diff/17001/18005#newcode39
include/pdf/SkPDFStream.h:39: explicit SkPDFStream(SkMemoryStream* stream);
On 2010/09/02 15:01:35, reed wrote:
> Is it necessary for stream to be a memorystream?

Nope, looks like all functionality is available from SkStream, though I expect
most uses to be with SkMemoryStream. Done.

[reed, 2010-09-09 16:10:38]

LGTM