Move SkTypeface to sk_sp.

Move SkTypeface to sk_sp.

Committed: https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1933393002

Committed: https://skia.googlesource.com/skia/+/13b9c95295f4c5732e34574789e721a6bc08f7b4

[bungeman-skia, 2016-04-29 19:25:06]

Description was changed from

==========
Move SkTypeface to sk_sp.
==========

to

==========
Move SkTypeface to sk_sp.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bungeman-skia, 2016-04-29 19:25:26]

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

[commit-bot: I haz the power, 2016-04-29 19:25:36]

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

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

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

[commit-bot: I haz the power, 2016-04-29 19:29:08]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[bungeman-skia, 2016-04-29 19:41:44]

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

[commit-bot: I haz the power, 2016-04-29 19:41:54]

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

[bungeman-skia, 2016-04-29 19:53:24]

bungeman@google.com changed reviewers:
+ fmalita@chromium.org, reed@google.com, tomhudson@google.com

[bungeman-skia, 2016-04-29 19:53:25]

I'm giving up for the moment on writing an efficient smart pointer system for
c++, because I've now written two of them and don't like any of the options.
What's here seems fine for now.

[commit-bot: I haz the power, 2016-04-29 20:11:52]

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

[commit-bot: I haz the power, 2016-04-29 20:11:53]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2016-04-29 21:10:36]

Disturbed by how often we're using sk_ref_sp(), since on the Chrome side that
usually means we're doing something halfway between raw pointers and smart
pointers.

https://codereview.chromium.org/1933393002/diff/20001/gm/fontmgr.cpp
File gm/fontmgr.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/gm/fontmgr.cpp#newcode277
gm/fontmgr.cpp:277:
paint.setTypeface(sk_sp<SkTypeface>(set->createTypeface(j)));
ditto

https://codereview.chromium.org/1933393002/diff/20001/gm/fontscalerdistortabl...
File gm/fontscalerdistortable.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/gm/fontscalerdistortabl...
gm/fontscalerdistortable.cpp:63: paint.setTypeface(typeface);
std::move(), if we're going to get rid of setTypeface()-by-pointer and have to
keep the temporary local here?

https://codereview.chromium.org/1933393002/diff/20001/gm/textblobrandomfont.cpp
File gm/textblobrandomfont.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/gm/textblobrandomfont.c...
gm/textblobrandomfont.cpp:48: paint.setTypeface(random);
Can inline sk_make_sp<SkRandomTypeface>(orig, paint, false) and not have the
local random?

(Like you did 30 lines later!)

https://codereview.chromium.org/1933393002/diff/20001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1933393002/diff/20001/include/core/SkTypeface...
include/core/SkTypeface.h:96: *  Returns a ref() to the default typeface. The
caller must call unref()
If we don't update these comments now, we'll need to update them when we get rid
of the LEGACY.

