Simplify SkPDFShader class. Now invalid objects are never created.

Simplify SkPDFShader class.  Now invalid objects are never created.

"Constructors should not do real work"

I have verified that all test PDFs render identically.

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

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

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-21 23:14:05]

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

[commit-bot: I haz the power, 2015-01-21 23:14:06]

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-22 05:14 UTC

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

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

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

ptal

[commit-bot: I haz the power, 2015-01-22 05:14:07]

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

[commit-bot: I haz the power, 2015-01-22 05:14:07]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[mtklein, 2015-01-22 15:54:31]

You're blowing my mind Hal.

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

https://codereview.chromium.org/862113004/diff/1/src/pdf/SkPDFShader.cpp#newc...
src/pdf/SkPDFShader.cpp:746: alphaFunctionShader->fSelf = alphaFunctionShader;
What is the possible value in assigning x->fSelf = x without ever setting fSelf
to anything else?  I see 
  alphaFunctionShader->fSelf = alphaFunctionShader;
  pdfFunctionShader->fSelf = pdfFunctionShader;
  imageShader->fSelf = imageShader;
and that's it.

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

https://codereview.chromium.org/862113004/diff/1/src/pdf/SkPDFShader.h#newcode53
src/pdf/SkPDFShader.h:53: SkPDFObject* fSelf;
This is just aching for a comment.  My first thought is, WTF, you mean 'this'?

[hal.canary, 2015-01-22 18:23:00]

On 2015/01/22 15:54:31, mtklein wrote:
> You're blowing my mind Hal.
> 


Since it's such a nonstandard thing to do, I have reverted that part of it. 
ptal now.

[mtklein, 2015-01-22 18:32:41]

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

https://codereview.chromium.org/862113004/diff/20001/src/pdf/SkPDFShader.h#ne...
src/pdf/SkPDFShader.h:62: virtual SkPDFObject* toPDFObject() = 0;
Uh, wait, what's really going on here?  Is SkPDFShader not an SkPDFObject, but
all its subclasses are?  That seems bad.

[hal.canary, 2015-01-22 18:44:58]

On 2015/01/22 18:32:41, mtklein wrote:
> https://codereview.chromium.org/862113004/diff/20001/src/pdf/SkPDFShader.h
> File src/pdf/SkPDFShader.h (right):
> 
>
https://codereview.chromium.org/862113004/diff/20001/src/pdf/SkPDFShader.h#ne...
> src/pdf/SkPDFShader.h:62: virtual SkPDFObject* toPDFObject() = 0;
> Uh, wait, what's really going on here?  Is SkPDFShader not an SkPDFObject, but
> all its subclasses are?  That seems bad.

It's been like that since before me.  I found it hard to think about myself. 
Another option is to have separate SkPDFCanon calls for 
SkPDFImageShader, SkPDFAlphaFunctionShader, and SkPDFFunctionShader.  I kind of
like that better.

[hal.canary, 2015-01-22 18:47:04]

On 2015/01/22 18:44:58, Hal Canary wrote:
> On 2015/01/22 18:32:41, mtklein wrote:
> > https://codereview.chromium.org/862113004/diff/20001/src/pdf/SkPDFShader.h
> > File src/pdf/SkPDFShader.h (right):
> > 
> >
>
https://codereview.chromium.org/862113004/diff/20001/src/pdf/SkPDFShader.h#ne...
> > src/pdf/SkPDFShader.h:62: virtual SkPDFObject* toPDFObject() = 0;
> > Uh, wait, what's really going on here?  Is SkPDFShader not an SkPDFObject,
but
> > all its subclasses are?  That seems bad.
> 
> It's been like that since before me.  I found it hard to think about myself. 
> Another option is to have separate SkPDFCanon calls for 
> SkPDFImageShader, SkPDFAlphaFunctionShader, and SkPDFFunctionShader.  I kind
of
> like that better.

If you like that direction, let me land this CL, and then build on this.

[mtklein, 2015-01-22 18:51:36]

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

https://codereview.chromium.org/862113004/diff/20001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:729: return NULL;
This leaks state.  You might want to pass SkAutoTDelete<State>* to these
Create() methods so you can call detach() as late as possible instead of being
forced to call it early.

