Fix issues related to resolving fonts based on name.

Fix issues related to resolving fonts based on name.

1) non-system font files are not added to the cache.
2) We cache the default fonts for quick lookup.

Committed: https://code.google.com/p/skia/source/detail?r=9441

[reed1, 2013-06-05 13:23:57]

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

https://codereview.chromium.org/16439004/diff/1/src/core/SkTypeface.cpp#newco...
src/core/SkTypeface.cpp:45: static SkTypeface*
gDefaultTypefaces[FONT_STYLE_COUNT] = {0};
nit: don't need {0} as it will automatically be zeroed since its static.

https://codereview.chromium.org/16439004/diff/1/src/core/SkTypeface.cpp#newco...
src/core/SkTypeface.cpp:46: SkASSERT(FONT_STYLE_COUNT > style);
nit: I think I always see this compare the other way: value < max

possible warning in some compilers, if the enum gets unsigned. I suggest:

    (unsigned)style < FONT_STYLE_COUNT

where FONT_STYLE_COUNT is also unsigned. This also gives you a free check in
case style is negative.

Also, I wonder if we shouldn't also explicitly mask of any stray bits, just so
we never crash in release mode:

style &= kBoldItalic

https://codereview.chromium.org/16439004/diff/1/src/ports/SkFontHost_fontconf...
File src/ports/SkFontHost_fontconfig.cpp (right):

https://codereview.chromium.org/16439004/diff/1/src/ports/SkFontHost_fontconf...
src/ports/SkFontHost_fontconfig.cpp:110: face =
SkTypefaceCache::FindByProcAndRef(find_proc_by_id, &identity.fID);
this may be correct, but is it strictly needed for the short/mid-term? I thought
the null-check in SkTypeface.cpp was sufficient.

[djsollen, 2013-06-05 13:51:59]

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

https://codereview.chromium.org/16439004/diff/1/src/core/SkTypeface.cpp#newco...
src/core/SkTypeface.cpp:45: static SkTypeface*
gDefaultTypefaces[FONT_STYLE_COUNT] = {0};
On 2013/06/05 13:23:57, reed1 wrote:
> nit: don't need {0} as it will automatically be zeroed since its static.

Done.

https://codereview.chromium.org/16439004/diff/1/src/core/SkTypeface.cpp#newco...
src/core/SkTypeface.cpp:46: SkASSERT(FONT_STYLE_COUNT > style);
On 2013/06/05 13:23:57, reed1 wrote:
> nit: I think I always see this compare the other way: value < max
> 
> possible warning in some compilers, if the enum gets unsigned. I suggest:
> 
>     (unsigned)style < FONT_STYLE_COUNT
> 
> where FONT_STYLE_COUNT is also unsigned. This also gives you a free check in
> case style is negative.
> 
> Also, I wonder if we shouldn't also explicitly mask of any stray bits, just so
> we never crash in release mode:
> 
> style &= kBoldItalic

Done.

https://codereview.chromium.org/16439004/diff/1/src/ports/SkFontHost_fontconf...
File src/ports/SkFontHost_fontconfig.cpp (right):

https://codereview.chromium.org/16439004/diff/1/src/ports/SkFontHost_fontconf...
src/ports/SkFontHost_fontconfig.cpp:110: face =
SkTypefaceCache::FindByProcAndRef(find_proc_by_id, &identity.fID);
On 2013/06/05 13:23:57, reed1 wrote:
> this may be correct, but is it strictly needed for the short/mid-term? I
thought
> the null-check in SkTypeface.cpp was sufficient.

No this is still needed. If an app asks for a font that we don't have we return
the default, but instead of using the default in the cache we create a new one. 
The NULL case was just a optimization of the most common use case.

[djsollen, 2013-06-05 14:09:09]

updated to defer the extra cache lookup to another CL.

[djsollen, 2013-06-05 14:20:31]

Committed patchset #3 manually as r9441.