Issue 384503002: WIP SkFontMgrAndroid

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+407 lines, -23 lines)			Patch
M	gyp/common_conditions.gypi	View		1 chunk	+1 line, -1 line	0 comments	Download
M	gyp/ports.gyp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	src/core/SkScalerContext.cpp	View	1 2 3 4	1 chunk	+3 lines, -16 lines	0 comments	Download
M	src/ports/SkFontConfigInterface_android.cpp	View	1 2 3 4	1 chunk	+0 lines, -5 lines	0 comments	Download
M	src/ports/SkFontConfigParser_android.cpp	View	1 2 3	2 chunks	+3 lines, -1 line	0 comments	Download
A	src/ports/SkFontMgr_android.cpp	View	1 2 3 4 5	1 chunk	+399 lines, -0 lines	22 comments	Download

Messages

Total messages: 2 (0 generated)

Expand Messages | Collapse Messages

bungeman-skia

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_android.cpp File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_android.cpp#newcode22 src/ports/SkFontMgr_android.cpp:22: // TODO: document isLocal in getFontDescriptor: We need to ...

6 years, 5 months ago (2014-07-21 15:48:01 UTC) #1

bungeman-skia

6 years, 5 months ago (2014-07-22 17:26:53 UTC) #2

Since I cannot collaborate directly on this CL, I have continued with
https://codereview.chromium.org/414483002/ .

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
File src/ports/SkFontMgr_android.cpp (right):

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:22: // TODO: document isLocal in
getFontDescriptor:
On 2014/07/21 15:48:00, bungeman1 wrote:
> We need to just change the name to 'shouldSerialize'. It really isn't about
who
> knows about the font, but whether the typeface thinks that it needs to be
> serialized for correct use in a pipe that cannot keep the SkTypeface pointer.

Acknowledged.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:30: bool find_name_and_attributes(SkStream*
stream, SkString* name,
On 2014/07/21 15:48:00, bungeman1 wrote:
> This is now gone, replaced by SkTypeface_FreeType::Scan (it also takes a ttc
> index now).

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:42: const SkString& name () const { return
fFamilyName; }
On 2014/07/21 15:48:00, bungeman1 wrote:
> nit: name()

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:46: 
On 2014/07/21 15:48:00, bungeman1 wrote:
> We can put an fTtcIndex right here, and use it as needed to get ttc goodness

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:53: /// them, so re-open them every time we need
a fresh stream
On 2014/07/21 15:48:00, bungeman1 wrote:
> This comment (or at least most of it) seems more attached to 'onOpenStream',
> maybe it should go closer to it?

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:117: // created from Streams; that one can call
duplicate().
On 2014/07/21 15:48:00, bungeman1 wrote:
> Looks like you've done this todo?

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:141: const SkString& fileName =
family->fFontFiles[i].fFileName;
On 2014/07/21 15:48:00, bungeman1 wrote:
> We have SkTTCFHeader.h and can write some simple code to sniff if it's a TTC
and
> how many fonts are in the TTC so we can pass the indexes in. Unless it's
> supposed to be up to the FontFamily to contain the indexes, in which case we
can
> just blindly pass that around.

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:150: find_name_and_attributes(stream.get(),
&fontName, &style, &isFixedWidth);
On 2014/07/21 15:48:00, bungeman1 wrote:
> We should check (at least assert or message) the return value of this for
> failure.

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:161: SkASSERT(index >= 0 && index <
fStyles.count());
On 2014/07/21 15:48:00, bungeman1 wrote:
> Since this is public API, I think we should probably just be runtime checking
> this always.

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:163: SkASSERT(name);
On 2014/07/21 15:48:01, bungeman1 wrote:
> These need to be like 'if (style) ...'. Any subset may be NULL, indicating
that
> they aren't interested in that value.

Done.

https://codereview.chromium.org/384503002/diff/100001/src/ports/SkFontMgr_and...
src/ports/SkFontMgr_android.cpp:165: name->reset();  // UNDEFINED SEMANTICS?
On 2014/07/21 15:48:00, bungeman1 wrote:
> I think this is right for when it isn't supported (which is hardly ever).
We'll
> need to back-fill this later I think.

Acknowledged.

Expand Messages | Collapse Messages