[hal.canary, 2015-01-22 21:05:09]

On 2015/01/22 18:51:36, mtklein wrote:
> This leaks state.  You might want to pass SkAutoTDelete<State>* to these
> Create() methods so you can call detach() as late as possible instead of being
> forced to call it early.

Great idea.  I'll do that.

[hal.canary, 2015-01-22 21:05:40]

I mean "I did that and please take a look."

[mtklein, 2015-01-22 21:16:33]

I know you like clang-format, but please keep in mind it does not absolve you of
responsibility for keeping your code readable.  It makes bad decisions which you
should undo.

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

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:603: SkAutoTDelete<State>& state = *statePtr;
This seems fine, but you might want to go all the way down to State* instead of
SkAutoTDelete<State>& ?

State* state = ownedState->get();
...
return SkPDFImageShder::Create(ownedState);

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:749: alphaFunctionShader->setData(colorStream.get());
Hey, what's the lifetime of colorStream?  If SkStreams aren't ref-counted
anymore, how is this safe?

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:814: ptr->unref();
This seemed more readable as you had it all on one line?

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:818: SK_DECLARE_STATIC_LAZY_PTR(SkPDFObject,
Also seemed more readable on two lines as you had it.

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:954: SkNEW_ARGS(SkPDFFunctionShader, (state.detach()));
?  Either way seems fine I guess, but I've seen more code in Skia 4 in than 8.

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:1172: SkNEW_ARGS(SkPDFImageShader, (state.detach()));
ditto

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.h
File src/pdf/SkPDFShader.h (right):

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.h#ne...
src/pdf/SkPDFShader.h:56: // This also takes ownership of shaderState.
// This also will detach() and take ownership of the State.  ?

[hal.canary, 2015-01-22 21:46:00]

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

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:603: SkAutoTDelete<State>& state = *statePtr;
On 2015/01/22 21:16:32, mtklein wrote:
> This seems fine, but you might want to go all the way down to State* instead
of
> SkAutoTDelete<State>& ?
> 
> State* state = ownedState->get();
> ...
> return SkPDFImageShder::Create(ownedState);

Even further:

  const State& state = **ownedState;

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:749: alphaFunctionShader->setData(colorStream.get());
On 2015/01/22 21:16:33, mtklein wrote:
> Hey, what's the lifetime of colorStream?  If SkStreams aren't ref-counted
> anymore, how is this safe?

duplicate() is called on the stream.

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:814: ptr->unref();
On 2015/01/22 21:16:33, mtklein wrote:
> This seemed more readable as you had it all on one line?

Done.

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:818: SK_DECLARE_STATIC_LAZY_PTR(SkPDFObject,
On 2015/01/22 21:16:33, mtklein wrote:
> Also seemed more readable on two lines as you had it.

Done.

[mtklein, 2015-01-22 21:54:21]

lgtm

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

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:603: SkAutoTDelete<State>& state = *statePtr;
On 2015/01/22 21:46:00, Hal Canary wrote:
> On 2015/01/22 21:16:32, mtklein wrote:
> > This seems fine, but you might want to go all the way down to State* instead
> of
> > SkAutoTDelete<State>& ?
> > 
> > State* state = ownedState->get();
> > ...
> > return SkPDFImageShder::Create(ownedState);
> 
> Even further:
> 
>   const State& state = **ownedState;
> 

+1

https://codereview.chromium.org/862113004/diff/40001/src/pdf/SkPDFShader.cpp#...
src/pdf/SkPDFShader.cpp:749: alphaFunctionShader->setData(colorStream.get());
On 2015/01/22 21:46:00, Hal Canary wrote:
> On 2015/01/22 21:16:33, mtklein wrote:
> > Hey, what's the lifetime of colorStream?  If SkStreams aren't ref-counted
> > anymore, how is this safe?
> 
> duplicate() is called on the stream.

Interesting.  Seems like we might want to switch that over to hand it off.  Good
to know it's safe.

[hal.canary, 2015-01-22 21:55:29]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-22 21:55:51]

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

[commit-bot: I haz the power, 2015-01-23 12:18:59]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/bc59ac6b12bbded2117fe3aa9643b2d138e5ddda