SkPDF: use sk_make_sp<T> when it makes sense.

SkPDF: use sk_make_sp<T> when it makes sense.
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1773033002

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

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

Description was changed from

==========
SkPDF: use sk_make_sp<T> when it makes sense.
==========

to

==========
SkPDF: use sk_make_sp<T> when it makes sense.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[hal.canary, 2016-03-08 01:25:34]

Description was changed from

==========
SkPDF: use sk_make_sp<T> when it makes sense.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPDF: use sk_make_sp<T> when it makes sense.
==========

[hal.canary, 2016-03-08 01:28:53]

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

[commit-bot: I haz the power, 2016-03-08 01:29:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1773033002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1773033002/1

[commit-bot: I haz the power, 2016-03-08 01:30:30]

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

[commit-bot: I haz the power, 2016-03-08 01:30:30]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[hal.canary, 2016-03-08 01:36:15]

Description was changed from

==========
SkPDF: use sk_make_sp<T> when it makes sense.
==========

to

==========
SkPDF: use sk_make_sp<T> when it makes sense.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[hal.canary, 2016-03-08 01:36:26]

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

[commit-bot: I haz the power, 2016-03-08 01:36:27]

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

[hal.canary, 2016-03-08 01:36:37]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-03-08 01:49:50]

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

[commit-bot: I haz the power, 2016-03-08 01:49:51]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2016-03-08 01:52:59]

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

[hal.canary, 2016-03-08 01:52:59]

ptal

[bungeman-skia, 2016-03-08 02:20:36]

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

https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:811: auto pdfFunctionShader =
sk_make_sp<SkPDFFunctionShader>(autoState->detach());
This appears to be the reason for trying to make the constructor public.

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

https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFShader.h#n...
src/pdf/SkPDFShader.h:57: SkPDFFunctionShader(SkPDFShader::State*);
eh, I don't think I'd make this public just because you want to be able to use
sk_make_sp on it.

[reed1, 2016-03-08 03:00:03]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-03-08 03:00:04]

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

https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFDevice.cpp...
src/pdf/SkPDFDevice.cpp:899: annotation->insertObject("Rect", rect.release());
The PDF code seems to require calling .release() in a lot of places. I wonder if
a different pattern or parameter type on inserObject() would be cleaner.

[hal.canary, 2016-03-08 03:08:51]

On 2016/03/08 03:00:04, reed1 wrote:
> https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFDevice.cpp
> File src/pdf/SkPDFDevice.cpp (right):
> 
>
https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFDevice.cpp...
> src/pdf/SkPDFDevice.cpp:899: annotation->insertObject("Rect", rect.release());
> The PDF code seems to require calling .release() in a lot of places. I wonder
if
> a different pattern or parameter type on inserObject() would be cleaner.

That's the next CL, already written.

[bungeman-skia, 2016-03-08 03:27:21]

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

https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFShader.h#n...
src/pdf/SkPDFShader.h:57: SkPDFFunctionShader(SkPDFShader::State*);
On 2016/03/08 02:20:36, bungeman1 wrote:
> eh, I don't think I'd make this public just because you want to be able to use
> sk_make_sp on it.

I find this interesting and std::shared_pointer has a similar issue. It's
unfortunate, but the general consensus seems to be to just leave the constructor
private and use 'new' or to do something like

class Foo {
private:
    struct this_constructor_is_actually_private {};
public:
    Foo(this_constructor_is_actually_private&) {};
    sk_sp<Foo> Make() {
        return sk_make_sp<Foo>(this_constructor_is_actually_private());
    }
};

The idea being that now sk_make_sp can call the Foo constructor (it's public),
but only if it is called with an object of type
this_constructor_is_actually_private which only a member of Foo can create.
Honestly, it just seems cleaner to use 'new' since then the interface is
cluttered with public looking constructors which are actually uncallable by
clients.

[hal.canary, 2016-03-08 11:58:48]

let's land this at patchset 2.

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

https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFShader.cpp...
src/pdf/SkPDFShader.cpp:811: auto pdfFunctionShader =
sk_make_sp<SkPDFFunctionShader>(autoState->detach());
On 2016/03/08 02:20:36, bungeman1 wrote:
> This appears to be the reason for trying to make the constructor public. 

yep.  changed back.

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

https://codereview.chromium.org/1773033002/diff/20001/src/pdf/SkPDFShader.h#n...
src/pdf/SkPDFShader.h:57: SkPDFFunctionShader(SkPDFShader::State*);
On 2016/03/08 03:27:21, bungeman1 wrote:
> On 2016/03/08 02:20:36, bungeman1 wrote:
> > eh, I don't think I'd make this public just because you want to be able to
use
> > sk_make_sp on it.
> 
> I find this interesting and std::shared_pointer has a similar issue. It's
> unfortunate, but the general consensus seems to be to just leave the
constructor
> private and use 'new' or to do something like
> 
> class Foo {
> private:
>     struct this_constructor_is_actually_private {};
> public:
>     Foo(this_constructor_is_actually_private&) {};
>     sk_sp<Foo> Make() {
>         return sk_make_sp<Foo>(this_constructor_is_actually_private());
>     }
> };
> 
> The idea being that now sk_make_sp can call the Foo constructor (it's public),
> but only if it is called with an object of type
> this_constructor_is_actually_private which only a member of Foo can create.
> Honestly, it just seems cleaner to use 'new' since then the interface is
> cluttered with public looking constructors which are actually uncallable by
> clients.

Done.

[bungeman-skia, 2016-03-08 15:29:29]

lgtm

[hal.canary, 2016-03-08 15:36:13]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-03-08 15:36:21]

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

[commit-bot: I haz the power, 2016-03-08 15:37:54]

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

[commit-bot: I haz the power, 2016-03-08 15:37:55]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[hal.canary, 2016-03-08 16:14:50]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2016-03-08 16:15:00]

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

[commit-bot: I haz the power, 2016-03-08 16:32:15]

Description was changed from

==========
SkPDF: use sk_make_sp<T> when it makes sense.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
SkPDF: use sk_make_sp<T> when it makes sense.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

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

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

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