PDF: Canon now owns a reference to all interned objects

PDF: Canon now owns a reference to all interned objects

Add SkPDFCanon::reset function to unref all objects.

No longer possible to remove object from canon

Motivation: this doesn't change these object's lifetime, (they will
still be fully unrefed when SkDocument::close() is called, but we no
longer have to remove them from the array when their destructor is
called.

Committed: https://skia.googlesource.com/skia/+/2e3f9d8a9309686eeb4c76ccfde5800da87a68b3

[hal.canary, 2015-02-27 19:12:40]

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

[hal.canary, 2015-02-27 19:12:40]

PTAL

[mtklein, 2015-02-27 19:50:17]

https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp
File src/pdf/SkPDFBitmap.cpp (right):

https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp#newc...
src/pdf/SkPDFBitmap.cpp:316: canon->addBitmap(SkRef(pdfBitmap));
This seems to distribute the responsibility for refcounting strangely.

Why isn't the ref inside addBitmap?  (Same for all other calls?)

[hal.canary, 2015-02-27 19:51:31]

On 2015/02/27 19:50:17, mtklein wrote:
> https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp
> File src/pdf/SkPDFBitmap.cpp (right):
> 
>
https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp#newc...
> src/pdf/SkPDFBitmap.cpp:316: canon->addBitmap(SkRef(pdfBitmap));
> This seems to distribute the responsibility for refcounting strangely.
> 
> Why isn't the ref inside addBitmap?  (Same for all other calls?)

No reason.  Would you prefer it the other way?

[mtklein, 2015-02-27 19:57:09]

On 2015/02/27 19:51:31, Hal Canary wrote:
> On 2015/02/27 19:50:17, mtklein wrote:
> > https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp
> > File src/pdf/SkPDFBitmap.cpp (right):
> > 
> >
>
https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp#newc...
> > src/pdf/SkPDFBitmap.cpp:316: canon->addBitmap(SkRef(pdfBitmap));
> > This seems to distribute the responsibility for refcounting strangely.
> > 
> > Why isn't the ref inside addBitmap?  (Same for all other calls?)
> 
> No reason.  Would you prefer it the other way?

Yeah, very much so.  It's important.  Might help if you re-read the canon code
in isolation.  It unrefs() things but never refs() them.  That's super WTF.

It's also weird on the other side of the canon API to create an object then
immediately ref it.  It's already ref'd when created.  It reads like a bug.

We shouldn't have to flip back and forth between two files to prove they're
correct.

[hal.canary, 2015-02-27 20:13:43]

https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp
File src/pdf/SkPDFBitmap.cpp (right):

https://codereview.chromium.org/966863002/diff/1/src/pdf/SkPDFBitmap.cpp#newc...
src/pdf/SkPDFBitmap.cpp:316: canon->addBitmap(SkRef(pdfBitmap));
On 2015/02/27 19:50:17, mtklein wrote:
> This seems to distribute the responsibility for refcounting strangely.
> 
> Why isn't the ref inside addBitmap?  (Same for all other calls?)

Done.

[hal.canary, 2015-02-27 20:13:54]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-02-27 20:14:16]

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

[commit-bot: I haz the power, 2015-02-27 20:14:17]

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-02-28 02:14 UTC

[mtklein, 2015-02-27 20:15:26]

lgtm

[commit-bot: I haz the power, 2015-02-27 20:16:26]

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

[commit-bot: I haz the power, 2015-02-27 20:16:26]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)

[hal.canary, 2015-02-27 20:33:28]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-02-27 20:33:29]

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/966863002/#ps40001 (title:
"2015-02-27 (Friday) 15:27:08 EST")

[commit-bot: I haz the power, 2015-02-27 20:34:15]

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

[commit-bot: I haz the power, 2015-02-27 20:41:09]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/2e3f9d8a9309686eeb4c76ccfde5800da87a68b3