https://codereview.chromium.org/1933393002/diff/20001/include/ports/SkFontCon...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/1933393002/diff/20001/include/ports/SkFontCon...
include/ports/SkFontConfigInterface.h:104: virtual SkTypeface*
createTypeface(const FontIdentity& identity) {
We can wait until a future patch to make this return sk_sp<>

https://codereview.chromium.org/1933393002/diff/20001/src/animator/SkDrawPain...
File src/animator/SkDrawPaint.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/src/animator/SkDrawPain...
src/animator/SkDrawPaint.cpp:262:
paint->setTypeface(sk_ref_sp(typeface->getTypeface()));
Let's have getTypeface() return sk_sp<> so we don't have to release() it in
SkPaintPart and sk_ref_sp() it here; that's kinda nasty.

https://codereview.chromium.org/1933393002/diff/20001/src/core/SkFont.cpp
File src/core/SkFont.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/src/core/SkFont.cpp#new...
src/core/SkFont.cpp:39: return sk_sp<SkFont>(new SkFont(std::move(face), size,
scaleX, skewX, mt, flags));
sk_make_sp<SkFont>(std::move(face), ...)

https://codereview.chromium.org/1933393002/diff/20001/tests/FontHostStreamTes...
File tests/FontHostStreamTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/tests/FontHostStreamTes...
tests/FontHostStreamTest.cpp:94: origTypeface = defaultTypeface.get();
How about SkTypeface::MakeDefault().get(), and get rid of the local?

https://codereview.chromium.org/1933393002/diff/20001/tests/SerializationTest...
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/tests/SerializationTest...
tests/SerializationTest.cpp:364: serialize_and_compare_typeface(typeface, "A!",
reporter);
std::move()

[bungeman-skia, 2016-04-29 22:03:24]

With this change there are now 163 instances of sk_ref_sp, this add 12.

https://codereview.chromium.org/1933393002/diff/20001/gm/fontmgr.cpp
File gm/fontmgr.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/gm/fontmgr.cpp#newcode277
gm/fontmgr.cpp:277:
paint.setTypeface(sk_sp<SkTypeface>(set->createTypeface(j)));
On 2016/04/29 21:10:36, tomhudson wrote:
> ditto

SkFontMgr is not being updated in this CL.

https://codereview.chromium.org/1933393002/diff/20001/gm/fontscalerdistortabl...
File gm/fontscalerdistortable.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/gm/fontscalerdistortabl...
gm/fontscalerdistortable.cpp:63: paint.setTypeface(typeface);
On 2016/04/29 21:10:36, tomhudson wrote:
> std::move(), if we're going to get rid of setTypeface()-by-pointer and have to
> keep the temporary local here?

Missed that one. Done.

https://codereview.chromium.org/1933393002/diff/20001/gm/textblobrandomfont.cpp
File gm/textblobrandomfont.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/gm/textblobrandomfont.c...
gm/textblobrandomfont.cpp:48: paint.setTypeface(random);
On 2016/04/29 21:10:36, tomhudson wrote:
> Can inline sk_make_sp<SkRandomTypeface>(orig, paint, false) and not have the
> local random?
> 
> (Like you did 30 lines later!)

Done.

https://codereview.chromium.org/1933393002/diff/20001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1933393002/diff/20001/include/core/SkTypeface...
include/core/SkTypeface.h:96: *  Returns a ref() to the default typeface. The
caller must call unref()
On 2016/04/29 21:10:36, tomhudson wrote:
> If we don't update these comments now, we'll need to update them when we get
rid
> of the LEGACY.

Yeah, these static methods are legacy as far as I'm concerned. Updated the
comments anyway.

https://codereview.chromium.org/1933393002/diff/20001/include/ports/SkFontCon...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/1933393002/diff/20001/include/ports/SkFontCon...
include/ports/SkFontConfigInterface.h:104: virtual SkTypeface*
createTypeface(const FontIdentity& identity) {
On 2016/04/29 21:10:36, tomhudson wrote:
> We can wait until a future patch to make this return sk_sp<>

Acknowledged.

https://codereview.chromium.org/1933393002/diff/20001/src/animator/SkDrawPain...
File src/animator/SkDrawPaint.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/src/animator/SkDrawPain...
src/animator/SkDrawPaint.cpp:262:
paint->setTypeface(sk_ref_sp(typeface->getTypeface()));
On 2016/04/29 21:10:36, tomhudson wrote:
> Let's have getTypeface() return sk_sp<> so we don't have to release() it in
> SkPaintPart and sk_ref_sp() it here; that's kinda nasty.

Done.

https://codereview.chromium.org/1933393002/diff/20001/src/core/SkFont.cpp
File src/core/SkFont.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/src/core/SkFont.cpp#new...
src/core/SkFont.cpp:39: return sk_sp<SkFont>(new SkFont(std::move(face), size,
scaleX, skewX, mt, flags));
On 2016/04/29 21:10:36, tomhudson wrote:
> sk_make_sp<SkFont>(std::move(face), ...)

Doesn't work because SkFont's constructor is private. sk_make_sp isn't generally
useful since it doesn't work for classes with static factories and all private
constructors (as all good intrusively reference counted objects should).

https://codereview.chromium.org/1933393002/diff/20001/tests/FontHostStreamTes...
File tests/FontHostStreamTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/tests/FontHostStreamTes...
tests/FontHostStreamTest.cpp:94: origTypeface = defaultTypeface.get();
On 2016/04/29 21:10:36, tomhudson wrote:
> How about SkTypeface::MakeDefault().get(), and get rid of the local?

Done.

https://codereview.chromium.org/1933393002/diff/20001/tests/SerializationTest...
File tests/SerializationTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/20001/tests/SerializationTest...
tests/SerializationTest.cpp:364: serialize_and_compare_typeface(typeface, "A!",
reporter);
On 2016/04/29 21:10:36, tomhudson wrote:
> std::move()

Done.

[reed1, 2016-05-01 18:53:52]

https://codereview.chromium.org/1933393002/diff/40001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/1933393002/diff/40001/include/core/SkRefCnt.h...
include/core/SkRefCnt.h:333: skstd::add_lvalue_reference_t<T> operator*() const
{
Is this change a necessary part of the "typeface" CL, or can it land separately?

https://codereview.chromium.org/1933393002/diff/40001/include/ports/SkFontCon...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/1933393002/diff/40001/include/ports/SkFontCon...
include/ports/SkFontConfigInterface.h:104: virtual SkTypeface*
createTypeface(const FontIdentity& identity) {
Future plan to upgrade this API?

[tomhudson, 2016-05-02 12:26:08]

Might be worth landing as-is, but Mike's right that the job's not done until
this is fixed.

https://codereview.chromium.org/1933393002/diff/40001/include/ports/SkFontCon...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/1933393002/diff/40001/include/ports/SkFontCon...
include/ports/SkFontConfigInterface.h:104: virtual SkTypeface*
createTypeface(const FontIdentity& identity) {
On 2016/05/01 18:53:52, reed1 wrote:
> Future plan to upgrade this API?

D'oh, yes, sloppy of me to miss this.

[f(malita), 2016-05-02 13:46:36]

lgtm w/ nits, modulo other reviewers.

https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp
File gm/colortype.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp#newcode30
gm/colortype.cpp:30: auto orig =
sk_tool_utils::create_portable_typeface("serif", SkTypeface::kBold);
Nit: Chrome disallows auto for smart ptrs, and I think we're trying to stick to
the same rule in Skia these days.

https://codereview.chromium.org/1933393002/diff/40001/gm/colortypexfermode.cpp
File gm/colortypexfermode.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/colortypexfermode.cp...
gm/colortypexfermode.cpp:37: auto orig =
sk_tool_utils::create_portable_typeface("serif", SkTypeface::kBold);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/gm/pdf_never_embed.cpp
File gm/pdf_never_embed.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/pdf_never_embed.cpp#...
gm/pdf_never_embed.cpp:35: p.setTypeface(typeface);
Nit: std::move(typeface)

https://codereview.chromium.org/1933393002/diff/40001/gm/textblobmixedsizes.cpp
File gm/textblobmixedsizes.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/textblobmixedsizes.c...
gm/textblobmixedsizes.cpp:38: paint.setTypeface(typeface);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:188: auto typeface = SkTypeface::MakeFromFontData(data);
Nit: avoid auto for sk_sp?

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkLua.cpp
File src/utils/SkLua.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkLua.cpp#new...
src/utils/SkLua.cpp:2053: (SkTypeface::Style)style);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
File src/utils/SkWhitelistTypefaces.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:36: auto foundFace =
SkTypeface::MakeFromName(fontName, style);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:194: auto typeface =
SkTypeface::MakeFromFontData(data);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:209: auto tf =
SkTypeface::MakeFromName(fontName, SkTypeface::kNormal);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:264: auto tf =
SkTypeface::MakeFromName(fontName, SkTypeface::kNormal);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostStreamTes...
File tests/FontHostStreamTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostStreamTes...
tests/FontHostStreamTest.cpp:94: auto streamTypeface =
SkTypeface::MakeFromStream(fontData.release());
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostTest.cpp
File tests/FontHostTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostTest.cpp#...
tests/FontHostTest.cpp:166: paint.setTypeface(typeface);
Nit: std::move(typeface)

https://codereview.chromium.org/1933393002/diff/40001/tests/TextBlobTest.cpp
File tests/TextBlobTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/TextBlobTest.cpp#...
tests/TextBlobTest.cpp:186: font.setTypeface(typeface);
Nit: ditto

https://codereview.chromium.org/1933393002/diff/40001/tests/TypefaceTest.cpp
File tests/TypefaceTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/TypefaceTest.cpp#...
tests/TypefaceTest.cpp:16: auto
t2(SkTypeface::MakeDefault(SkTypeface::kNormal));
Nit: avoid auto for sk_sp

https://codereview.chromium.org/1933393002/diff/40001/tests/TypefaceTest.cpp#...
tests/TypefaceTest.cpp:25: auto t3(SkTypeface::MakeFromName("non-existent-font",
SkTypeface::kNormal));
Nit: ditto

[bungeman-skia, 2016-05-02 20:06:30]

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

[commit-bot: I haz the power, 2016-05-02 20:06:33]

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

[bungeman-skia, 2016-05-02 20:07:23]

The CQ bit was unchecked by bungeman@google.com

[bungeman-skia, 2016-05-02 20:15:50]

Description was changed from

==========
Move SkTypeface to sk_sp.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Move SkTypeface to sk_sp.
==========

[bungeman-skia, 2016-05-02 20:24:55]

https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp
File gm/colortype.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp#newcode30
gm/colortype.cpp:30: auto orig =
sk_tool_utils::create_portable_typeface("serif", SkTypeface::kBold);
On 2016/05/02 13:46:35, f(malita) wrote:
> Nit: Chrome disallows auto for smart ptrs, and I think we're trying to stick
to
> the same rule in Skia these days.

These were all in the CL I started this with... will remove since I don't like
them either. Done.

https://codereview.chromium.org/1933393002/diff/40001/gm/colortypexfermode.cpp
File gm/colortypexfermode.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/colortypexfermode.cp...
gm/colortypexfermode.cpp:37: auto orig =
sk_tool_utils::create_portable_typeface("serif", SkTypeface::kBold);
On 2016/05/02 13:46:35, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/gm/pdf_never_embed.cpp
File gm/pdf_never_embed.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/pdf_never_embed.cpp#...
gm/pdf_never_embed.cpp:35: p.setTypeface(typeface);
On 2016/05/02 13:46:35, f(malita) wrote:
> Nit: std::move(typeface)

Done.

https://codereview.chromium.org/1933393002/diff/40001/gm/textblobmixedsizes.cpp
File gm/textblobmixedsizes.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/textblobmixedsizes.c...
gm/textblobmixedsizes.cpp:38: paint.setTypeface(typeface);
On 2016/05/02 13:46:35, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/1933393002/diff/40001/include/core/SkRefCnt.h...
include/core/SkRefCnt.h:333: skstd::add_lvalue_reference_t<T> operator*() const
{
On 2016/05/01 18:53:52, reed1 wrote:
> Is this change a necessary part of the "typeface" CL, or can it land
separately?

No, I'll stick this in another CL, it got mixed up in here. This is what the stl
does (after some debate). The difference is that it works with void, so you can
create the type sk_sp<void>, which isn't particularly useful, but avoids certain
issues when writing generic template code.

https://codereview.chromium.org/1933393002/diff/40001/include/ports/SkFontCon...
File include/ports/SkFontConfigInterface.h (right):

https://codereview.chromium.org/1933393002/diff/40001/include/ports/SkFontCon...
include/ports/SkFontConfigInterface.h:104: virtual SkTypeface*
createTypeface(const FontIdentity& identity) {
On 2016/05/02 12:26:08, tomhudson wrote:
> On 2016/05/01 18:53:52, reed1 wrote:
> > Future plan to upgrade this API?
> 
> D'oh, yes, sloppy of me to miss this.

Done.

https://codereview.chromium.org/1933393002/diff/40001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:188: auto typeface = SkTypeface::MakeFromFontData(data);
On 2016/05/02 13:46:35, f(malita) wrote:
> Nit: avoid auto for sk_sp?

Done.

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkLua.cpp
File src/utils/SkLua.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkLua.cpp#new...
src/utils/SkLua.cpp:2053: (SkTypeface::Style)style);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
File src/utils/SkWhitelistTypefaces.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:36: auto foundFace =
SkTypeface::MakeFromName(fontName, style);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:194: auto typeface =
SkTypeface::MakeFromFontData(data);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:209: auto tf =
SkTypeface::MakeFromName(fontName, SkTypeface::kNormal);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/src/utils/SkWhitelistTy...
src/utils/SkWhitelistTypefaces.cpp:264: auto tf =
SkTypeface::MakeFromName(fontName, SkTypeface::kNormal);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostStreamTes...
File tests/FontHostStreamTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostStreamTes...
tests/FontHostStreamTest.cpp:94: auto streamTypeface =
SkTypeface::MakeFromStream(fontData.release());
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostTest.cpp
File tests/FontHostTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/FontHostTest.cpp#...
tests/FontHostTest.cpp:166: paint.setTypeface(typeface);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: std::move(typeface)

Done.

https://codereview.chromium.org/1933393002/diff/40001/tests/TextBlobTest.cpp
File tests/TextBlobTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/TextBlobTest.cpp#...
tests/TextBlobTest.cpp:186: font.setTypeface(typeface);
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

https://codereview.chromium.org/1933393002/diff/40001/tests/TypefaceTest.cpp
File tests/TypefaceTest.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/tests/TypefaceTest.cpp#...
tests/TypefaceTest.cpp:16: auto
t2(SkTypeface::MakeDefault(SkTypeface::kNormal));
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: avoid auto for sk_sp

Done.

https://codereview.chromium.org/1933393002/diff/40001/tests/TypefaceTest.cpp#...
tests/TypefaceTest.cpp:25: auto t3(SkTypeface::MakeFromName("non-existent-font",
SkTypeface::kNormal));
On 2016/05/02 13:46:36, f(malita) wrote:
> Nit: ditto

Done.

[reed1, 2016-05-03 13:45:00]

https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp
File gm/colortype.cpp (right):

https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp#newcode30
gm/colortype.cpp:30: auto orig =
sk_tool_utils::create_portable_typeface("serif", SkTypeface::kBold);
On 2016/05/02 13:46:35, f(malita) wrote:
> Nit: Chrome disallows auto for smart ptrs, and I think we're trying to stick
to
> the same rule in Skia these days.

I didn't know that. I'd like to be caught up in that discussion.

[f(malita), 2016-05-03 14:02:28]

On 2016/05/03 13:45:00, reed1 wrote:
> https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp
> File gm/colortype.cpp (right):
> 
>
https://codereview.chromium.org/1933393002/diff/40001/gm/colortype.cpp#newcode30
> gm/colortype.cpp:30: auto orig =
> sk_tool_utils::create_portable_typeface("serif", SkTypeface::kBold);
> On 2016/05/02 13:46:35, f(malita) wrote:
> > Nit: Chrome disallows auto for smart ptrs, and I think we're trying to stick
> to
> > the same rule in Skia these days.
> 
> I didn't know that. I'd like to be caught up in that discussion.

The relevant thread is here:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0

The argument boils down to auto hurting readability when used with smart
pointers, and I tend to agree:

  auto foo = bar.giveMeFoo();
  baz.setFoo(foo);

Is this missing a std::move() or dealing with raw ptrs?

http://i.imgur.com/X2dNIZ5.jpg

[bungeman-skia, 2016-05-10 17:50:48]

reed, you ok with this?

[bungeman-skia, 2016-05-10 18:53:33]

Description was changed from

==========
Move SkTypeface to sk_sp.
==========

to

==========
Move SkTypeface to sk_sp.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bungeman-skia, 2016-05-10 18:53:57]

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

[commit-bot: I haz the power, 2016-05-10 18:54:02]

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

[commit-bot: I haz the power, 2016-05-10 19:16:12]

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

[commit-bot: I haz the power, 2016-05-10 19:16:14]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-05-11 18:59:14]

lgtm

[bungeman-skia, 2016-05-11 19:03:00]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2016-05-11 19:03:01]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1933393002/#ps80001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-05-11 19:03:12]

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

[bungeman-skia, 2016-05-11 19:03:18]

Description was changed from

==========
Move SkTypeface to sk_sp.
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Move SkTypeface to sk_sp.
==========

[commit-bot: I haz the power, 2016-05-11 19:38:22]

Description was changed from

==========
Move SkTypeface to sk_sp.
==========

to

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
==========

[commit-bot: I haz the power, 2016-05-11 19:38:23]

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

[scroggo, 2016-05-12 13:22:05]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1974783002/ by scroggo@google.com.

The reason for reverting is: fontmgr_iterAndroid failing to draw emoji. E.g.
https://gold.skia.org/search2?blame=6296da736fbf40aae881650c239420f64e576c3f&....

[bungeman-skia, 2016-05-12 15:31:12]

Description was changed from

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
==========

to

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
==========

[bungeman-skia, 2016-05-12 16:46:32]

Description was changed from

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
==========

to

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bungeman-skia, 2016-05-12 16:47:41]

https://codereview.chromium.org/1933393002/diff/80001/src/ports/SkFontMgr_and...
File src/ports/SkFontMgr_android.cpp (left):

https://codereview.chromium.org/1933393002/diff/80001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:390: if (glyphID != 0) {
How did this sneak in here? Without this test this method is just 'return
nullptr;'!

[bungeman-skia, 2016-05-12 16:48:02]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2016-05-12 16:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/1933393002/#ps100001
(title: "Restore deleted Android code.")

[commit-bot: I haz the power, 2016-05-12 16:48:11]

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

[commit-bot: I haz the power, 2016-05-12 17:09:35]

Description was changed from

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Move SkTypeface to sk_sp.

Committed:
https://skia.googlesource.com/skia/+/6296da736fbf40aae881650c239420f64e576c3f
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/13b9c95295f4c5732e34574789e721a6bc08f7b4
==========

[commit-bot: I haz the power, 2016-05-12 17:09:36]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/13b9c95295f4c5732e34574789e721a6bc08f7b4