SkPDF: clean up skpdfdocument, add static functions

SkPDF: clean up skpdfdocument, add static functions

Add SkPDFDocument::EmitPDF and SkPDFDocument::GetCountOfFontTypes

Also, make SkPDFDocument::appendPage return void, not bool.

Motivation: These static functions can be used to print subsets of a
pdf document (functionality to be added in a later CL).

BUG=skia:3585
Committed: https://skia.googlesource.com/skia/+/9d85145590894e8114056f0bbe24ba76581336c5

[hal.canary, 2015-03-25 12:49:52]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-25 12:50:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1036853002/1

[commit-bot: I haz the power, 2015-03-25 12:50:01]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-03-25 18:50 UTC

[hal.canary, 2015-03-25 12:50:38]

halcanary@google.com changed reviewers:
+ mtklein@google.com

[hal.canary, 2015-03-25 12:50:38]

ptal

[mtklein, 2015-03-25 13:10:08]

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h
File src/pdf/SkPDFDocument.h (right):

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:30: class SkPDFDocument {
It's starting to seem like SkPDFDocument doesn't need to exist.  It's becoming a
hanger for two static methods.  Is there more than one user of SkPDFDocument? 
If not, it seems like we should start migrating all this code to that single
user and delete this.

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:33: ~SkPDFDocument() { fPageDevices.unrefAll(); }
This destructor seems like another good candidate to leave outlined.  It's not
really trivial work.  I guess it's okay if it's only called once, but then that
circles back to wondering why SkPDFDocument needs to exist.

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:40: *                     should be created using the
same SkPDFCanon.
What happens if they're not?  Conflicting IDs?

I'm somewhat suspicious of classes which cannot enforce their own operational
invariants.  Seems like someone somewhere can enforce this structurally rather
than assert it, and this work should be done by them.

[hal.canary, 2015-03-25 13:11:49]

On 2015/03/25 13:10:08, mtklein wrote:
> https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h
> File src/pdf/SkPDFDocument.h (right):
> 
>
https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
> src/pdf/SkPDFDocument.h:30: class SkPDFDocument {
> It's starting to seem like SkPDFDocument doesn't need to exist.  It's becoming
a
> hanger for two static methods.  Is there more than one user of SkPDFDocument? 
> If not, it seems like we should start migrating all this code to that single
> user and delete this.
> 
>
https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
> src/pdf/SkPDFDocument.h:33: ~SkPDFDocument() { fPageDevices.unrefAll(); }
> This destructor seems like another good candidate to leave outlined.  It's not
> really trivial work.  I guess it's okay if it's only called once, but then
that
> circles back to wondering why SkPDFDocument needs to exist.
> 
>
https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
> src/pdf/SkPDFDocument.h:40: *                     should be created using the
> same SkPDFCanon.
> What happens if they're not?  Conflicting IDs?
> 
> I'm somewhat suspicious of classes which cannot enforce their own operational
> invariants.  Seems like someone somewhere can enforce this structurally rather
> than assert it, and this work should be done by them.

You asked me to split up the CL.  This is step one.

[mtklein, 2015-03-25 13:12:18]

On 2015/03/25 13:11:49, Hal Canary wrote:
> On 2015/03/25 13:10:08, mtklein wrote:
> > https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h
> > File src/pdf/SkPDFDocument.h (right):
> > 
> >
>
https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
> > src/pdf/SkPDFDocument.h:30: class SkPDFDocument {
> > It's starting to seem like SkPDFDocument doesn't need to exist.  It's
becoming
> a
> > hanger for two static methods.  Is there more than one user of
SkPDFDocument? 
> > If not, it seems like we should start migrating all this code to that single
> > user and delete this.
> > 
> >
>
https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
> > src/pdf/SkPDFDocument.h:33: ~SkPDFDocument() { fPageDevices.unrefAll(); }
> > This destructor seems like another good candidate to leave outlined.  It's
not
> > really trivial work.  I guess it's okay if it's only called once, but then
> that
> > circles back to wondering why SkPDFDocument needs to exist.
> > 
> >
>
https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
> > src/pdf/SkPDFDocument.h:40: *                     should be created using
the
> > same SkPDFCanon.
> > What happens if they're not?  Conflicting IDs?
> > 
> > I'm somewhat suspicious of classes which cannot enforce their own
operational
> > invariants.  Seems like someone somewhere can enforce this structurally
rather
> > than assert it, and this work should be done by them.
> 
> You asked me to split up the CL.  This is step one.

What are the other steps?

[hal.canary, 2015-03-25 13:12:29]

On 2015/03/25 13:11:49, Hal Canary wrote:
> You asked me to split up the CL.  This is step one.


or rather step ~4 of ~8.

[hal.canary, 2015-03-25 13:14:39]

On 2015/03/25 13:12:18, mtklein wrote:
> > > If not, it seems like we should start migrating all this code to that
single
> > > user and delete this.
> > 
> > You asked me to split up the CL.  This is step one.
> 
> What are the other steps?

-  Eliminate SkPDFDocument as a class (rather than a collection of static
functions)
-  Move the array of pages to SkDocument_PDF.
-  Let SkDocument_PDF enforce the single-canon rule (like it already does).

[mtklein, 2015-03-25 13:17:07]

lgtm

[hal.canary, 2015-03-25 13:19:45]

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h
File src/pdf/SkPDFDocument.h (right):

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:30: class SkPDFDocument {
On 2015/03/25 13:10:08, mtklein wrote:
> It's starting to seem like SkPDFDocument doesn't need to exist.  It's becoming
a
> hanger for two static methods.  Is there more than one user of SkPDFDocument? 
> If not, it seems like we should start migrating all this code to that single
> user and delete this.

Options:
-  Keep this functionality together in a it's current .cpp
-  Rename the .cpp to SkPDFEmiter.cpp or something
-  move static functions to SkPDFDevice (already a very big file).

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:33: ~SkPDFDocument() { fPageDevices.unrefAll(); }
On 2015/03/25 13:10:08, mtklein wrote:
> This destructor seems like another good candidate to leave outlined.  It's not
> really trivial work.  I guess it's okay if it's only called once, but then
that
> circles back to wondering why SkPDFDocument needs to exist.

It only gets called from one place (skdocument_pdf), and that class will take
direct ownership in a later cl.

https://codereview.chromium.org/1036853002/diff/1/src/pdf/SkPDFDocument.h#new...
src/pdf/SkPDFDocument.h:40: *                     should be created using the
same SkPDFCanon.
On 2015/03/25 13:10:08, mtklein wrote:
> What happens if they're not?  Conflicting IDs?

No de-duping across pages.  (a too-big pdf)

> I'm somewhat suspicious of classes which cannot enforce their own operational
> invariants.  Seems like someone somewhere can enforce this structurally rather
> than assert it, and this work should be done by them.

*You can not make this mistake from the public API.*

[commit-bot: I haz the power, 2015-03-25 14:11:11]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/9d85145590894e8114056f0bbe24ba76581336c5