Add SkTypeface* parameter to SkScalerContext (and its callers)

Add SkTypeface* parameter to SkScalerContext (and its callers)
Use SkTypeface to create scalercontext instead of SkFontHost
Committed: https://code.google.com/p/skia/source/detail?r=8223

[reed1, 2013-03-14 22:09:06]

OK, this one I'm serious.

[bungeman-skia, 2013-03-15 16:12:44]

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkFont...
File trunk/include/core/SkFontHost.h (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkFont...
trunk/include/core/SkFontHost.h:129: static SkTypeface*
NextLogicalTypeface(SkFontID currFontID, SkFontID origFontID);
I know this is going away, but it seems like we should document that this
returns a ref, it isn't obvious from the name.

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkType...
File trunk/include/core/SkTypeface.h (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkType...
trunk/include/core/SkTypeface.h:199: SkScalerContext* createScalerContext(const
SkDescriptor* desc);
I agree about not making this one public, for sure. Exposing SkScalerContext and
SkDescriptor!?

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkType...
trunk/include/core/SkTypeface.h:210: virtual SkScalerContext*
onCreateScalerContext(const SkDescriptor*) const = 0;
It seems implementations can return NULL, should we document that and state that
if they do they'll end up with an Empty?

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
File trunk/src/core/SkScalerContext.cpp (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:82: , fTypeface(SkRef(typeface))
fTypeface is a pointer, not an SkAutoTUnref, so the ref should be done in the
body. I'd actually prefer fTypeface to be an SkAutoTUnref<SkTypeface> and leave
this line alone.

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:118: fTypeface->unref();
Remove if fTypeface is changed to an SkAutoTUnref.

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:125: SkTypeface* newFace =
SkFontHost::NextLogicalTypeface(rec.fFontID, rec.fOrigFontID);
NextLogicalTypeface returns a ref?

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:784: c = SkNEW_ARGS(SkScalerContext_Empty,
(this, desc));
Since all Empty do (almost) the same thing, should we have just one? It seems it
would be nice to avoid having to new something up when that's probably the only
reason the above call would have returned NULL. (I suppose in the case of
FreeType the font data could just be bad and creating the face fail.)

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
File trunk/src/core/SkScalerContext.h (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.h:218: SkTypeface*     fTypeface;
SkAutoTUnref<SkTypeface> would be nice, since it documents that this ref has the
same lifetime as the ScalerContext itself.

https://codereview.chromium.org/12706010/diff/21002/trunk/src/ports/SkFontHos...
File trunk/src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/ports/SkFontHos...
trunk/src/ports/SkFontHost_win.cpp:557: const SkDescriptor* desc)
A tab??

https://codereview.chromium.org/12706010/diff/21002/trunk/src/ports/SkFontHos...
File trunk/src/ports/SkFontHost_win_dw.cpp (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/ports/SkFontHos...
trunk/src/ports/SkFontHost_win_dw.cpp:715: const SkDescriptor* desc)
More tabs!

https://codereview.chromium.org/12706010/diff/21002/trunk/src/views/win/SkOSW...
File trunk/src/views/win/SkOSWindow_win.cpp (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/views/win/SkOSW...
trunk/src/views/win/SkOSWindow_win.cpp:107: static const struct {
Not sure what this has to do with this change, but I like it except for the
tabs. (Full disclosure: I like tabs, but Skia doesn't.)

[reed1, 2013-03-15 17:46:44]

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkFont...
File trunk/include/core/SkFontHost.h (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkFont...
trunk/include/core/SkFontHost.h:129: static SkTypeface*
NextLogicalTypeface(SkFontID currFontID, SkFontID origFontID);
On 2013/03/15 16:12:44, bungeman1 wrote:
> I know this is going away, but it seems like we should document that this
> returns a ref, it isn't obvious from the name.

Done.

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkType...
File trunk/include/core/SkTypeface.h (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/include/core/SkType...
trunk/include/core/SkTypeface.h:210: virtual SkScalerContext*
onCreateScalerContext(const SkDescriptor*) const = 0;
On 2013/03/15 16:12:44, bungeman1 wrote:
> It seems implementations can return NULL, should we document that and state
that
> if they do they'll end up with an Empty?

Done.

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
File trunk/src/core/SkScalerContext.cpp (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:82: , fTypeface(SkRef(typeface))
On 2013/03/15 16:12:44, bungeman1 wrote:
> fTypeface is a pointer, not an SkAutoTUnref, so the ref should be done in the
> body. I'd actually prefer fTypeface to be an SkAutoTUnref<SkTypeface> and
leave
> this line alone.

Not sure I agree that pointers should not be initialized this way, but I have
changed fTypeface to be an auto-guy

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:118: fTypeface->unref();
On 2013/03/15 16:12:44, bungeman1 wrote:
> Remove if fTypeface is changed to an SkAutoTUnref.

Done.

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:125: SkTypeface* newFace =
SkFontHost::NextLogicalTypeface(rec.fFontID, rec.fOrigFontID);
On 2013/03/15 16:12:44, bungeman1 wrote:
> NextLogicalTypeface returns a ref?

rewrote dox

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.cpp:784: c = SkNEW_ARGS(SkScalerContext_Empty,
(this, desc));
On 2013/03/15 16:12:44, bungeman1 wrote:
> Since all Empty do (almost) the same thing, should we have just one? It seems
it
> would be nice to avoid having to new something up when that's probably the
only
> reason the above call would have returned NULL. (I suppose in the case of
> FreeType the font data could just be bad and creating the face fail.)

1. This case is incredibly rare
2. SkScalerContext is not reference counted

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
File trunk/src/core/SkScalerContext.h (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/core/SkScalerCo...
trunk/src/core/SkScalerContext.h:218: SkTypeface*     fTypeface;
On 2013/03/15 16:12:44, bungeman1 wrote:
> SkAutoTUnref<SkTypeface> would be nice, since it documents that this ref has
the
> same lifetime as the ScalerContext itself.

Done.

https://codereview.chromium.org/12706010/diff/21002/trunk/src/views/win/SkOSW...
File trunk/src/views/win/SkOSWindow_win.cpp (right):

https://codereview.chromium.org/12706010/diff/21002/trunk/src/views/win/SkOSW...
trunk/src/views/win/SkOSWindow_win.cpp:107: static const struct {
On 2013/03/15 16:12:44, bungeman1 wrote:
> Not sure what this has to do with this change, but I like it except for the
> tabs. (Full disclosure: I like tabs, but Skia doesn't.)

Accidentally picked up when I editing on my win machine. Removing from this CL
(need to remember to land it separately).

[bungeman-skia, 2013-03-15 18:17:02]

lgtm

[reed1, 2013-03-19 14:45:23]

Committed patchset #8 manually as r8223 (presubmit successful).