|
|
Created:
4 years, 5 months ago by kojii Modified:
4 years, 2 months ago CC:
ajuma+watch_chromium.org, darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable "system-ui" generic font family
This patch enables the "system-ui" generic font family on all
platforms.
Following changes are planned but not included in this CL:
1. Content sets the current system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].
[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002
BUG=654679
Committed: https://crrev.com/bb3db4f221492076bca16af4d6fb9f358b79cedb
Cr-Commit-Position: refs/heads/master@{#425967}
Patch Set 1 #Patch Set 2 : WIP: system-ui #Patch Set 3 : WIP #Patch Set 4 : WIP #Patch Set 5 : Mac support #Patch Set 6 : Linux support #Patch Set 7 : Cleanup #Patch Set 8 : For rebaseline #Patch Set 9 : Rebaseline #Patch Set 10 : Minor cleanup for following CLs #Patch Set 11 : Fix Linux #Patch Set 12 : Rebase #Patch Set 13 : Rebase #Patch Set 14 : WIP #Patch Set 15 : Refactor and cleanup #
Total comments: 9
Patch Set 16 : Updated comments #
Total comments: 9
Patch Set 17 : drott review #Patch Set 18 : Cleanup defaultFontFamily #Patch Set 19 : Fix previous CL broke Android #Patch Set 20 : drott review #Patch Set 21 : Mac fix #Patch Set 22 : Cleanup and Mac fix #
Total comments: 5
Patch Set 23 : drott nit #Messages
Total messages: 116 (90 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
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 kojii@chromium.org
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 kojii@chromium.org
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 kojii@chromium.org
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-...)
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: 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: 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 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 #15 (id:280001) has been deleted
Description was changed from ========== WIP: system-ui BUG= ========== to ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the localized system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. 3. On Mac, expand "system-ui" to underlying name by size. This is expected to improve http://crbug.com/627609 . 4. On Mac, expand "system-ui" to underlying cascading list. This is more complex than other items and is expected to take longer. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ==========
The CQ bit was unchecked by kojii@chromium.org
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Patchset #15 (id:260002) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kojii@chromium.org changed reviewers: + drott@chromium.org, eae@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Note, I found LayoutTheme in late stage, incorrectly assuming "font: menu" is IE-only and thus Windows only, so this CL has little integration with LayoutTheme. Maybe what Emil meant "menu" was "font: menu", not the menu in browser. Should this be better integrated with LayoutTheme?
Good change, looks mostly good to me, a few comments and questions on the float parameter for systemFontForSize and the localized font name case. https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html:1: <!DOCTYPE html> Good test, thanks. https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const AtomicString& FontCache::systemFontFamily(float) { When will you need the parameter? If Mac instantiates with "systemFontForSize" in FontFamilyMatcherMac, do we need this parameter here? https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:237: // been replaced to the localied name. Use the Skia default family Can you add some explanations where was it replaced? localied -> localized. If I understand correctly, this will be in upcoming CLs? In this case, can you add a TODO with bug link here? When does this fail and we don't have a localized name? What are the chances it fails? As fallback if the localized name is missing, I'd prefer to fallback to one of the browser side generic families instead of Skia's default family, would that make sense? Skia's default is more similar to our LastResort - is that the intention? Or would falling back to another FontFamilyNames category make more sense? If it's closer to LastResort can we extract the name from FontCache::getLastResortFallbackFont? That gives us more control and we don't have to rely on Skia's default which may change.
Thank you for your quick review, replies inline. https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html:1: <!DOCTYPE html> On 2016/10/12 at 11:01:59, drott wrote: > Good test, thanks. ;) Thanks for creating this framework, this is really helpful. https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const AtomicString& FontCache::systemFontFamily(float) { On 2016/10/12 at 11:01:59, drott wrote: > When will you need the parameter? If Mac instantiates with "systemFontForSize" in FontFamilyMatcherMac, do we need this parameter here? I'm thinking to replace "system-ui" with ".SF NS Text" or ".SF NS Display", then with the list of cascading list. By replacing family name in this stage, we get different FontData for each actual fonts. This API surface supports only the first phase. I believe it's the significant part of broken letter spacing, and would like to consider the design when I'm more in sync with your plan to deprecate FontFallbackList and SimpleFontData. I haven't really well tested replacing ".SF NS Text"/".SF NS Display" works well yet, should have it soon. That said, this may need a revise, or two. I just added size now because that gives me where FontCache can replace the family name (i.e., only after it computed the size and know the locale.) https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:237: // been replaced to the localied name. Use the Skia default family On 2016/10/12 at 11:01:59, drott wrote: > Can you add some explanations where was it replaced? localied -> localized. > If I understand correctly, this will be in upcoming CLs? In this case, can you add a TODO with bug link here? FontCacheLinux already has setSystemFontFamily() in this CL, but nobody calls it yet. Getting the string from resources and call it will be in the next CL https://codereview.chromium.org/2138613002 > When does this fail and we don't have a localized name? What are the chances it fails? Linux English version does not have the string, so will use Skia default. I believe CJK versions have localized font names in the resources. Also CrOS has the resource even in English version. > As fallback if the localized name is missing, I'd prefer to fallback to one of the browser side generic families instead of Skia's default family, would that make sense? Skia's default is more similar to our LastResort - is that the intention? Or would falling back to another FontFamilyNames category make more sense? > If it's closer to LastResort can we extract the name from FontCache::getLastResortFallbackFont? That gives us more control and we don't have to rely on Skia's default which may change. It is the intention. When I read through our UI/menu rendering code in the browser, Linux/English uses nullptr to get the font from Skia, so this is mimicing our UI/menu rendering code. Our LastResort could be more suitable for document rendering, the same logic as UI/menu rendering looks better to me, no? Thanks for catching the spell error, will fix in the next CL.
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...
> Thanks for catching the spell error, will fix in the next CL. With your feedback, I rewrote the comments. I hope this is clearer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I like it but please wait for drott. Thank you!
Hi Koji, thanks for the explanations, my comments below. https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const AtomicString& FontCache::systemFontFamily(float) { So you're saying you would like to move the replacement/resolution of system-ui from MatchNSFontFamily to here in order for use to get different FontPlatformData objects? https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp:61: mutableSystemFontFamily() = Let's DCHECK for a non zero-length familyName argument here and assign it. https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) Okay, thanks for your explanations in the other patch regarding the resource bundle localization of the font name and when this can fail. Instead of mimicking the behaviour on the browser side and trying to emulate that here and introducing a fallback to the Skia default font (which we don't do anywhere else so far, I believe), can we just change the browser side code to always set a name? If the name is not localized then I'd suggest to get the default SkTypeface on the browser side, call SkTypeface::getFamilyName and pass that on, so that we don't have to special case on the Blink side and we'll always get exactly what the browser side uses for the UI font. This way we can remove the #if OS() switch here as well.
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 the review drott@, and thank you eae@ for sharing your thoughts. Replies inline: https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const AtomicString& FontCache::systemFontFamily(float) { On 2016/10/13 at 07:59:34, drott wrote: > So you're saying you would like to move the replacement/resolution of system-ui from MatchNSFontFamily to here in order for use to get different FontPlatformData objects? Yes, that way we can keep one SkTypeface/SimpleFontData/hb to map to single OpenType font. It's not working really well yet, I need more your help on this (will send separate e-mail) but I hope the direction is right? https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp:61: mutableSystemFontFamily() = On 2016/10/13 at 07:59:35, drott wrote: > Let's DCHECK for a non zero-length familyName argument here and assign it. Done. https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) On 2016/10/13 at 07:59:35, drott wrote: > Okay, thanks for your explanations in the other patch regarding the resource bundle localization of the font name and when this can fail. > > Instead of mimicking the behaviour on the browser side and trying to emulate that here and introducing a fallback to the Skia default font (which we don't do anywhere else so far, I believe), can we just change the browser side code to always set a name? > > If the name is not localized then I'd suggest to get the default SkTypeface on the browser side, call SkTypeface::getFamilyName and pass that on, so that we don't have to special case on the Blink side and we'll always get exactly what the browser side uses for the UI font. > > This way we can remove the #if OS() switch here as well. Done in Blink. Sorry touching UI in browser is too far beyond my current knowledge, hope this works for you.
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 #17 (id:350001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const AtomicString& FontCache::systemFontFamily(float) { On 2016/10/13 at 09:03:20, kojii wrote: > Yes, that way we can keep one SkTypeface/SimpleFontData/hb to map to single OpenType font. It's not working really well yet, I need more your help on this (will send separate e-mail) but I hope the direction is right? Yes, sounds reasonable. https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) I'm reluctant to fallback to Skia's default font name here, since we can avoid it and have this always exactly match what's used on the browser side. In https://codereview.chromium.org/2138613002 line 173 you're retrieving the font from the resource bundle. I've tried finding it, but where is the code that determines the primary UI font on the browser side? IIUC you said that the browser side prefers the font from the localized resource bundle, and if that one is not used, Skia's default is used. How is the primary UI font accessed in the browser? Instead of this change in line 173 in your other CL, I'd prefer a situation where we always set this to exactly what is used on the Browser side. We can retrieve Chrome's final UI font same as the UI code does it, and extract the family name from there. This keeps things consistent and we don't have a somewhat magic fallback to a default font name that can change below Blink.
https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) On 2016/10/13 at 12:36:13, drott wrote: > I'm reluctant to fallback to Skia's default font name here, since we can avoid it and have this always exactly match what's used on the browser side. In https://codereview.chromium.org/2138613002 line 173 you're retrieving the font from the resource bundle. I've tried finding it, but where is the code that determines the primary UI font on the browser side? IIUC you said that the browser side prefers the font from the localized resource bundle, and if that one is not used, Skia's default is used. How is the primary UI font accessed in the browser? Instead of this change in line 173 in your other CL, I'd prefer a situation where we always set this to exactly what is used on the Browser side. We can retrieve Chrome's final UI font same as the UI code does it, and extract the family name from there. This keeps things consistent and we don't have a somewhat magic fallback to a default font name that can change below Blink. Not only it's a lot of work for me, but also I don't think it is a good idea. Linux uses MenuConfig in TOOLKIT_VIEWS, but CrOS uses something different. I consulted with oshima@ and he recommended to use ResourceBundle instead, so here we are. Please see the doc, Linux/ChromeOS section: https://docs.google.com/document/d/1BI0OiWRUvsBOuPxPlF5J-_xtUZ49eVDUEXZXoF32Z... Even if we wrote two separate code each for Linux and ChromeOS, I'm not sure we can retrieve the SkTypeface from the menu control; I don't know TOOLKIT_VIEWS at all and whatever menu system CrOS uses at all either, but generally UI controls encapsulate the font instance and other such impl-specific details. MenuConfig is there, AFAIU, so that callers do not have to deal with the font instance, no?
https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) In the document you're saying: "ResourceBundle::GetSharedInstance().GetFontWithDelta(0).GetFontName() can get the system font name on all platforms. There are some tricky points: On Linux, this is set to empty. Instead, Chromium relies on Skia’s capability to find the default font by giving a null family name." Could you point me to the code in Skia that does figure out a different default font than Arial? I don't find it. Perhaps Chrome instead relies on specifying an empty font family name to gfx::FontList, but I don't think Skia has this logic. https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re... seems to initialize the defaults for gfx::FontList. My Chromium open source and my Chrome official build on Linux - if I identify the font right - use "Ubuntu" as the menu/system font. Perhaps I am misunderstanding something, but where is this coming from if the ResourceBundle is empty? Our last resort on Linux is "Sans", which maps to "DejaVu Sans" on my Ubuntu Fontconfig. I believe ChromeOS uses Arimo as the system font. linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt has Arial, which does not make sense to me in this context. Or is this temporary until the next CL - what will this result be after https://codereview.chromium.org/2138613002 ? This is the reason I don't want to fallback to Skia's nullptr-initialized SkTypeface here. How does Chrome figure out the system font - perhaps just a default constructed gfx::FontList (after it's been initialised through resource bundle) does the trick? In that case, using this in the upcoming CL for setting the system font on Linux and extracting the family name of the first font in that list, and setting this to the WebPreferences might be the solution?
https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) On 2016/10/13 at 16:45:13, drott wrote: > In the document you're saying: > > "ResourceBundle::GetSharedInstance().GetFontWithDelta(0).GetFontName() can get the system font name on all platforms. > There are some tricky points: > On Linux, this is set to empty. Instead, Chromium relies on Skia’s capability to find the default font by giving a null family name." > > Could you point me to the code in Skia that does figure out a different default font than Arial? I don't find it. Perhaps Chrome instead relies on specifying an empty font family name to gfx::FontList, but I don't think Skia has this logic. > > https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re... > seems to initialize the defaults for gfx::FontList. Can you elaborate your question more? On Linux, there are 3 SkFontMgr's for FontConfig, I admit I don't understand the relationship very well, but SkFontConfig_direct always returns Arial. Is this what you're talking about? SkFontMgr_fontconfig has a switch when familyName is null: https://cs.chromium.org/chromium/src/third_party/skia/src/ports/SkFontMgr_fon... but it goes to Fc* API, so hard to trace further. Android is easier to understand "nullptr" maps to the default font: https://cs.chromium.org/chromium/src/third_party/skia/src/ports/SkFontMgr_and... > My Chromium open source and my Chrome official build on Linux - if I identify the font right - use "Ubuntu" as the menu/system font. Perhaps I am misunderstanding something, but where is this coming from if the ResourceBundle is empty? > > Our last resort on Linux is "Sans", which maps to "DejaVu Sans" on my Ubuntu Fontconfig. I believe ChromeOS uses Arimo as the system font. linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt has Arial, which does not make sense to me in this context. Or is this temporary until the next CL - what will this result be after https://codereview.chromium.org/2138613002 ? There are two factors that prevents "-expected" to be the same font as you see on your Linux: 1. defaultFontFamily() in this CL returns "Arial" with --run-layout-test flag but "DejaVu Sans" without the flag. I believe this is to avoid test results being flaky by different lang or resources, but I can't explain why more. 2. After the resource CL landed, we look at resources, but content-shell may load different resources. See render_view_test.cc in the CL; it loads "content_shell.pak" for CrOS since the content-shell.pak doesn't seem to have it. So when the next CL lands, rendering will change. On CrOS, it'll be to IDS_UI_FONT_FAMILY_CROS (which looks like it is Roboto to me,) but "-expected" might not reflect it. > This is the reason I don't want to fallback to Skia's nullptr-initialized SkTypeface here. True that I haven't really understood all the code paths on Linux/CrOS including inside of FontConfig, but the part I've read so far looks nullptr returns the Skia default font. There's another alternative. In order to support "font: menu" etc. in CSS2/3: https://drafts.csswg.org/css-fonts-3/#propdef-font we already have LayoutThemeFontProvider::defaultGUIFont(): https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... This always returns "Arial" on Linux/CrOS. If you prefer this on Linux/CrOS, I'm fine with this. > How does Chrome figure out the system font - perhaps just a default constructed gfx::FontList (after it's been initialised through resource bundle) does the trick? In that case, using this in the upcoming CL for setting the system font on Linux and extracting the family name of the first font in that list, and setting this to the WebPreferences might be the solution? I don't know all the code paths yet, it's quite too much work and overkill I think, but in summary: 1. Linux uses MenuConfig. Not sure if this comes from IDS_UI_FONT_FAMILY (set to "default" in .grd/.xtb file) or kept empty intentionally, but local content-shell/chromium has empty. True not all code paths were really identified. 2. According to oshima@, ResourceBundle is how CrOS determines the system font, and this is empty on Linux. 3. Blink hardcodes "font: menu" to "Arial" on all Linux flavors today. So I think this is good enough to start and look for feedback if any, but the same method as "font: menu" is also fine with me. That might be safer, since localization-dependent bug is harder to be found before stable.
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...
Patchset #19 (id:410001) has been deleted
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Patchset #19 (id:430001) has been deleted
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.
https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right): https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN) On 2016/10/14 at 04:40:19, kojii wrote: > On 2016/10/13 at 16:45:13, drott wrote: > > In the document you're saying: > > > > "ResourceBundle::GetSharedInstance().GetFontWithDelta(0).GetFontName() can get the system font name on all platforms. > > There are some tricky points: > > On Linux, this is set to empty. Instead, Chromium relies on Skia’s capability to find the default font by giving a null family name." > > > > Could you point me to the code in Skia that does figure out a different default font than Arial? I don't find it. Perhaps Chrome instead relies on specifying an empty font family name to gfx::FontList, but I don't think Skia has this logic. > > > > https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re... > > seems to initialize the defaults for gfx::FontList. > > Can you elaborate your question more? The phrasing in the document sounds to me like Skia would do anything smart to figure out the _system_ default font. I don't think it does. > On Linux, there are 3 SkFontMgr's for FontConfig, I admit I don't understand the relationship very well, but SkFontConfig_direct always returns Arial. Is this what you're talking about? Yes. > > My Chromium open source and my Chrome official build on Linux - if I identify the font right - use "Ubuntu" as the menu/system font. Perhaps I am misunderstanding something, but where is this coming from if the ResourceBundle is empty? Could you try to answer this question, please? > > Our last resort on Linux is "Sans", which maps to "DejaVu Sans" on my Ubuntu Fontconfig. I believe ChromeOS uses Arimo as the system font. linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt has Arial, which does not make sense to me in this context. Or is this temporary until the next CL - what will this result be after https://codereview.chromium.org/2138613002 ? > > There are two factors that prevents "-expected" to be the same font as you see on your Linux: > 1. defaultFontFamily() in this CL returns "Arial" with --run-layout-test flag but "DejaVu Sans" without the flag. I believe this is to avoid test results being flaky by different lang or resources, but I can't explain why more. > 2. After the resource CL landed, we look at resources, but content-shell may load different resources. See render_view_test.cc in the CL; it loads "content_shell.pak" for CrOS since the content-shell.pak doesn't seem to have it. It's okay to have something that does not match the system font when running tests or content shell, but the UI case should work in all languages, so independent of when the ResourceBundle for a language is empty. > > This is the reason I don't want to fallback to Skia's nullptr-initialized SkTypeface here. > > True that I haven't really understood all the code paths on Linux/CrOS including inside of FontConfig, but the part I've read so far looks nullptr returns the Skia default font. Yes, but the Skia default font is not what we want since it's not the system font. > There's another alternative. In order to support "font: menu" etc. in CSS2/3: > https://drafts.csswg.org/css-fonts-3/#propdef-font > > we already have LayoutThemeFontProvider::defaultGUIFont(): > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > This always returns "Arial" on Linux/CrOS. If you prefer this on Linux/CrOS, I'm fine with this. Same here, this is not the real system font. > > How does Chrome figure out the system font - perhaps just a default constructed gfx::FontList (after it's been initialised through resource bundle) does the trick? In that case, using this in the upcoming CL for setting the system font on Linux and extracting the family name of the first font in that list, and setting this to the WebPreferences might be the solution? > > I don't know all the code paths yet, it's quite too much work and overkill I think, but in summary: > > 1. Linux uses MenuConfig. Not sure if this comes from IDS_UI_FONT_FAMILY (set to "default" in .grd/.xtb file) or kept empty intentionally, but local content-shell/chromium has empty. True not all code paths were really identified. If MenuConfig works, then this can be used in the upcoming CL to set this value in Linux, can't it? > 2. According to oshima@, ResourceBundle is how CrOS determines the system font, and this is empty on Linux. Does this work for languages where the ResourceBundle does not explicitly set this value? > So I think this is good enough to start and look for feedback if any, but the same method as "font: menu" is also fine with me. That might be safer, since localization-dependent bug is harder to be found before stable. I'd be much in favour of completing the story before we land this. The way I see it: 1) The system font family name on Linux and CrOS should always come from outside Blink and be set through WebFontrendering and not allow an empty value. Falling back to a Skia default in the Blink code, always Arial as much as I can tell, does not have much to do with the concept of system-ui. 2) On Linux, MenuConfig seems to be a promising candidate to set this to WebPreferences. 3) On CrOS, ResourceBundle seems to be a good candidate, but how about languages where the resource files don't mention a font name explicitly? How does gfx::FontList know about the system font? 4) For testing: It's okay to initialise WebFontRendering's setting with something hard-coded that's part of the set of fonts that content shell on Linux loads ( https://cs.chromium.org/chromium/src/content/shell/app/blink_test_platform_su... )
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: Exceeded global retry quota
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...
Description was changed from ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the localized system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. 3. On Mac, expand "system-ui" to underlying name by size. This is expected to improve http://crbug.com/627609 . 4. On Mac, expand "system-ui" to underlying cascading list. This is more complex than other items and is expected to take longer. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ========== to ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the localized system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. 3. On Mac, expand "system-ui" to underlying cascading list. This is more complex than other items and is expected to take longer. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ==========
Description was changed from ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the localized system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. 3. On Mac, expand "system-ui" to underlying cascading list. This is more complex than other items and is expected to take longer. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ========== to ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the localized system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. All our GV discussion reflected, and the following Linux patch[1] looks good.
LGTM now, with a nit on the nested #ifdefs. Thanks for updating the CL.
On 2016/10/18 at 07:48:15, drott wrote: > LGTM now, with a nit on the nested #ifdefs. Thanks for updating the CL. Thank you for your prompt review. I can't find the nits...is it about adding comments like this: #endif // !OS(MACOSX) ?
> I can't find the nits...is it about adding comments like this: > #endif // !OS(MACOSX) > ? Sorry, I somehow replied instead of posting my comment, it seems. Hope this includes them now. https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt (right): https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3: "Times New Roman" : 37 Curious: Are we going to set this to something else for testing, in a subsequent CL? https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:124: #else Nit perhaps, but I am considering how we could make this more readable and remove the #ifdef nesting here. Could you move this block up, first, and use #ifdef OS(MACOSX), then just return getFontPlatformData, then #endif, then write the rest? I think that would avoid the nesting.
https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt (right): https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3: "Times New Roman" : 37 On 2016/10/18 at 10:16:29, drott wrote: > Curious: Are we going to set this to something else for testing, in a subsequent CL? Yes and no. It used to be the Skia default font, so Arial. In PS22, we handle "system-ui" as not found, and we fallback to the document default font. Chrome will set correctly in a subsequent CL, but content-shell/run-layout-test will not, so this -expected file will not change, but you will see the correct font in Chrome. https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:124: #else On 2016/10/18 at 10:16:30, drott wrote: > Nit perhaps, but I am considering how we could make this more readable and remove the #ifdef nesting here. Could you move this block up, first, and use #ifdef OS(MACOSX), then just return getFontPlatformData, then #endif, then write the rest? I think that would avoid the nesting. Ok, it's still nesting IIUC but nest in #else might be better.
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...
nit done. https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right): https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/fonts/FontCache.cpp:124: #else On 2016/10/18 at 10:37:35, kojii wrote: > On 2016/10/18 at 10:16:30, drott wrote: > > Nit perhaps, but I am considering how we could make this more readable and remove the #ifdef nesting here. Could you move this block up, first, and use #ifdef OS(MACOSX), then just return getFontPlatformData, then #endif, then write the rest? I think that would avoid the nesting. > > Ok, it's still nesting IIUC but nest in #else might be better. Note, this part will need some re-write, since when we add UMA, this function doesn't have Document. I'm still seeking for the best place to convert BlinkMacSystemFont in a following CL.
On 2016/10/18 at 10:37:36, kojii wrote: > https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt (right): > > https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3: "Times New Roman" : 37 > On 2016/10/18 at 10:16:29, drott wrote: > > Curious: Are we going to set this to something else for testing, in a subsequent CL? > > Yes and no. It used to be the Skia default font, so Arial. In PS22, we handle "system-ui" as not found, and we fallback to the document default font. > > Chrome will set correctly in a subsequent CL, but content-shell/run-layout-test will not, so this -expected file will not change, but you will see the correct font in Chrome. Okay, thanks. Are we not setting WebFontRendering preferences in testing at all? Or are we just not setting the system font? In the latter case, perhaps we could set it to one of the fonts loaded in fontconfig_util_linux just to make it different from Times in testing, but that's a minor thing.
On 2016/10/18 at 10:42:41, kojii wrote: > Note, this part will need some re-write, since when we add UMA, this function doesn't have Document. I'm still seeking for the best place to convert BlinkMacSystemFont in a following CL. What do you plan to measure with UMA, BlinkMacSystemFont usage? And you need the document for UMA? You can perhaps check HarfBuzzFace zeroCopySuccessHistogram, I am doing a BooleanHistogram there without a document, if that helps.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Let's address these questions in following CLs. Having a code should help our discussions.
On 2016/10/18 at 14:01:45, kojii wrote: > Let's address these questions in following CLs. Having a code should help our discussions. Yes, sure. This CL is good to go from my point of view, as LGTM'ed in #98.
The CQ bit was checked by kojii@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the localized system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ========== to ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the current system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:530001)
Message was sent while issue was closed.
Description was changed from ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the current system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 ========== to ========== Enable "system-ui" generic font family This patch enables the "system-ui" generic font family on all platforms. Following changes are planned but not included in this CL: 1. Content sets the current system font to Blink on Linux/CrOS[1]. 2. Remove internal use of "BlinkMacSystemFont" and add UMA[2]. [1] https://codereview.chromium.org/2138613002 [2] https://codereview.chromium.org/2388623002 BUG=654679 Committed: https://crrev.com/bb3db4f221492076bca16af4d6fb9f358b79cedb Cr-Commit-Position: refs/heads/master@{#425967} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/bb3db4f221492076bca16af4d6fb9f358b79cedb Cr-Commit-Position: refs/heads/master@{#425967} |