[PDF] Make stream compression optional on a per device basis.

[PDF] Make stream compression optional on a per device basis.

There are a lot of small pieces to make this change work:
- SkPDFDocument (and SkPDFCatalog) take flags to disable compression (and font embedding - not implemented yet, can disable font subsetting for now).
- SkPDFStream now defers compression until the size/emit step.
- Classes that *had* a stream (because they didn't know the stream size at construction time) now *are* streams to make the substitution work correctly.
- The SkPDFShader implementation got pulled apart into two classes, one that is a SkPDFDict, and one that is a SkPDFStream (making the common ancestor SkPDFObject).
- Added helper methods in SkPDFObject for children that have simple resource lists.
- Added an iterator to SkPDFDict so that a substitute SkPDFStream can get a copy of the stream dictionary.
- Change SkPDFDocument to have a pointer to an SkPDFCatalog to remove a new circular header reference.

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

[Steve VanDeBogart, 2011-07-13 20:55:21]

I wanted to get this done before I left, but it turned out to require a lot of
changes.  I can break this up into a number of different CLs if you prefer.  If
you end up with nothing but nits, feel free to commit it before I get back.

[Steve VanDeBogart, 2011-07-19 00:12:16]

So, do you want me to break this out into smaller changes?

[Chris Guillory, 2011-07-19 01:31:33]

If you think that makes sense. I haven't gotten through this one just yet.

On Mon, Jul 18, 2011 at 5:12 PM, <vandebo@chromium.org> wrote:

> So, do you want me to break this out into smaller changes?
>
>
>
http://codereview.appspot.com/**4700045/<http://codereview.appspot.com/4700045/>
>

[Chris Guillory, 2011-07-19 18:27:45]

LGTM. Some comments.

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

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFCatalog.h#ne...
include/pdf/SkPDFCatalog.h:68: SkPDFDocument::Flags getDocumentFlags() { return
fDocumentFlags; }
Function can be const.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#n...
include/pdf/SkPDFDocument.h:26: class SkPDFCatalog;
Nit: Not in sort order.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#n...
include/pdf/SkPDFDocument.h:37: kNoCompression_Flag = 0x01,  //<mask disable
stream compression.
Nit: Why do these comment have a '<' character?

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#n...
include/pdf/SkPDFDocument.h:39: 
Nit: Extra newline.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h
File include/pdf/SkPDFShader.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h#new...
include/pdf/SkPDFShader.h:56: class ShaderCanonicalEntry {
Struct?

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h#new...
include/pdf/SkPDFShader.h:65: static SkTDArray<ShaderCanonicalEntry>&
canonicalShaders();
Can we make these other static member functions start with a capital letter.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFStream.h
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFStream.h#new...
include/pdf/SkPDFStream.h:37: *  @param stream The data part of the stream.
stream -> data

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFStream.h#new...
include/pdf/SkPDFStream.h:40: // Deprecated constructor.
Nit: Use /* */ comment style.

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFDocument.cpp#new...
src/pdf/SkPDFDocument.cpp:19: #include "SkPDFCatalog.h"
Not in sort order.

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFDocument.cpp#new...
src/pdf/SkPDFDocument.cpp:150: for (int i = fSecondPageFirstResourceIndex; i <
fPageResources.count(); i++)
Braces.

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:290: class SkPDFShader::State {
struct?

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFStream.cpp#newco...
src/pdf/SkPDFStream.cpp:23: static bool skipCompression(SkPDFCatalog* catalog) {
This doesn't appear to follow the static function naming convention.

[reed1, 2011-07-19 18:41:53]

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (left):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#o...
include/pdf/SkPDFDocument.h:56: private:
What is motivating going from an embedded object to a ptr?

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h
File include/pdf/SkPDFShader.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h#new...
include/pdf/SkPDFShader.h:53: protected:
Yay! I like hiding the details.

http://codereview.appspot.com/4700045/diff/2001/src/core/SkFlate.cpp
File src/core/SkFlate.cpp (left):

http://codereview.appspot.com/4700045/diff/2001/src/core/SkFlate.cpp#oldcode28
src/core/SkFlate.cpp:28: bool SkFlate::HaveFlate() {
Yay!

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFStream.cpp#newco...
src/pdf/SkPDFStream.cpp:23: static bool skipCompression(SkPDFCatalog* catalog) {
On 2011/07/19 18:27:45, Chris Guillory wrote:
> This doesn't appear to follow the static function naming convention.

For local functions its fine. The only real convention we try to follow is to
CapitalizeStaticMethodsInAClass.

[Steve VanDeBogart, 2011-07-19 18:49:24]

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (left):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#o...
include/pdf/SkPDFDocument.h:56: private:
On 2011/07/19 18:41:53, reed1 wrote:
> What is motivating going from an embedded object to a ptr?

With the other changes, we ended up with a circular reference in headers
(SkPDFDocument.h depended on SkPDFCatalog, which depended on SkPDFDocument.h).
Making this a pointer breaks the cycle.

[reed1, 2011-07-19 18:50:51]

Too bad.

On Tue, Jul 19, 2011 at 2:49 PM,  <vandebo@chromium.org> wrote:
>
> http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h
> File include/pdf/SkPDFDocument.h (left):
>
>
http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#o...
> include/pdf/SkPDFDocument.h:56: private:
> On 2011/07/19 18:41:53, reed1 wrote:
>>
>> What is motivating going from an embedded object to a ptr?
>
> With the other changes, we ended up with a circular reference in headers
> (SkPDFDocument.h depended on SkPDFCatalog, which depended on
> SkPDFDocument.h). Making this a pointer breaks the cycle.
>
> http://codereview.appspot.com/4700045/
>

[Steve VanDeBogart, 2011-07-20 01:28:26]

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

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFCatalog.h#ne...
include/pdf/SkPDFCatalog.h:68: SkPDFDocument::Flags getDocumentFlags() { return
fDocumentFlags; }
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Function can be const.

Done.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h
File include/pdf/SkPDFDocument.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#n...
include/pdf/SkPDFDocument.h:37: kNoCompression_Flag = 0x01,  //<mask disable
stream compression.
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Nit: Why do these comment have a '<' character?

Oops, missed the !.  See SkPaint.h:100

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFDocument.h#n...
include/pdf/SkPDFDocument.h:39: 
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Nit: Extra newline.

That's intentional, kDraftMode is a combination of other flags.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h
File include/pdf/SkPDFShader.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h#new...
include/pdf/SkPDFShader.h:56: class ShaderCanonicalEntry {
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Struct?

Because it actually has functionality (operator==) I think the style guide
suggests class.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFShader.h#new...
include/pdf/SkPDFShader.h:65: static SkTDArray<ShaderCanonicalEntry>&
canonicalShaders();
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Can we make these other static member functions start with a capital letter.

Done.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFStream.h
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFStream.h#new...
include/pdf/SkPDFStream.h:37: *  @param stream The data part of the stream.
On 2011/07/19 18:27:45, Chris Guillory wrote:
> stream -> data

Done.

http://codereview.appspot.com/4700045/diff/2001/include/pdf/SkPDFStream.h#new...
include/pdf/SkPDFStream.h:40: // Deprecated constructor.
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Nit: Use /* */ comment style.

Done.

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFDocument.cpp#new...
src/pdf/SkPDFDocument.cpp:19: #include "SkPDFCatalog.h"
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Not in sort order.

Done.

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFDocument.cpp#new...
src/pdf/SkPDFDocument.cpp:150: for (int i = fSecondPageFirstResourceIndex; i <
fPageResources.count(); i++)
On 2011/07/19 18:27:45, Chris Guillory wrote:
> Braces.

Done.

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFShader.cpp#newco...
src/pdf/SkPDFShader.cpp:290: class SkPDFShader::State {
On 2011/07/19 18:27:45, Chris Guillory wrote:
> struct?

operator==

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

http://codereview.appspot.com/4700045/diff/2001/src/pdf/SkPDFStream.cpp#newco...
src/pdf/SkPDFStream.cpp:23: static bool skipCompression(SkPDFCatalog* catalog) {
On 2011/07/19 18:27:45, Chris Guillory wrote:
> This doesn't appear to follow the static function naming convention.

Done.

[Chris Guillory, 2011-07-20 01:32:41]

http://codereview.appspot.com/4700045/diff/15001/include/pdf/SkPDFStream.h
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/4700045/diff/15001/include/pdf/SkPDFStream.h#ne...
include/pdf/SkPDFStream.h:63: kUnused_State,         //<The stream hasn't been
requested yet.
Need ! here too.

[Steve VanDeBogart, 2011-07-20 17:38:40]

http://codereview.appspot.com/4700045/diff/15001/include/pdf/SkPDFStream.h
File include/pdf/SkPDFStream.h (right):

http://codereview.appspot.com/4700045/diff/15001/include/pdf/SkPDFStream.h#ne...
include/pdf/SkPDFStream.h:63: kUnused_State,         //<The stream hasn't been
requested yet.
On 2011/07/20 01:32:41, Chris Guillory wrote:
> Need ! here too.

Done.