Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2441343003:
Allow the default generic font family settings to find the first available font (Closed)
|
Created:
4 years, 1 month ago by kojii Modified:
4 years, 1 month ago CC:
ajuma+watch_chromium.org, Alexei Svitkine (slow), blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, Daniel Erat, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow the default generic font family settings to find the first available font
This patch allows the default generic font family settings to have a
list of fonts that resolves to the first available font on the system.
Since only small part of the settings are lists, this patch introduces
a convention that the font name is a list only when it starts with ",".
This avoids scanning the name and checking the font availability when
it's not necessary.
MaybeGetLocalizedFontName is moved to FontSettingsUtilities to
eliminate the duplication in options and extensions.
The extension API chrome.fontSettings.getFont[1] is unchanged. It
resolves to the first available font on getting the value.
Resource changes in grd/xtb files will be in following patches (e.g.,
crrev.com/2454583002)
[1] https://developer.chrome.com/extensions/fontSettings#method-getFont
BUG=658646
Committed: https://crrev.com/6c0885e2699ef3aac4f3d8186d0520e6386094b9
Cr-Commit-Position: refs/heads/master@{#428590}
Patch Set 1 #Patch Set 2 : WIP #Patch Set 3 : WIP #Patch Set 4 : Add tests #Patch Set 5 : More tests #Patch Set 6 : Fix and resource changes for ja #Patch Set 7 : Split resource changes to a separate CL #Patch Set 8 : Fix font settings (non-advanced) #Patch Set 9 : Fix non-Windows builds #Patch Set 10 : Merge dupicated code #Patch Set 11 : Cleanup #Patch Set 12 : Add more tests #
Total comments: 19
Patch Set 13 : Un-shared FirstAvailableOrFirst as per msw review #
Total comments: 10
Patch Set 14 : Add font_settings_utils_unittest #
Total comments: 13
Patch Set 15 : msw, dbeam, falken review #Patch Set 16 : Code shared in ui/gfx #
Total comments: 8
Patch Set 17 : msw review #Messages
Total messages: 95 (73 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from ========== WIP: generic BUG= ========== to ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since not all settings require to have a list, this patch introduces a convention that the font name is considered as a list only when it starts with "," to avoid scanning all font names. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches. [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 ==========
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #12 (id:220001) has been deleted
Description was changed from ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since not all settings require to have a list, this patch introduces a convention that the font name is considered as a list only when it starts with "," to avoid scanning all font names. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches. [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 ========== to ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since only small part of the settings are lists, this patch introduces a convention that the font name is a list only when it starts with ",". This avoids scanning the name and checking the font availability when it's not necessary. MaybeGetLocalizedFontName is moved to FontSettingsUtilities to eliminate the duplication in options and extensions. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches. [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 ==========
kojii@chromium.org changed reviewers: + dbeam@chromium.org, eae@chromium.org, falken@chromium.org, msw@chromium.org
PTAL. falken: chrome/browser/extensions/api/font_settings/font_settings_api.cc dbeam: chrome/browser/ui/webui/options/font_settings_* eae: third_party/WebKit/Source/platform/* msw: ui/gfx/font*
Description was changed from ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since only small part of the settings are lists, this patch introduces a convention that the font name is a list only when it starts with ",". This avoids scanning the name and checking the font availability when it's not necessary. MaybeGetLocalizedFontName is moved to FontSettingsUtilities to eliminate the duplication in options and extensions. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches. [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 ========== to ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since only small part of the settings are lists, this patch introduces a convention that the font name is a list only when it starts with ",". This avoids scanning the name and checking the font availability when it's not necessary. MaybeGetLocalizedFontName is moved to FontSettingsUtilities to eliminate the duplication in options and extensions. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches (e.g., crrev.com/2454583002) [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM for Source/platform https://codereview.chromium.org/2441343003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/2441343003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:308: return String::fromUTF8( I don't suppose we could change gfx::Font::FirstAvailableOrFirst to take a utf-16 string? If not this is fine but please add a comment explaining the conversions.
This seems a bit unusual, can you try using similar existing codepaths? https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:11: // Returns the first available font if it starts with ",". These comments belong with the function declarations in the header. https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:15: return gfx::Font::FirstAvailableOrFirst(font_name_or_list); Why aren't we using the standard gfx::FontList codepaths here? Why would this ignore the other fonts in the list? Having fallbacks is important. If we are indeed only using the first font, that should probably be part of the function name, and the reasoning should be justified in a comment. https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:23: std::string FontSettingsUtilities::MaybeGetLocalizedFontName( Ditto to my comments and questions above. https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils_win.cc (right): https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils_win.cc:17: // Returns the localized name of a font so that settings can find it within the Ditto, this comment belongs in the header or in the function body. https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc File ui/gfx/font.cc (right): https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode100 ui/gfx/font.cc:100: fontManager->legacyCreateTypeface(family.c_str(), SkFontStyle())); Why are we using a legacy skia codepath here and an unusual codepath below? gfx::Font has a constructor that takes a name and size, then you could apply a style as needed. Why would we want to construct fonts differently here than elsewhere in ui/gfx and chrome? https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode103 ui/gfx/font.cc:103: sk_sp<SkFontStyleSet> sset(fontManager->matchFamily(family.c_str())); nit: |style_set| or just |set| https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode109 ui/gfx/font.cc:109: std::vector<std::string> families = base::SplitString( Could we use gfx::FontList's constructor that takes a font description string here? Perhaps the callers could just do something like gfx::Font first(gfx::FontList(description_string).DeriveFoo(foo).GetPrimaryFont()); https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.h#newcode133 ui/gfx/font.h:133: static std::string FirstAvailableOrFirst(const std::string&); Add a comment, what's the difference between "first available" and "first"? https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.h#newcode133 ui/gfx/font.h:133: static std::string FirstAvailableOrFirst(const std::string&); Is this actually needed as part of the gfx::font api? If this unusual font-related functionality is only used by blink, and nowhere else in the codebase, then perhaps this doesn't belong here.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msw@, thank you having a quick review. As per your comments, PS13: * Moves the Font changes to font_settings_util * and have a different version in Blink Does this look better? More detailed replies inline below. https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:11: // Returns the first available font if it starts with ",". On 2016/10/26 at 20:33:49, msw wrote: > These comments belong with the function declarations in the header. Thank you, not very familiar with Chromium side yet. https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:15: return gfx::Font::FirstAvailableOrFirst(font_name_or_list); On 2016/10/26 at 20:33:49, msw wrote: > Why aren't we using the standard gfx::FontList codepaths here? Why would this ignore the other fonts in the list? Having fallbacks is important. If we are indeed only using the first font, that should probably be part of the function name, and the reasoning should be justified in a comment. gfx::FontList cascades by the availability of glyphs, but this function needs the availability of font. If the first available font doesn't have a glyph, it needs to fallback to the next font specified in CSS. I'll add comments to clarify. https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc File ui/gfx/font.cc (right): https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode100 ui/gfx/font.cc:100: fontManager->legacyCreateTypeface(family.c_str(), SkFontStyle())); On 2016/10/26 at 20:33:49, msw wrote: > Why are we using a legacy skia codepath here and an unusual codepath below? gfx::Font has a constructor that takes a name and size, then you could apply a style as needed. Why would we want to construct fonts differently here than elsewhere in ui/gfx and chrome? Because if I create gfx::Font("that does not exist"), it fallbacks to kFallbackFontFamilyName, and it doesn't tell if the specified name exists or not. https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode103 ui/gfx/font.cc:103: sk_sp<SkFontStyleSet> sset(fontManager->matchFamily(family.c_str())); On 2016/10/26 at 20:33:49, msw wrote: > nit: |style_set| or just |set| Done. https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode109 ui/gfx/font.cc:109: std::vector<std::string> families = base::SplitString( On 2016/10/26 at 20:33:49, msw wrote: > Could we use gfx::FontList's constructor that takes a font description string here? Perhaps the callers could just do something like gfx::Font first(gfx::FontList(description_string).DeriveFoo(foo).GetPrimaryFont()); Hmm...FontList::ParseDescription() needs size, and include empty entries. If this looks like not generic enough to put in gfx, I can move to font_settings_utils and make a copy in Blink. Does that work better? I actually did so initially but looked for somewhere to share the code in PS10. But the code is small and probably not too generic, I'm fine not to share if doing so looks better. PS13 shows the un-shared version.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry that the best path forward isn't clear to me here... https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:15: return gfx::Font::FirstAvailableOrFirst(font_name_or_list); On 2016/10/26 22:20:02, kojii wrote: > On 2016/10/26 at 20:33:49, msw wrote: > > Why aren't we using the standard gfx::FontList codepaths here? Why would this > ignore the other fonts in the list? Having fallbacks is important. If we are > indeed only using the first font, that should probably be part of the function > name, and the reasoning should be justified in a comment. > > gfx::FontList cascades by the availability of glyphs, but this function needs > the availability of font. If the first available font doesn't have a glyph, it > needs to fallback to the next font specified in CSS. > > I'll add comments to clarify. Using FontList to just parse the string and then return the primary font name wouldn't preclude using the CSS fallback mechanism. But I see there may be some other obstacles to going in that direction... https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc File ui/gfx/font.cc (right): https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode100 ui/gfx/font.cc:100: fontManager->legacyCreateTypeface(family.c_str(), SkFontStyle())); On 2016/10/26 22:20:03, kojii wrote: > On 2016/10/26 at 20:33:49, msw wrote: > > Why are we using a legacy skia codepath here and an unusual codepath below? > gfx::Font has a constructor that takes a name and size, then you could apply a > style as needed. Why would we want to construct fonts differently here than > elsewhere in ui/gfx and chrome? > > Because if I create gfx::Font("that does not exist"), it fallbacks to > kFallbackFontFamilyName, and it doesn't tell if the specified name exists or > not. Since FontList::ParseDescription requires a size, would SkTypeface::MakeFromName help? The less repetition of deprecated lower-level skia code we have in chrome, the better. (that said, it's good this follows a pattern similar to that in SkTypeface::MakeFromName, perhaps mention that in a comment?). Perhaps gfx::Font could have a flag indicating if it fell back to some default, but I'm not entirely sure we need do that just yet. https://codereview.chromium.org/2441343003/diff/240001/ui/gfx/font.cc#newcode109 ui/gfx/font.cc:109: std::vector<std::string> families = base::SplitString( On 2016/10/26 22:20:03, kojii wrote: > On 2016/10/26 at 20:33:49, msw wrote: > > Could we use gfx::FontList's constructor that takes a font description string > here? Perhaps the callers could just do something like gfx::Font > first(gfx::FontList(description_string).DeriveFoo(foo).GetPrimaryFont()); > > Hmm...FontList::ParseDescription() needs size, and include empty entries. > > If this looks like not generic enough to put in gfx, I can move to > font_settings_utils and make a copy in Blink. Does that work better? I actually > did so initially but looked for somewhere to share the code in PS10. But the > code is small and probably not too generic, I'm fine not to share if doing so > looks better. > > PS13 shows the un-shared version. Hmm, perhaps derat or asvitkine would have a better idea if this code belongs in gfx... or perhaps there's some better location for sharing blink font util code between c/b/ui/webui/options and third_party/WebKit/Source/platform/fonts/... https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:13: bool IsFontFamilyAvailable(const std::string& family, SkFontMgr* fontManager) { If these functions are file-local, they should go in an anon namespace. https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:24: // Returns the first available font. If there is no available font, returns the Ditto nit: say 'font name' or 'font family'. https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:32: for (auto& family : families) { nit: 'const auto&' https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:21: // Returns the first available font if it starts with ",". This doesn't actually return a font, can you clarify the return value? Is it a localized font family name? Also, clarify what this returns if the string does not start with a comma. It would also be good to give the actual param name instead of 'it', ie. "if |font_name_or_list| starts with". https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:24: static std::string ResolveFontList(const std::string&); nit: add param names here and below. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:19: sk_sp<SkFontStyleSet> set(fontManager->matchFamily(family.c_str())); How does this non-linux code align with Mac and Windows? It seems like it's missing the DirectWrite and GDI code for font lookup. Shouldn't that be part of IsFontFamilyAvailable (run in a nested loop), instead of within MaybeGetLocalizedFontName, after the loop returns some value. The current pattern seems like it might miss values that only PlatformFontWin could resolve. But maybe that is what you actually intend, if the PlatformFontWin would return a fallback for an invalid font name that you wouldn't want to respect. For example if |font_name_list| is "OnlyFoundByDirectWrite, GenericFont", then ResolveFontList would return "GenericFont". Perhaps it should return "OnlyFoundByDirectWrite" or similar for GDI, but maybe not if those return values are just going to be fallback fonts... it's hard to say without knowing more about the caller's intent. I also have no idea about Mac...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:37: } ^ why are we duplicating this with the blink code? https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:24: static std::string ResolveFontList(const std::string&); add parameter names
The change to font_settings_api.cc lgtm. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:28: // only for the system locale, but the pref value may be in the English name. The comment is not from your patch, but it might be illustrative to add an example (e.g., MSゴシック）... (e.g., MS Gothic) if memory serves correctly.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you msw, dbeam, falken. All comments reflected/replied inline below: https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:13: bool IsFontFamilyAvailable(const std::string& family, SkFontMgr* fontManager) { On 2016/10/27 at 00:04:18, msw wrote: > If these functions are file-local, they should go in an anon namespace. Done. https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:24: // Returns the first available font. If there is no available font, returns the On 2016/10/27 at 00:04:18, msw wrote: > Ditto nit: say 'font name' or 'font family'. Done. https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:32: for (auto& family : families) { On 2016/10/27 at 00:04:18, msw wrote: > nit: 'const auto&' Done. https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:21: // Returns the first available font if it starts with ",". On 2016/10/27 at 00:04:18, msw wrote: > This doesn't actually return a font, can you clarify the return value? Is it a localized font family name? Also, clarify what this returns if the string does not start with a comma. It would also be good to give the actual param name instead of 'it', ie. "if |font_name_or_list| starts with". Clarified. This function returns the specified name, the function below |MaybeGetLocalizedFontName| gets localized name. https://codereview.chromium.org/2441343003/diff/260001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:24: static std::string ResolveFontList(const std::string&); On 2016/10/27 at 00:04:18, msw wrote: > nit: add param names here and below. Done. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:19: sk_sp<SkFontStyleSet> set(fontManager->matchFamily(family.c_str())); On 2016/10/27 at 00:04:18, msw wrote: > How does this non-linux code align with Mac and Windows? It seems like it's missing the DirectWrite and GDI code for font lookup. Shouldn't that be part of IsFontFamilyAvailable (run in a nested loop), instead of within MaybeGetLocalizedFontName, after the loop returns some value. The current pattern seems like it might miss values that only PlatformFontWin could resolve. But maybe that is what you actually intend, if the PlatformFontWin would return a fallback for an invalid font name that you wouldn't want to respect. > > For example if |font_name_list| is "OnlyFoundByDirectWrite, GenericFont", then ResolveFontList would return "GenericFont". Perhaps it should return "OnlyFoundByDirectWrite" or similar for GDI, but maybe not if those return values are just going to be fallback fonts... it's hard to say without knowing more about the caller's intent. I also have no idea about Mac... First, this function needs to return available font visible to Skia, because chrome setting UI and Blink creates font from the name independently. Skia doesn't recognize "GenericFont", I added the case to font_settings_utils_unittest.cc. MakeFromName does not accept fontManager as a parameter, and matchFamily() is not supported in Linux (fontconfig), so this looks like the only choice for now. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:37: } On 2016/10/27 at 02:07:24, Dan Beam wrote: > ^ why are we duplicating this with the blink code? Two reasons: 1. No good place to share the code. Tried ui::gfx but msw@ disliked in PS12. Can't find good place in base. 2. Blink string has Latin1/UTF-16 while chrome has UTF-8. Blink calls this during layout and thus would like to be fast. See eae@'s comment in PS12. I'm fine to re-try to share the code if you could advice me a good place -- I'm not very familiar with chromium side yet. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:24: static std::string ResolveFontList(const std::string&); On 2016/10/27 at 02:07:24, Dan Beam wrote: > add parameter names Done. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:28: // only for the system locale, but the pref value may be in the English name. On 2016/10/27 at 02:21:59, falken wrote: > The comment is not from your patch, but it might be illustrative to add an example (e.g., MSゴシック）... (e.g., MS Gothic) if memory serves correctly. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Just to make sure reviewers understand the purpose of this change. We'd like to pick fonts for chrome://settings/fonts, one for each standard, serif, sans-serif, etc., but "common" font in CJK is a bit more complex probably due to large font size. Japanese for example: Win7/8: Meiryo, MS Gothic Win8.1: Meiryo, MS Gothic, Yu Gothic Win10 (ja): Meiryo, MS Gothic, Yu Gothic Win10 (en): Yu Gothic Win10 update (en): Yu Gothic, MS Gothic Due to some other problems we're working on, Meiryo renders the best if available, but it's not available on non-Japanese versions of Win10. Chinese has similar issue. Blink has a code path that shapes text using HarfBuzz and handles font fallback, which is IIUC similar to RenderText in chrome, and that code wants to know what the standard (or serif etc.) font is. I hope this explains why we want first available font rather than character-level fallback.
Mostly lg, I'm open to consolidating the functions in ui/gfx if folks agree that's an appropriate place for blink & webui utility code that isn't used by browser ui. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:19: sk_sp<SkFontStyleSet> set(fontManager->matchFamily(family.c_str())); On 2016/10/27 04:50:56, kojii wrote: > On 2016/10/27 at 00:04:18, msw wrote: > > How does this non-linux code align with Mac and Windows? It seems like it's > missing the DirectWrite and GDI code for font lookup. Shouldn't that be part of > IsFontFamilyAvailable (run in a nested loop), instead of within > MaybeGetLocalizedFontName, after the loop returns some value. The current > pattern seems like it might miss values that only PlatformFontWin could resolve. > But maybe that is what you actually intend, if the PlatformFontWin would return > a fallback for an invalid font name that you wouldn't want to respect. > > > > For example if |font_name_list| is "OnlyFoundByDirectWrite, GenericFont", then > ResolveFontList would return "GenericFont". Perhaps it should return > "OnlyFoundByDirectWrite" or similar for GDI, but maybe not if those return > values are just going to be fallback fonts... it's hard to say without knowing > more about the caller's intent. I also have no idea about Mac... > > First, this function needs to return available font visible to Skia, because > chrome setting UI and Blink creates font from the name independently. > > Skia doesn't recognize "GenericFont", I added the case to > font_settings_utils_unittest.cc. > > MakeFromName does not accept fontManager as a parameter, and matchFamily() is > not supported in Linux (fontconfig), so this looks like the only choice for now. Sorry for not being clear, "GenericFont" was meant to be a placeholder for a very common font name that skia should always find, you don't really need to add that fake font name to a unit test. I now understand that we don't want to do separate DirectWrite font lookup, we *just* want fonts that skia can find; thanks for clarifying. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:37: } On 2016/10/27 04:50:56, kojii wrote: > On 2016/10/27 at 02:07:24, Dan Beam wrote: > > ^ why are we duplicating this with the blink code? > > Two reasons: > 1. No good place to share the code. Tried ui::gfx but msw@ disliked in PS12. > Can't find good place in base. > 2. Blink string has Latin1/UTF-16 while chrome has UTF-8. Blink calls this > during layout and thus would like to be fast. See eae@'s comment in PS12. > > I'm fine to re-try to share the code if you could advice me a good place -- I'm > not very familiar with chromium side yet. I'm not sure if ui/gfx makes sense, because this is only used for blink and webui at the moment, not browser ui... WDYT, Dan?
https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:37: } On 2016/10/27 17:18:11, msw wrote: > On 2016/10/27 04:50:56, kojii wrote: > > On 2016/10/27 at 02:07:24, Dan Beam wrote: > > > ^ why are we duplicating this with the blink code? > > > > Two reasons: > > 1. No good place to share the code. Tried ui::gfx but msw@ disliked in PS12. > > Can't find good place in base. > > 2. Blink string has Latin1/UTF-16 while chrome has UTF-8. Blink calls this > > during layout and thus would like to be fast. See eae@'s comment in PS12. > > > > I'm fine to re-try to share the code if you could advice me a good place -- > I'm > > not very familiar with chromium side yet. > > I'm not sure if ui/gfx makes sense, because this is only used for blink and > webui at the moment, not browser ui... WDYT, Dan? ui/base?
https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:37: } On 2016/10/27 17:29:41, Dan Beam wrote: > On 2016/10/27 17:18:11, msw wrote: > > On 2016/10/27 04:50:56, kojii wrote: > > > On 2016/10/27 at 02:07:24, Dan Beam wrote: > > > > ^ why are we duplicating this with the blink code? > > > > > > Two reasons: > > > 1. No good place to share the code. Tried ui::gfx but msw@ disliked in PS12. > > > Can't find good place in base. > > > 2. Blink string has Latin1/UTF-16 while chrome has UTF-8. Blink calls this > > > during layout and thus would like to be fast. See eae@'s comment in PS12. > > > > > > I'm fine to re-try to share the code if you could advice me a good place -- > > I'm > > > not very familiar with chromium side yet. > > > > I'm not sure if ui/gfx makes sense, because this is only used for blink and > > webui at the moment, not browser ui... WDYT, Dan? > > ui/base? If ui/ is the right top-level dir, then gfx/ would be the best subdir. (that matches the location of most skia and text util code in //ui) Sorry for the churn caused by my uncertainty, I guess ui/gfx is fine. Perhaps add some explanatory usage comments with these utils, something like "Used by Blink to pick the primary serif/sans/fixed/etc. fonts from region-specific IDS lists." (and please correct me if I'm wrong)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #16 (id:320001) has been deleted
PTAL, PS16 shared the code. https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.cc (right): https://codereview.chromium.org/2441343003/diff/280001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.cc:37: } On 2016/10/27 at 18:24:02, msw wrote: > On 2016/10/27 17:29:41, Dan Beam wrote: > > On 2016/10/27 17:18:11, msw wrote: > > > On 2016/10/27 04:50:56, kojii wrote: > > > > On 2016/10/27 at 02:07:24, Dan Beam wrote: > > > > > ^ why are we duplicating this with the blink code? > > > > > > > > Two reasons: > > > > 1. No good place to share the code. Tried ui::gfx but msw@ disliked in PS12. > > > > Can't find good place in base. > > > > 2. Blink string has Latin1/UTF-16 while chrome has UTF-8. Blink calls this > > > > during layout and thus would like to be fast. See eae@'s comment in PS12. > > > > > > > > I'm fine to re-try to share the code if you could advice me a good place -- > > > I'm > > > > not very familiar with chromium side yet. > > > > > > I'm not sure if ui/gfx makes sense, because this is only used for blink and > > > webui at the moment, not browser ui... WDYT, Dan? > > > > ui/base? > > If ui/ is the right top-level dir, then gfx/ would be the best subdir. > (that matches the location of most skia and text util code in //ui) > Sorry for the churn caused by my uncertainty, I guess ui/gfx is fine. Blink can't use ui/base (yet?); only ui/gfx and several under base/. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/DEPS Thank you for the whole discussion, this was really helpful to understand chromium structures better for me. > Perhaps add some explanatory usage comments with these utils, something like "Used by Blink to pick the primary serif/sans/fixed/etc. fonts from region-specific IDS lists." (and please correct me if I'm wrong) You described very accurately, added, thank you again.
Replying to eae@'s earlier comment since we're sharing the code again. https://codereview.chromium.org/2441343003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/2441343003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:308: return String::fromUTF8( On 2016/10/26 at 16:59:36, eae wrote: > I don't suppose we could change gfx::Font::FirstAvailableOrFirst to take a utf-16 string? If not this is fine but please add a comment explaining the conversions. I think it's possible, but after looking into it, I think it doesn't help much. The only input string as of now are all ASCII, and gfx::Font needs to convert the inputs to UTF-8 when it calls Skia. So from the perf perspective, the ideal thing is gfx::Font::FirstAvailableOrFirst to have Latin1 string to avoid string copies and convert between ASCII/UTF-8, but it looks like too specific. GenericFontFamilySettings caches the result, so this is called at most 1 or 2 times per renderer/setting change/script, and probably only for a few scripts. I think we're ok for now, and if the usage increases, I'm happy to look into further to avoid the conversions and copies. Added comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2441343003/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:32: static std::string MaybeGetLocalizedFontName(const std::string&); nit: param name https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.cc File ui/gfx/font.cc (right): https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.cc#newcode97 ui/gfx/font.cc:97: namespace { nit: put anon namespaces at the top of the file. https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.cc#newcode119 ui/gfx/font.cc:119: if (IsFontFamilyAvailable(family, fm.get())) Is it worth skipping this and early-returning families[0] when there's only one? https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.h#newcode137 ui/gfx/font.h:137: static std::string FirstAvailableOrFirst(const std::string& font_name_list); This probably shouldn't be a Font class member function; I'd add it to new font_util.h|cc files, maybe to skia_util.h|cc, or perhaps as a static FontList member (it's somewhat related, but not even used there...) but I've given you bad advice before, so feel free to ask another gfx owner :)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for your prompt review as always, all done. https://codereview.chromium.org/2441343003/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/font_settings_utils.h (right): https://codereview.chromium.org/2441343003/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/font_settings_utils.h:32: static std::string MaybeGetLocalizedFontName(const std::string&); On 2016/10/28 at 07:05:06, msw wrote: > nit: param name Done. https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.cc File ui/gfx/font.cc (right): https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.cc#newcode97 ui/gfx/font.cc:97: namespace { On 2016/10/28 at 07:05:06, msw wrote: > nit: put anon namespaces at the top of the file. Done. https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.cc#newcode119 ui/gfx/font.cc:119: if (IsFontFamilyAvailable(family, fm.get())) On 2016/10/28 at 07:05:06, msw wrote: > Is it worth skipping this and early-returning families[0] when there's only one? Done, unlikely for now but still good to have it. https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.h File ui/gfx/font.h (right): https://codereview.chromium.org/2441343003/diff/340001/ui/gfx/font.h#newcode137 ui/gfx/font.h:137: static std::string FirstAvailableOrFirst(const std::string& font_name_list); On 2016/10/28 at 07:05:06, msw wrote: > This probably shouldn't be a Font class member function; I'd add it to new font_util.h|cc files, maybe to skia_util.h|cc, or perhaps as a static FontList member (it's somewhat related, but not even used there...) but I've given you bad advice before, so feel free to ask another gfx owner :) FontList makes perfect sense to me, I was actually wondering which is better. Thank you for this many round of reviews, please don't call the efforts to seek for better as bad ;)
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Thank you all, so much appreciated!
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2441343003/#ps360001 (title: "msw review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #17 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since only small part of the settings are lists, this patch introduces a convention that the font name is a list only when it starts with ",". This avoids scanning the name and checking the font availability when it's not necessary. MaybeGetLocalizedFontName is moved to FontSettingsUtilities to eliminate the duplication in options and extensions. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches (e.g., crrev.com/2454583002) [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 ========== to ========== Allow the default generic font family settings to find the first available font This patch allows the default generic font family settings to have a list of fonts that resolves to the first available font on the system. Since only small part of the settings are lists, this patch introduces a convention that the font name is a list only when it starts with ",". This avoids scanning the name and checking the font availability when it's not necessary. MaybeGetLocalizedFontName is moved to FontSettingsUtilities to eliminate the duplication in options and extensions. The extension API chrome.fontSettings.getFont[1] is unchanged. It resolves to the first available font on getting the value. Resource changes in grd/xtb files will be in following patches (e.g., crrev.com/2454583002) [1] https://developer.chrome.com/extensions/fontSettings#method-getFont BUG=658646 Committed: https://crrev.com/6c0885e2699ef3aac4f3d8186d0520e6386094b9 Cr-Commit-Position: refs/heads/master@{#428590} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6c0885e2699ef3aac4f3d8186d0520e6386094b9 Cr-Commit-Position: refs/heads/master@{#428590} |
