SkPDF: PDFShader code modernized.

SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2193973002

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

[hal.canary, 2016-07-29 21:04:09]

Description was changed from

==========
SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkAutoFree gets move constructor

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
==========

to

==========
SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkAutoFree gets move constructor

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2193973002
==========

[hal.canary, 2016-07-29 21:04:42]

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

[commit-bot: I haz the power, 2016-07-29 21:04:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hal.canary, 2016-07-29 21:37:49]

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

[hal.canary, 2016-07-29 21:37:49]

PTAL

[hal.canary, 2016-07-29 21:54:45]

Description was changed from

==========
SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkAutoFree gets move constructor

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2193973002
==========

to

==========
SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2193973002
==========

[hal.canary, 2016-08-01 14:32:32]

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

[commit-bot: I haz the power, 2016-08-01 14:32:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 14:59:08]

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

[commit-bot: I haz the power, 2016-08-01 14:59:09]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2016-08-02 20:59:12]

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFCanon.cpp#...
src/pdf/SkPDFCanon.cpp:25: fGraphicStateRecords.foreach ([](WrapGS w) {
w.fPtr->unref(); });
Nit: TODO(halcanary) replace with sk_sp<> so we don't need the explicit
teardown?

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFCanon.h#ne...
src/pdf/SkPDFCanon.h:51: sk_sp<SkPDFObject> findFunctionShader(const
SkPDFShader::State&) const;
Just so that I understand: these functions are named "find*" but they're
returning a smart pointer so that a caller *can* take ownership if it needs to?

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:1817: SkXfermode::kSrcOver_Mode, true);
eh, I guess this is a wash in terms of intelligibility.

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:530: const SkPDFShader::State& state);
Why forward declare these? Why not just define them here?

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:627: static std::unique_ptr<SkStreamAsset>
create_pattern_fill_content(
Tangent: am I suppose to conclude something from your choice to use
std::unique_ptr<> instead of sk_sp<>?

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFShader.h#n...
src/pdf/SkPDFShader.h:52: class State {
This goes from hidden to public and mostly unencapsulated?
Except there's no part of the API uses SkPDFShader::State. Why is this here?

[hal.canary, 2016-08-02 21:00:57]

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:627: static std::unique_ptr<SkStreamAsset>
create_pattern_fill_content(
On 2016/08/02 20:59:12, tomhudson wrote:
> Tangent: am I suppose to conclude something from your choice to use
> std::unique_ptr<> instead of sk_sp<>?

class SK_API SkStream : public SkNoncopyable ...

[hal.canary, 2016-08-02 21:25:01]

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFCanon.cpp#...
src/pdf/SkPDFCanon.cpp:25: fGraphicStateRecords.foreach ([](WrapGS w) {
w.fPtr->unref(); });
On 2016/08/02 20:59:12, tomhudson wrote:
> Nit: TODO(halcanary) replace with sk_sp<> so we don't need the explicit
> teardown?

Done.

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFCanon.h#ne...
src/pdf/SkPDFCanon.h:51: sk_sp<SkPDFObject> findFunctionShader(const
SkPDFShader::State&) const;
On 2016/08/02 20:59:12, tomhudson wrote:
> Just so that I understand: these functions are named "find*" but they're
> returning a smart pointer so that a caller *can* take ownership if it needs
to?

yes.

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:627: static std::unique_ptr<SkStreamAsset>
create_pattern_fill_content(
On 2016/08/02 21:00:57, Hal Canary wrote:
> On 2016/08/02 20:59:12, tomhudson wrote:
> > Tangent: am I suppose to conclude something from your choice to use
> > std::unique_ptr<> instead of sk_sp<>?
> 
> class SK_API SkStream : public SkNoncopyable ...

More detail:  it never makes sense to share a stream since it has a current
position.  std:unique_ptr is the new standard way of handling them.

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

https://codereview.chromium.org/2193973002/diff/40001/src/pdf/SkPDFShader.h#n...
src/pdf/SkPDFShader.h:52: class State {
On 2016/08/02 20:59:12, tomhudson wrote:
> This goes from hidden to public and mostly unencapsulated?
> Except there's no part of the API uses SkPDFShader::State. Why is this here?

SkPDFCanon now uses this directly, rather than via pointers.

[tomhudson, 2016-08-03 16:57:26]

lgtm

[hal.canary, 2016-08-03 17:42:35]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-08-03 17:42:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 18:16:59]

Description was changed from

==========
SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2193973002
==========

to

==========
SkPDF: PDFShader code modernized.

Motivation:  reduce code complexity.

SkCanon stores SkPDFShader::State next to SkDFObject, not inside.

many places use sk_sp<T> rather than T* to represent ownership.

SkPDFShader::State no longer holds bitmap.

SkPDFShader::State gets move constructor, no longer heap-allocated.

Classes removed:
  SkPDFFunctionShader
  SkPDFAlphaFunctionShader
  SkPDFImageShader

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2193973002

Committed:
https://skia.googlesource.com/skia/+/dabd4f0b799318cb6e90b69ae1ec0ed0b6d32f60
==========

[commit-bot: I haz the power, 2016-08-03 18:17:00]

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