SKPDF: refactor pdfcatalog and pdfdocument

SKPDF:  refactor pdfcatalog and pdfdocument

SkPDFCatalog:
-   remove first-page-specific code (no longer needed, never
    used) (e.g. addObject()).
-   Make use of SkHashMap for lookups (simplifies code).
-   inline all small methods
-   emitXrefTable moved to SkPDFDocument.cpp
-   no longer store offsets in this data structure (moved to
    SkPDFDocument.cpp)
-   setFileOffset gone.
-   own substitute refs directly.

SkPDFDocument::EmitPDF()
-   All sites that call into SkPDFCatalog modified.
-   catalog.addObject only called in a single place, after the
    resouceSet is built
-   offsets moved to local array.

SkPDFPage:
-   finalizePage no longer deals with SkPDFCatalog or resource sets.
-   GeneratePageTree no longer deals with SkPDFCatalog

SkPDFObjRef
-   emitObject respects the substitution map

Unit Tests:
-   respect SkPDFCatalog::addObject signature change.

SkTHash:
-   #include SkChecksum for SkGoodHash
-   Copyright notice added

Committed: https://skia.googlesource.com/skia/+/f41061cc8dc717b63efe75d2357a3176f04153ec

[hal.canary, 2015-03-23 22:12:11]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-23 22:12:22]

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

[commit-bot: I haz the power, 2015-03-23 22:12:23]

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-24 04:12 UTC

[commit-bot: I haz the power, 2015-03-23 22:19:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 22:19:39]

Try jobs failed on following builders:
  Test-Ubuntu-GCE-NoGPU-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCE-NoGPU-x86_64...)
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[hal.canary, 2015-03-23 22:30:31]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-03-23 22:30:40]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2015-03-23 22:30:46]

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

[commit-bot: I haz the power, 2015-03-23 22:30:47]

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-24 04:30 UTC

[commit-bot: I haz the power, 2015-03-23 22:31:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-23 22:31:59]

Try jobs failed on following builders:
  Test-Ubuntu-GCE-NoGPU-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCE-NoGPU-x86_64...)
  Test-Ubuntu-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCE-NoGPU-x86_64...)

[hal.canary, 2015-03-23 22:42:26]

Patchset #1 (id:20001) has been deleted

[hal.canary, 2015-03-23 22:42:30]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-23 22:42:40]

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

[commit-bot: I haz the power, 2015-03-23 22:42:41]

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-24 04:42 UTC

[hal.canary, 2015-03-23 22:59:31]

The CQ bit was unchecked by halcanary@google.com

[hal.canary, 2015-03-23 22:59:32]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-03-23 22:59:33]

The CQ bit was unchecked by halcanary@google.com

[commit-bot: I haz the power, 2015-03-23 22:59:33]

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

[commit-bot: I haz the power, 2015-03-23 22:59:34]

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-24 04:59 UTC

[hal.canary, 2015-03-23 22:59:55]

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

[hal.canary, 2015-03-23 22:59:56]

PTAL

[mtklein, 2015-03-24 16:03:06]

I got through catalog so far.  This seems like another good candidate to split
into 4 CLs.

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.cpp
File src/pdf/SkPDFCatalog.cpp (right):

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.cp...
src/pdf/SkPDFCatalog.cpp:16: fSubstituteMap.foreach(&Unrefer::Unref);
fSubstituteMap.foreach([](SkPDFObject*, SkPDFObject** v) { (*v)->unref(); });  
?

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h
File src/pdf/SkPDFCatalog.h (right):

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h#...
src/pdf/SkPDFCatalog.h:13: #include "SkChecksum.h"
probably unneeded?

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h#...
src/pdf/SkPDFCatalog.h:33: if (fObjectNumbers.find(obj)) {
These functions working with hashes are all short, but I'm not sure they're good
candidates for inlining.  find() and set() are rather non-trivial methods, and
as template methods, they'll inline through.  We probably want to keep those out
of line.

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h#...
src/pdf/SkPDFCatalog.h:43: int32_t getObjectNumber(SkPDFObject* obj) {
Think it's worth merging addObject and getObjectNumber into 

int32_t findOrAdd(SkPDFObject* o) {
  if (int32_t* n = fObjectNumbers.find(o)) {
    return *n;
  }
  int32_t n = fObjectNumbers.count()+1;
  fObjectNumbers.set(o, n);
  return n;
}

?

[hal.canary, 2015-03-24 20:08:05]

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.cpp
File src/pdf/SkPDFCatalog.cpp (right):

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.cp...
src/pdf/SkPDFCatalog.cpp:16: fSubstituteMap.foreach(&Unrefer::Unref);
On 2015/03/24 16:03:05, mtklein wrote:
> fSubstituteMap.foreach([](SkPDFObject*, SkPDFObject** v) { (*v)->unref(); }); 

> ?

I've never written a C++ lambda before!

Done.

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h
File src/pdf/SkPDFCatalog.h (right):

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h#...
src/pdf/SkPDFCatalog.h:13: #include "SkChecksum.h"
moved to SkTHash where it belongs.

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h#...
src/pdf/SkPDFCatalog.h:33: if (fObjectNumbers.find(obj)) {
On 2015/03/24 16:03:06, mtklein wrote:
> These functions working with hashes are all short, but I'm not sure they're
good
> candidates for inlining.  find() and set() are rather non-trivial methods, and
> as template methods, they'll inline through.  We probably want to keep those
out
> of line.

Done.

https://codereview.chromium.org/1033543002/diff/60001/src/pdf/SkPDFCatalog.h#...
src/pdf/SkPDFCatalog.h:43: int32_t getObjectNumber(SkPDFObject* obj) {
On 2015/03/24 16:03:06, mtklein wrote:
> Think it's worth merging addObject and getObjectNumber into 
> 
> int32_t findOrAdd(SkPDFObject* o) {
>   if (int32_t* n = fObjectNumbers.find(o)) {
>     return *n;
>   }
>   int32_t n = fObjectNumbers.count()+1;
>   fObjectNumbers.set(o, n);
>   return n;
> }
> 
> ?

No.  I will need to know if the object has been added to the catalog for the
next CL (which does away with SkTSet).

We need to get the object number at a different point.

[hal.canary, 2015-03-24 20:09:24]

On 2015/03/24 16:03:06, mtklein wrote:
> I got through catalog so far.  This seems like another good candidate to split
> into 4 CLs.

I changed SkPDFCatalog, then changed its callsites.  I'll try to pull out what I
can.

[hal.canary, 2015-03-25 16:15:36]

Please take a look again.  I can walk you through this if need be.

[mtklein, 2015-03-25 18:00:40]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-03-25 18:00:41]

lgtm

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

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

[commit-bot: I haz the power, 2015-03-25 18:02:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 18:02:24]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[hal.canary, 2015-03-25 18:17:54]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-03-25 18:17:55]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1033543002/#ps160001
(title: "2015-03-25 (Wednesday) 14:17:42 EDT")

[commit-bot: I haz the power, 2015-03-25 18:18:05]

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

[commit-bot: I haz the power, 2015-03-25 18:29:23]

Committed patchset #7 (id:160001) as
https://skia.googlesource.com/skia/+/f41061cc8dc717b63efe75d2357a3176f04153ec