SkPDF: Refactor SkPDFObject heiararchy.

SkPDF: Refactor SkPDFObject heiararchy.

Flatten and use a tagged union type

        Δmemory ~= -2.4% ± 0.5%
        Δtime   ~= -1.2% ± 0.2%

BUG=skia:3585
Committed: https://skia.googlesource.com/skia/+/130444fdaf737c5931a934018ab50b0c91a3499e

[hal.canary, 2015-04-23 19:25:44]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 19:26:01]

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

[commit-bot: I haz the power, 2015-04-23 19:26:02]

Dry run: COMMIT=false is deprecated please use the dry run button instead.

[hal.canary, 2015-04-23 20:48:00]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 20:48:20]

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

[commit-bot: I haz the power, 2015-04-23 20:53:30]

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

[commit-bot: I haz the power, 2015-04-23 20:53:33]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2015-04-23 21:00:50]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-04-23 21:00:59]

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

[hal.canary, 2015-04-23 21:01:03]

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

[hal.canary, 2015-04-23 21:01:04]

PTAL

[tomhudson, 2015-04-24 21:32:30]

lgtm

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (left):

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp#...
src/pdf/SkPDFTypes.cpp:236: SkASSERT(substitutes.getSubstitute(fValue[i]) ==
fValue[i]);
Not worth maintaining this invariant?

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

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp#...
src/pdf/SkPDFTypes.cpp:43: if (this != &other) {
Never worked with && before, so I'm assuming you've got this right / tested it
explicitly / it's isomorphic to **.

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp#...
src/pdf/SkPDFTypes.cpp:412: // }
Commented out code - delete?

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

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:66: // move
This comment tells me nothing. Please improve.
(Getting rid of is acceptable, but since I haven't a clue why this function
exists I'd rather see an explanation here.)

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:127: static bool IsValidName(const char*);
Musing for future consideration: do we ever use this other than in a construct
something like:

SkString name = ...
if (!SkPDFUnion::IsValidName(name.c_str())) {
  name = SkPDFUnion::EscapeName(name.c_str(), strlen);
}

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:344: *  ownership of the object;
Nit: interrupted comment edit?

[hal.canary, 2015-04-24 21:51:46]

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

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp#...
src/pdf/SkPDFTypes.cpp:43: if (this != &other) {
On 2015/04/24 21:32:29, tomhudson wrote:
> Never worked with && before, so I'm assuming you've got this right / tested it
> explicitly / it's isomorphic to **.

It is not isomorphic to **.

This is C++11 notation for a r-value reference, that is: a temporary value.

[hal.canary, 2015-04-25 13:41:02]

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp
File src/pdf/SkPDFTypes.cpp (left):

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp#...
src/pdf/SkPDFTypes.cpp:236: SkASSERT(substitutes.getSubstitute(fValue[i]) ==
fValue[i]);
Only top level objects (referred to via ObjRefs) should have substitutes.  I put
this assert in place to make sure I understood the code.  Now I am confident it
is unnecessary.

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

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.cpp#...
src/pdf/SkPDFTypes.cpp:412: // }
On 2015/04/24 21:32:29, tomhudson wrote:
> Commented out code - delete?

Done.

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

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:66: // move
On 2015/04/24 21:32:29, tomhudson wrote:
> This comment tells me nothing. Please improve.
> (Getting rid of is acceptable, but since I haven't a clue why this function
> exists I'd rather see an explanation here.)

Done.  Please let me know if you need more information to clarify.

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:127: static bool IsValidName(const char*);
On 2015/04/24 21:32:30, tomhudson wrote:
> Musing for future consideration: do we ever use this other than in a construct
> something like:
> 
> SkString name = ...
> if (!SkPDFUnion::IsValidName(name.c_str())) {
>   name = SkPDFUnion::EscapeName(name.c_str(), strlen);
> }

Actually, in every case we create a Name object, we either KNOW it is a valid
name, in which case we ASSERT(IsValidName("name'))  If we don't know, we always
escape it.

Since this function is not used outside of this class, I'm removing it from the
header.

https://codereview.chromium.org/1069103003/diff/60001/src/pdf/SkPDFTypes.h#ne...
src/pdf/SkPDFTypes.h:344: *  ownership of the object;
On 2015/04/24 21:32:30, tomhudson wrote:
> Nit: interrupted comment edit?

Thinking in C.  ';' ends a statement.

[hal.canary, 2015-04-25 13:41:18]

The CQ bit was checked by halcanary@google.com

[hal.canary, 2015-04-25 13:41:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/1069103003/#ps80001
(title: "2015-04-25 (Saturday) 09:40:48 EDT")

[commit-bot: I haz the power, 2015-04-25 13:41:31]

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

[commit-bot: I haz the power, 2015-04-25 13:45:13]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/130444fdaf737c5931a934018ab50b0c91a3499e