[SkSVGDevice] Full font family support

[SkSVGDevice] Full font family support

Use a family name iterator and list all names instead of just one
returned by SkTypeface::getFamilyName().

R=reed@google.com,mtklein@google.com

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

[f(malita), 2015-02-12 17:37:47]

https://codereview.chromium.org/923583002/diff/1/include/core/SkTArray.h
File include/core/SkTArray.h (right):

https://codereview.chromium.org/923583002/diff/1/include/core/SkTArray.h#newc...
include/core/SkTArray.h:367: int find(const T& elem) const {
Similar to SkTDArray's helpers.

[reed1, 2015-02-12 17:55:13]

using strings in SkTArray is fine, but may be slightly expensive, since resizing
the array calls a lot of constructors/destructors. Do you just add things to the
array?

https://codereview.chromium.org/923583002/diff/1/include/core/SkTArray.h
File include/core/SkTArray.h (right):

https://codereview.chromium.org/923583002/diff/1/include/core/SkTArray.h#newc...
include/core/SkTArray.h:373: return (int) (iter - fItemArray);
SkToInt()

[f(malita), 2015-02-12 18:00:54]

On 2015/02/12 17:55:13, reed1 wrote:
> using strings in SkTArray is fine, but may be slightly expensive, since
resizing
> the array calls a lot of constructors/destructors. Do you just add things to
the
> array?

Yes, just a quick way to detect/remove dupes. Empirically, it seems quite rare
to get more than 1-2 unique names.

I guess we could use SkTDArray + heap-allocated strings, but this whole business
seems a tad overkill. Do we have a canonical SkSet I could use instead?

[mtklein, 2015-02-12 18:02:37]

On 2015/02/12 18:00:54, f(malita) wrote:
> On 2015/02/12 17:55:13, reed1 wrote:
> > using strings in SkTArray is fine, but may be slightly expensive, since
> resizing
> > the array calls a lot of constructors/destructors. Do you just add things to
> the
> > array?
> 
> Yes, just a quick way to detect/remove dupes. Empirically, it seems quite rare
> to get more than 1-2 unique names.
> 
> I guess we could use SkTDArray + heap-allocated strings, but this whole
business
> seems a tad overkill. Do we have a canonical SkSet I could use instead?

I don't think we have a good one.  Given SkStrings are non-POD, I think for now
building on SkTArray is a good idea.

I do think we can build a good one on top of the SkTInlineHashTable I've got
drafted up here: https://codereview.chromium.org/912983002/

[f(malita), 2015-02-12 18:10:19]

https://codereview.chromium.org/923583002/diff/1/include/core/SkTArray.h
File include/core/SkTArray.h (right):

https://codereview.chromium.org/923583002/diff/1/include/core/SkTArray.h#newc...
include/core/SkTArray.h:373: return (int) (iter - fItemArray);
On 2015/02/12 17:55:13, reed1 wrote:
> SkToInt()

Done.

[reed1, 2015-02-12 19:18:35]

headers lgtm

[mtklein, 2015-02-12 21:14:11]

https://codereview.chromium.org/923583002/diff/20001/src/svg/SkSVGDevice.cpp
File src/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/923583002/diff/20001/src/svg/SkSVGDevice.cpp#...
src/svg/SkSVGDevice.cpp:468: SkAutoTUnref<const SkTypeface>
tface(paint.getTypeface() ?
Do we really need to be refing the typeface here?  Our need for it doesn't
outlive the function, right?

[f(malita), 2015-02-12 21:17:59]

https://codereview.chromium.org/923583002/diff/20001/src/svg/SkSVGDevice.cpp
File src/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/923583002/diff/20001/src/svg/SkSVGDevice.cpp#...
src/svg/SkSVGDevice.cpp:468: SkAutoTUnref<const SkTypeface>
tface(paint.getTypeface() ?
On 2015/02/12 21:14:10, mtklein wrote:
> Do we really need to be refing the typeface here?  Our need for it doesn't
> outlive the function, right?

The only reason to do that is to be able to use the same AutoTUnref in both
cases (note that SkTypeface::RefDefault() always refs). It's possible to
refactor to avoid the extra ref but I think it'll be less readable overall.

[mtklein, 2015-02-12 21:19:33]

On 2015/02/12 21:17:59, f(malita) wrote:
> https://codereview.chromium.org/923583002/diff/20001/src/svg/SkSVGDevice.cpp
> File src/svg/SkSVGDevice.cpp (right):
> 
>
https://codereview.chromium.org/923583002/diff/20001/src/svg/SkSVGDevice.cpp#...
> src/svg/SkSVGDevice.cpp:468: SkAutoTUnref<const SkTypeface>
> tface(paint.getTypeface() ?
> On 2015/02/12 21:14:10, mtklein wrote:
> > Do we really need to be refing the typeface here?  Our need for it doesn't
> > outlive the function, right?
> 
> The only reason to do that is to be able to use the same AutoTUnref in both
> cases (note that SkTypeface::RefDefault() always refs). It's possible to
> refactor to avoid the extra ref but I think it'll be less readable overall.

Oh, I see, GetDefautTypeface is not public.  It probably should be...

Anyway, mod that, lgtm.

[f(malita), 2015-02-13 03:10:56]

New patchsets have been uploaded after l-g-t-m from
reed@google.com,mtklein@google.com

[f(malita), 2015-02-13 03:21:24]

Updated to use the shiny SkTHashSet.

[mtklein, 2015-02-13 15:01:20]

lgtm

https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp
File src/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp#...
src/svg/SkSVGDevice.cpp:170: // This is a lame hash function, but we don't
really expect to see more than 1-2
At least, it is not without precedent:

http://www.i-programmer.info/news/98-languages/6758-the-reason-for-the-weird-...

https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp#...
src/svg/SkSVGDevice.cpp:483: familySet.add(familyString.fString);
Well, now I want SkTHashSet's add to return a bool....

[f(malita), 2015-02-13 15:09:09]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2015-02-13 15:09:48]

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

[f(malita), 2015-02-13 15:24:09]

https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp
File src/svg/SkSVGDevice.cpp (right):

https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp#...
src/svg/SkSVGDevice.cpp:483: familySet.add(familyString.fString);
On 2015/02/13 15:01:20, mtklein wrote:
> Well, now I want SkTHashSet's add to return a bool....

Ack.

Yeah, that would let us avoid a double hash lookup - but it's a pattern that
will hit the current temp-copy-on-repeated-add much harder.

[mtklein, 2015-02-13 15:27:33]

On 2015/02/13 15:24:09, f(malita) wrote:
> https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp
> File src/svg/SkSVGDevice.cpp (right):
> 
>
https://codereview.chromium.org/923583002/diff/80001/src/svg/SkSVGDevice.cpp#...
> src/svg/SkSVGDevice.cpp:483: familySet.add(familyString.fString);
> On 2015/02/13 15:01:20, mtklein wrote:
> > Well, now I want SkTHashSet's add to return a bool....
> 
> Ack.
> 
> Yeah, that would let us avoid a double hash lookup - but it's a pattern that
> will hit the current temp-copy-on-repeated-add much harder.

Yeah, unless we make it do a no-op in that case.  SkTHashTable might want
separate set() and add(), only varying by what they do with dupes.  Then
SkTHashSet::add() would use add(), not set().

[commit-bot: I haz the power, 2015-02-13 16:55:28]

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