High level pdf classes and pdf specific interface.

High level pdf classes and pdf specific interface.

The guts of the implementation will be in SkPDFDevice and below.  This is a first implementation of everything above that point.

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

[agl, 2010-10-11 15:04:23]

On Fri, Oct 1, 2010 at 9:21 PM,  <vandebo@chromium.org> wrote:
> Description:
> High level pdf classes and pdf specific interface.

Crap. Sorry. You should ping me after one working day on code reviews.
If I take longer then I'm either on holiday or (as in this case) I
missed it. Reviewing now.


AGL

[agl, 2010-10-11 15:30:58]

LGTM

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

http://codereview.appspot.com/2342043/diff/1/include/pdf/SkPDFCatalog.h#newco...
include/pdf/SkPDFCatalog.h:87: // Number of object on the first page.
s/object/objects/

http://codereview.appspot.com/2342043/diff/1/include/pdf/SkPDFCatalog.h#newco...
include/pdf/SkPDFCatalog.h:89: // Next object number to assign. (on page > 1)
move the '.' to the end or capitalise 'On' and put a period before ')'

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp
File src/core/SkString.cpp (right):

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp#newcode91
src/core/SkString.cpp:91: {
at some point this file will need a style cleanup. I think the '{' of an if
should be on the same line but I'm certain that you shouldn't change styles
between 'if' and 'do' :)

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp#newcode109
src/core/SkString.cpp:109: while (p < stop)
memcpy

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp#newcode113
src/core/SkString.cpp:113: return string;
The string isn't NUL terminated. I assume that's deliberate and that you've
already calculated the size.

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

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcode74
src/pdf/SkPDFDocument.cpp:74: for (int i = 1; i < fPages.count(); i++) {
Starting at 1 is odd but I understand that there may be a dummy entry in
fPages[0] because there's no page 0. Just checking.

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcode84
src/pdf/SkPDFDocument.cpp:84: fileOffset = fCatalog.setFileOffset(fPages[0],
fileOffset);
Something is clearly off here. You may wish to discard the return values of the
first two calls, in which you shouldn't assign them at all but it looks like you
want +=

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcode87
src/pdf/SkPDFDocument.cpp:87: fileOffset =
fCatalog.setFileOffset(fPageResources[i], fileOffset);
likewise here, +=?

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcode94
src/pdf/SkPDFDocument.cpp:94: fileOffset = fCatalog.setFileOffset(fPageTree[i],
fileOffset);
ditto?

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcod...
src/pdf/SkPDFDocument.cpp:102: fileOffset =
fCatalog.setFileOffset(fPageResources[i], fileOffset);
ditto?

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

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFPage.cpp#newcode38
src/pdf/SkPDFPage.cpp:38: contentStream->unref();  // SkRefPtr and new both took
a reference.
Yea, it's pretty clear that the design of Skia's reference counting was a
mistake :(

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFPage.cpp#newcode76
src/pdf/SkPDFPage.cpp:76: SkTDArray<SkPDFDict*> curNodes;
One wonders if an array of SkRefPtrs wouldn't be better, although there may be
something that I'm missing.

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFPage.cpp#newcode91
src/pdf/SkPDFPage.cpp:91: i++;
aren't these two lines a complicated way of saying 'break'?

[Steve VanDeBogart, 2010-10-12 00:10:29]

Thanks for the review.

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

http://codereview.appspot.com/2342043/diff/1/include/pdf/SkPDFCatalog.h#newco...
include/pdf/SkPDFCatalog.h:87: // Number of object on the first page.
On 2010/10/11 15:30:58, agl wrote:
> s/object/objects/

Done.

http://codereview.appspot.com/2342043/diff/1/include/pdf/SkPDFCatalog.h#newco...
include/pdf/SkPDFCatalog.h:89: // Next object number to assign. (on page > 1)
On 2010/10/11 15:30:58, agl wrote:
> move the '.' to the end or capitalise 'On' and put a period before ')'

Done.

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp
File src/core/SkString.cpp (right):

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp#newcode91
src/core/SkString.cpp:91: {
On 2010/10/11 15:30:58, agl wrote:
> at some point this file will need a style cleanup. I think the '{' of an if
> should be on the same line but I'm certain that you shouldn't change styles
> between 'if' and 'do' :)

Yea, I was trying to match style, but it seems to already be inconsistent.

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp#newcode109
src/core/SkString.cpp:109: while (p < stop)
On 2010/10/11 15:30:58, agl wrote:
> memcpy

Done.

http://codereview.appspot.com/2342043/diff/1/src/core/SkString.cpp#newcode113
src/core/SkString.cpp:113: return string;
On 2010/10/11 15:30:58, agl wrote:
> The string isn't NUL terminated. I assume that's deliberate and that you've
> already calculated the size.

Yup

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

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcode74
src/pdf/SkPDFDocument.cpp:74: for (int i = 1; i < fPages.count(); i++) {
On 2010/10/11 15:30:58, agl wrote:
> Starting at 1 is odd but I understand that there may be a dummy entry in
> fPages[0] because there's no page 0. Just checking.

fPages[0] is actually the first page, not a dummy entry.  The for body was
unroll for i = 0 because some of the arguments were different.  I pulled it back
into the loop and made the argument differences explicit to make things clearer.

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFDocument.cpp#newcode84
src/pdf/SkPDFDocument.cpp:84: fileOffset = fCatalog.setFileOffset(fPages[0],
fileOffset);
On 2010/10/11 15:30:58, agl wrote:
> Something is clearly off here. You may wish to discard the return values of
the
> first two calls, in which you shouldn't assign them at all but it looks like
you
> want +=

setFileOffset returned the updated fileoffset (arg2 plus size of the object.)  I
changed this to return just the size of the object, making these +='s

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

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFPage.cpp#newcode76
src/pdf/SkPDFPage.cpp:76: SkTDArray<SkPDFDict*> curNodes;
On 2010/10/11 15:30:58, agl wrote:
> One wonders if an array of SkRefPtrs wouldn't be better, although there may be
> something that I'm missing.

SkTDArray (and the other classes like it), copies the content directly, skipping
constructors and destructors.  This is why SkTDArray is only used for pointers
or simple structs.

I have been and will continue running valgrind to make sure the right things are
happening.

http://codereview.appspot.com/2342043/diff/1/src/pdf/SkPDFPage.cpp#newcode91
src/pdf/SkPDFPage.cpp:91: i++;
On 2010/10/11 15:30:58, agl wrote:
> aren't these two lines a complicated way of saying 'break'?

Indeed. Done.