Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
427293003:
Implement SkFontMgr_Android::onMatchFamilyStyleCharacter. (Closed)
|
Created:
6 years, 4 months ago by bungeman-skia Modified:
6 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Project:
skia Visibility:
Public. |
DescriptionImplement SkFontMgr_Android::onMatchFamilyStyleCharacter.
This will allow us to replace SkGetFallbackFamilyNameForChar
and also eventually fix issues with it's reliance on names.
Committed: https://skia.googlesource.com/skia/+/65fcd3de3cb9dfd766c460500586eb939e6a6d68
Patch Set 1 : First thing that works. #
Total comments: 17
Patch Set 2 : Address comments, move mask to parser. #
Total comments: 14
Patch Set 3 : Address comments, treat mask as style. #Patch Set 4 : Remove commented out test shim. #
Total comments: 6
Patch Set 5 : Address comments, clarify code. #Messages
Total messages: 14 (0 generated)
At PS1 the Clankium/Blink side change is
- SkString skiaFamilyName;
- if (!SkGetFallbackFamilyNameForChar(c, locale, &skiaFamilyName) ||
skiaFamilyName.isEmpty())
+ SkFontMgr* fm = SkFontMgr::RefDefault();
+ SkTypeface* typeface = fm->matchFamilyStyleCharacter(NULL, SkFontStyle(),
locale, c);
+
+ if (NULL == typeface)
return emptyAtom;
+ SkString skiaFamilyName;
+ typeface->getFamilyName(&skiaFamilyName);
+ fm->unref();
+ typeface->unref();
https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.cpp
File src/ports/SkFontMgr_android.cpp (right):
https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android....
src/ports/SkFontMgr_android.cpp:38: //}
I'll remove this block soon. I've commented out the following blocks on Clankium
and this is all that is needed to keep things linking. Once we remove the bit in
SkScalerContext.cpp which calls this, it will no longer be needed at all.
+# [ 'OS == "android"', {
+# 'defines': [
+# 'SK_FONTHOST_DOES_NOT_USE_FONTMGR',
+# ],
+# }],
+# '../third_party/skia/src/ports/SkFontConfigInterface_android.cpp',
+# '../third_party/skia/src/ports/SkFontConfigInterface_direct.cpp',
+# '../third_party/skia/src/fonts/SkFontMgr_fontconfig.cpp',
+# '../third_party/skia/src/ports/SkFontHost_fontconfig.cpp',
https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android....
src/ports/SkFontMgr_android.cpp:72: , fUseInFallback(useInFallback)
For the moment I'm sticking this on the typeface since that's more or less how
it was being done before. However, it appears that we should just never add
these to the fallback font families to begin with.
https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.cpp File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:91: public: seems like having 2 getters makes more sense than making these fields public https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:151: int ttcIndex = family->fFontFiles[i].fIndex; const int https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:161: static int32_t acceptedVariants = SkPaintOptionsAndroid::kDefault_Variant | let's document why we do this. The compact variant is the real source of the issue as they are used by the OS in places that are space constrained and having variable ascent/descent values are not acceptable. So we exclude the compact variant for when building our dictionary of fonts. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:418: if (families[i]->fIsFallbackFont) { in general this if/else block is very dense and hard to parse. Can we dedupe any of this code or at least insert a few comments to help explain?
Please give us a full-sentences-English CL description. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.cpp File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:68: bool useInFallback) You pass "is accepted fallback variant" to "use in fallback". Naming not transparent; comment or rename. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:73: { } Nit: Skia style? https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:91: public: On 2014/07/30 17:31:43, djsollen wrote: > seems like having 2 getters makes more sense than making these fields public Acknowledged. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:161: static int32_t acceptedVariants = SkPaintOptionsAndroid::kDefault_Variant | On 2014/07/30 17:31:43, djsollen wrote: > let's document why we do this. > > The compact variant is the real source of the issue as they are used by the OS > in places that are space constrained and having variable ascent/descent values > are not acceptable. > > So we exclude the compact variant for when building our dictionary of fonts. Acknowledged. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:418: if (families[i]->fIsFallbackFont) { On 2014/07/30 17:31:43, djsollen wrote: > in general this if/else block is very dense and hard to parse. Can we dedupe any > of this code or at least insert a few comments to help explain? Acknowledged.
PS2 is quite a bit cleaner and also works well with Clankium. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.cpp File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:68: bool useInFallback) On 2014/07/30 17:54:58, tomhudson wrote: > You pass "is accepted fallback variant" to "use in fallback". > Naming not transparent; comment or rename. Removed. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:73: { } On 2014/07/30 17:54:58, tomhudson wrote: > Nit: Skia style? Hmm, we do this in a number of other places, Skia style is a little... something. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:91: public: On 2014/07/30 17:31:43, djsollen wrote: > seems like having 2 getters makes more sense than making these fields public You Java people and your gratuitous setters and getters. Even in Java I would just make this a final field. Heck, in Java I would have made this a static inner class. Made private const and SkFontMgr_Android a friend for reading. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:151: int ttcIndex = family->fFontFiles[i].fIndex; On 2014/07/30 17:31:43, djsollen wrote: > const int Done. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:161: static int32_t acceptedVariants = SkPaintOptionsAndroid::kDefault_Variant | On 2014/07/30 17:31:43, djsollen wrote: > let's document why we do this. > > The compact variant is the real source of the issue as they are used by the OS > in places that are space constrained and having variable ascent/descent values > are not acceptable. > > So we exclude the compact variant for when building our dictionary of fonts. Moved all the way over to the parser. https://codereview.chromium.org/427293003/diff/1/src/ports/SkFontMgr_android.... src/ports/SkFontMgr_android.cpp:418: if (families[i]->fIsFallbackFont) { On 2014/07/30 17:31:43, djsollen wrote: > in general this if/else block is very dense and hard to parse. Can we dedupe any > of this code or at least insert a few comments to help explain? Made it much different now.
https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:207: uint32_t ignoredVariants) ignoredVariants = 0; https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:328: parseConfigFile(testFallbackConfigFile, fallbackFonts, 0); will this break Chromium? https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:48: void GetFontFamilies(SkTDArray<FontFamily*> &fontFamilies, uint32_t ignoredFallbackVariants = 0); add a comment to the method documenting what this param does. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:36: //SkTypeface* SkAndroidNextLogicalTypeface(SkFontID, SkFontID, SkPaintOptionsAndroid const&) { drop this https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:89: friend class SkFontMgr_Android; why!! A simple get function makes it clear that you should only be querying fLang https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:162: fontName = *cannonicalName; what is the benefit of passing the same name for each font in the family? https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:257: SkPaintOptionsAndroid::kCompact_Variant); document why we ignore the compact variant for fallbacks!
PS3 has been tested with Clankium and things work. The main idea here is to treat 'variant' as weakly bound style. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:207: uint32_t ignoredVariants) On 2014/07/31 14:51:37, djsollen wrote: > ignoredVariants = 0; Actually, I'm trying to follow the "as few default arguments as possible" directive. I'll probably end up removing the default from the top level call as well. Also, I've removed this. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:328: parseConfigFile(testFallbackConfigFile, fallbackFonts, 0); On 2014/07/31 14:51:37, djsollen wrote: > will this break Chromium? Yes, I need to add the param here as well and fix up the current caller. Except now I don't need to do anything, since it's been removed and this file is no longer changing. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:48: void GetFontFamilies(SkTDArray<FontFamily*> &fontFamilies, uint32_t ignoredFallbackVariants = 0); On 2014/07/31 14:51:37, djsollen wrote: > add a comment to the method documenting what this param does. Removed. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:36: //SkTypeface* SkAndroidNextLogicalTypeface(SkFontID, SkFontID, SkPaintOptionsAndroid const&) { On 2014/07/31 14:51:37, djsollen wrote: > drop this As soon as https://codereview.chromium.org/434623002/ lands. Until then I need this around for testing and to remind me that this can't be committed yet (either the other change must land first or I'll have to come up with another fix). https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:89: friend class SkFontMgr_Android; On 2014/07/31 14:51:37, djsollen wrote: > why!! A simple get function makes it clear that you should only be querying > fLang As discussed since this is all hidden away in the cpp file, just made the things public since they're const. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:162: fontName = *cannonicalName; On 2014/07/31 14:51:37, djsollen wrote: > what is the benefit of passing the same name for each font in the family? The name is the family name as specified by the FontFamily. Basically, we need to ensure getFamilyName on the typeface will return a string which can be looked up to find the typeface again. I'll try to clarify this in the naming. https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:257: SkPaintOptionsAndroid::kCompact_Variant); On 2014/07/31 14:51:37, djsollen wrote: > document why we ignore the compact variant for fallbacks! Eck, I don't think we want to. Now that I've done this and you've written this and I tried to document it, it turns out that 'variant' is just like 'width' or 'stretch' except vertical, and is a style against which we should be matching. In other words, if we're doing fallback for a 'compact' font, then we'd prefer 'compact' matches.
At PS4 this is something which can be committed, and with some fancy footwork can also be used by Clankium to get off of the static calls currently being made.
https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:335: if (compactLiteralLen <= familyNameLen) { should this be < or would a font with the name "compact" also be a compact font? https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:346: for (int i = 0; i < 2; ++i, ignoredVariantMask = ~ignoredVariantMask) { document that the second time through the loop looks for the opposite of the initial mask. https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:372: } while (!lang.getTag().isEmpty() && (lang = lang.getParent(), true)); At first glance it is not clear that the last run through the loop is with an empty langTag, so please document it.
https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... File src/ports/SkFontMgr_android.cpp (right): https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:335: if (compactLiteralLen <= familyNameLen) { On 2014/08/01 20:24:04, djsollen wrote: > should this be < or would a font with the name "compact" also be a compact font? So, when I wrote this I was under the impression from the enumeration that fonts were effectively 'elegant' or 'compact' with default sort if being a lazy out. It actually appears that 'default' means (in some sense) both 'elegant' and 'compact'. It's 'elegant' because it hasn't been squashed, but it's 'compact' because it doesn't draw out of the ascent/descent. As a result, this is something which actually needs to go on the 'style' request, along with the 5000 other things which would also be nice to have on the requested style. For the time being I will set it up so that this can be implemented easily in the future, but will currently use the 'elegant' first approach as that's what Blink expects. https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:346: for (int i = 0; i < 2; ++i, ignoredVariantMask = ~ignoredVariantMask) { On 2014/08/01 20:24:04, djsollen wrote: > document that the second time through the loop looks for the opposite of the > initial mask. Changed the logic to make this more obvious and added a comment. https://codereview.chromium.org/427293003/diff/60001/src/ports/SkFontMgr_andr... src/ports/SkFontMgr_android.cpp:372: } while (!lang.getTag().isEmpty() && (lang = lang.getParent(), true)); On 2014/08/01 20:24:04, djsollen wrote: > At first glance it is not clear that the last run through the loop is with an > empty langTag, so please document it. Documented.
On 2014/07/31 22:15:53, bungeman1 wrote: > https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... > File src/ports/SkFontMgr_android.cpp (right): > > https://codereview.chromium.org/427293003/diff/20001/src/ports/SkFontMgr_andr... > src/ports/SkFontMgr_android.cpp:36: //SkTypeface* > SkAndroidNextLogicalTypeface(SkFontID, SkFontID, SkPaintOptionsAndroid const&) { > On 2014/07/31 14:51:37, djsollen wrote: > > drop this > > As soon as https://codereview.chromium.org/434623002/ lands. Until then I need > this around for testing and to remind me that this can't be committed yet > (either the other change must land first or I'll have to come up with another > fix). '3002 landed this morning and seems to be sticking.
lgtm
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/bungeman@google.com/427293003/80001
Message was sent while issue was closed.
Change committed as 65fcd3de3cb9dfd766c460500586eb939e6a6d68 |
