More changes to SkPDFShader to eliminate multiple inheritance!

More changes to SkPDFShader to eliminate multiple inheritance!

Committed: https://skia.googlesource.com/skia/+/530ea8e24bc88f2d7973c35a703f18c1dafb56dc

[hal.canary, 2015-01-22 23:37:35]

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

[hal.canary, 2015-01-22 23:37:35]

This needs to be rebased on http://crrev.com/862113004 when that lands.  But
this will make things easier to read.

[hal.canary, 2015-01-23 13:20:45]

Patchset #3 (id:30005) has been deleted

[hal.canary, 2015-01-23 13:54:58]

On 2015/01/22 23:37:35, Hal Canary wrote:
> This needs to be rebased on http://crrev.com/862113004 when that lands.  But
> this will make things easier to read.

done.  

I have verified that all test PDFs render identically.

[hal.canary, 2015-01-23 13:55:02]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-23 13:55:50]

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

[commit-bot: I haz the power, 2015-01-23 13:55:50]

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-01-23 19:55 UTC

[mtklein, 2015-01-23 14:16:11]

lgtm

https://codereview.chromium.org/873543002/diff/20001/src/pdf/SkPDFCanon.cpp
File src/pdf/SkPDFCanon.cpp (right):

https://codereview.chromium.org/873543002/diff/20001/src/pdf/SkPDFCanon.cpp#n...
src/pdf/SkPDFCanon.cpp:42: // TODO(halcanary):  add this method to SkTDArray.
I'm on board with a ? at the end.

Generally I don't think it's a good idea to put methods like this on a class,
here for two reasons:
  1) you can implement it just as efficiently as the class can, 
     in a few lines of code;
  2) it's slow relative to the main methods of the class.

By 2), I mean that most important methods of SkTDArray are worst-case or
amortized O(1), so an O(N) method is slow relative to SkTDArray.  It seems to be
encouraging bad code to put an O(N) method in there among the O(1) ones unless
it's named something like slowRemove().

Centralizing convenience code like this is a maintenance burden and tends to
lock code into certain data structures and algorithms.  If you put this on
SkTDArray, other code might start calling it.  Then if you want to start sorting
your arrays, you'll either have to make sure the other callers are also sorted,
or abandon your use of the method and write a new one.  Either way you're making
extra work for our future selves, if only because it becomes that many more
entities we have to think about when working on this code.

The benefit of code reuse usually outweighs the burden of the coupling that
comes from centralization when it's offering up some fundamentally new
capability, or maybe if it is a particularly challenging or subtle method to
write.  Code size, whether lines in source or bytes of machine code, is not
important relative to these other concerns, and centralizing template methods
never helps that anyway.

https://codereview.chromium.org/873543002/diff/20001/src/pdf/SkPDFCanon.cpp#n...
src/pdf/SkPDFCanon.cpp:55: T* find_item(const SkTDArray<T*>& ptrArray, const U&
object) {
consider object -> key ?

https://codereview.chromium.org/873543002/diff/20001/src/pdf/SkPDFShader.cpp
File src/pdf/SkPDFShader.cpp (right):

https://codereview.chromium.org/873543002/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:702: SkAutoTUnref<SkPDFGraphicState>
alphaGs(createsmask_graphic_state(state));
Let's add an _ between create and smask?

[commit-bot: I haz the power, 2015-01-23 14:17:37]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/530ea8e24bc88f2d7973c35a703f18c1dafb56dc

[hal.canary, 2015-01-23 15:14:09]

On 2015/01/23 14:16:11, mtklein wrote:
> > add this method to SkTDArray.
> I'm on board with a ? at the end.

comment removed in http://crrev.com/872643005 ; this isn't a burden as is.


> > alphaGs(createsmask_graphic_state(state));
> Let's add an _ between create and smask?

done in http://crrev.com/872643005