|
|
DescriptionFallback Font Cache and Character Hinting for VRShell
- An Android-only font cache was implemented, mimiking Linux's.
- In order to increase cache hits, we use ICU scripts and only query one character per script.
- Only get a fallback font if the main font is not able to render the character. And always check if previous fallback fonts already support the character. This change solves the issue of extra line spacing that was being introduced by including extra (unnecessary) fallback fonts.
BUG=715591
Review-Url: https://codereview.chromium.org/2863693002
Cr-Commit-Position: refs/heads/master@{#470706}
Committed: https://chromium.googlesource.com/chromium/src/+/03c0b57bb3900cb5f1f481163a40a038e2498f39
Patch Set 1 #Patch Set 2 : using ICU script to increase cache hits #
Total comments: 6
Patch Set 3 : Removing unnecessary conversion to vector #Patch Set 4 : script based cache #
Total comments: 1
Patch Set 5 : improving comment #Patch Set 6 : bugfix on UScriptCode #
Total comments: 16
Patch Set 7 : addressing nit #Patch Set 8 : Moving font fallback search to vr_shell #Patch Set 9 : Rebase #Patch Set 10 : fix runtime error #
Total comments: 1
Patch Set 11 : removing ref #Patch Set 12 : SkTypeface complete type #
Messages
Total messages: 45 (21 generated)
acondor@google.com changed reviewers: + mthiesse@chromium.org, vollick@chromium.org
PTAL before reaching ui/gfx OWNERS.
Description was changed from ========== Fallback Font Cache and Character Hinting for VRShell - An Android-only font cache was implemented, mimiking Linux's. - A set of characters are selected to hint the fallback fonts, given that character sets are continuous in Unicode. BUG=715591 ========== to ========== Fallback Font Cache and Character Hinting for VRShell - An Android-only font cache was implemented, mimiking Linux's. - A set of characters are selected to hint the fallback fonts, given that character sets are continuous in Unicode. - Additionally, this change solves the issue of extra line spacing that was being introduced by including extra (unnecessary) fallback fonts. BUG=715591 ==========
As discussed offline, we're going to try using ICU's uscript_getScript instead of generating hit characters.
On 2017/05/04 19:28:43, mthiesse wrote: > As discussed offline, we're going to try using ICU's uscript_getScript instead > of generating hit characters. s/hit/hint
Description was changed from ========== Fallback Font Cache and Character Hinting for VRShell - An Android-only font cache was implemented, mimiking Linux's. - A set of characters are selected to hint the fallback fonts, given that character sets are continuous in Unicode. - Additionally, this change solves the issue of extra line spacing that was being introduced by including extra (unnecessary) fallback fonts. BUG=715591 ========== to ========== Fallback Font Cache and Character Hinting for VRShell - An Android-only font cache was implemented, mimiking Linux's. - In order to increase cache hits, we use ICU scripts and only query one character per script. - Only get a fallback font if the main font is not able to render the character. And always check if previous fallback fonts already support the character. This change solves the issue of extra line spacing that was being introduced by including extra (unnecessary) fallback fonts. BUG=715591 ==========
ICU was introduced. PTAL.
https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.cc:34: base::LazyInstance<ScriptCharMap>::Leaky g_script_char_map = Doesn't look like you need this map. Just change the supported_characters_ map type to <UScriptCode, KnownGlyph>. https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.cc:107: const char* bcp47_locales[kMaxLocales]; I think this code would be more readable if you got rid of bcp47_locales and kMaxLocales. ie. sk_sp<SkTypeface> tf(font_mgr->matchFamilyStyleCharacter(nullptr, SkFontStyle(), locale_.empty() ? nullptr : locale_.c_str(), locale_.empty() ? 0 : 1, c)); https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.h:25: const Font& font, nit: default_font? Also document this parameter above?
https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.cc:34: base::LazyInstance<ScriptCharMap>::Leaky g_script_char_map = On 2017/05/08 14:46:35, mthiesse wrote: > Doesn't look like you need this map. > > Just change the supported_characters_ map type to <UScriptCode, KnownGlyph>. Good catch. Done. https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.cc:107: const char* bcp47_locales[kMaxLocales]; On 2017/05/08 14:46:35, mthiesse wrote: > I think this code would be more readable if you got rid of bcp47_locales and > kMaxLocales. > > ie. > sk_sp<SkTypeface> tf(font_mgr->matchFamilyStyleCharacter(nullptr, SkFontStyle(), > locale_.empty() ? nullptr : locale_.c_str(), locale_.empty() ? 0 : 1, c)); The array is needed, but I got rid of the rest. https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.h:25: const Font& font, On 2017/05/08 14:46:35, mthiesse wrote: > nit: default_font? Also document this parameter above? Done.
lgtm https://codereview.chromium.org/2863693002/diff/60001/ui/gfx/font_fallback_an... File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/60001/ui/gfx/font_fallback_an... ui/gfx/font_fallback_android.cc:39: // scripts (174). nit: The 174 number will get stale, "174 at time of writing" would be better.
acondor@google.com changed reviewers: + sadrul@chromium.org
+sadrul PTAL at ui/gfx
sadrul@chromium.org changed reviewers: + asvitkine@chromium.org - sadrul@chromium.org
font -> asvitkine@
https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/textures/ui_texture.cc (right): https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/textures/ui_texture.cc:25: namespace { Nit: Add an empty line after this now that the block is non-trivial. https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/textures/ui_texture.cc:45: Nit: Remove extra unnecessary change https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:35: } Nit: Add an empty line after this. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:66: }; Nit: DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:74: ~CachedFontSet() = default; Nit: Add an empty line after this. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:113: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.h:25: GFX_EXPORT std::string GetFallbackFontNameForChar( If this is only used by vr_shell, why not put the implementation there?
https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android... File chrome/browser/android/vr_shell/textures/ui_texture.cc (right): https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/textures/ui_texture.cc:25: namespace { On 2017/05/09 14:52:16, Alexei Svitkine (slow) wrote: > Nit: Add an empty line after this now that the block is non-trivial. Done. https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android... chrome/browser/android/vr_shell/textures/ui_texture.cc:45: On 2017/05/09 14:52:16, Alexei Svitkine (slow) wrote: > Nit: Remove extra unnecessary change Done. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:35: } On 2017/05/09 14:52:17, Alexei Svitkine (slow) wrote: > Nit: Add an empty line after this. Done. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:66: }; On 2017/05/09 14:52:17, Alexei Svitkine (slow) wrote: > Nit: DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:74: ~CachedFontSet() = default; On 2017/05/09 14:52:16, Alexei Svitkine (slow) wrote: > Nit: Add an empty line after this. Done. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.cc:113: }; On 2017/05/09 14:52:17, Alexei Svitkine (slow) wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.h:25: GFX_EXPORT std::string GetFallbackFontNameForChar( On 2017/05/09 14:52:17, Alexei Svitkine (slow) wrote: > If this is only used by vr_shell, why not put the implementation there? Because this is platform dependent code and this seemed like the most appropriate place to put it for now. VR Shell is currently android-only, but it will be multi-platform in the future.
https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.h:25: GFX_EXPORT std::string GetFallbackFontNameForChar( On 2017/05/09 16:21:51, acondor wrote: > On 2017/05/09 14:52:17, Alexei Svitkine (slow) wrote: > > If this is only used by vr_shell, why not put the implementation there? > > Because this is platform dependent code and this seemed like the most > appropriate place to put it for now. VR Shell is currently android-only, but it > will be multi-platform in the future. Any directory in Chrome could have platform-specific code - by using the same convention you use here - i.e. suffixing the files with the platform name. I don't see that as an argument for this needing to be in gfx/. Usually code is placed at lower levels when it's used by multiple places - and it's fine to have code more local until then and to move it later if it later gets more than one users from other directories.
https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_a... ui/gfx/font_fallback_android.h:25: GFX_EXPORT std::string GetFallbackFontNameForChar( On 2017/05/09 16:40:41, Alexei Svitkine (slow) wrote: > On 2017/05/09 16:21:51, acondor wrote: > > On 2017/05/09 14:52:17, Alexei Svitkine (slow) wrote: > > > If this is only used by vr_shell, why not put the implementation there? > > > > Because this is platform dependent code and this seemed like the most > > appropriate place to put it for now. VR Shell is currently android-only, but > it > > will be multi-platform in the future. > > Any directory in Chrome could have platform-specific code - by using the same > convention you use here - i.e. suffixing the files with the platform name. I > don't see that as an argument for this needing to be in gfx/. > > Usually code is placed at lower levels when it's used by multiple places - and > it's fine to have code more local until then and to move it later if it later > gets more than one users from other directories. You are right. I moved the code to vr_shell namespace but kept out the _android suffix until we start implementing the Shell for other platforms.
lgtm
The CQ bit was checked by acondor@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2863693002/#ps140001 (title: "Moving font fallback search to vr_shell")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) 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 acondor@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2863693002/#ps160001 (title: "Rebase")
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
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_...)
The CQ bit was checked by acondor@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2863693002/#ps180001 (title: "fix runtime error")
The CQ bit was unchecked by acondor@google.com
mthiesse, please have a second look to the runtime error fix. Apparently SkString::c_str is not null terminated.
lgtm https://codereview.chromium.org/2863693002/diff/180001/ui/gfx/platform_font_l... File ui/gfx/platform_font_linux.h (right): https://codereview.chromium.org/2863693002/diff/180001/ui/gfx/platform_font_l... ui/gfx/platform_font_linux.h:56: sk_sp<SkTypeface> typeface() const { return std::ref(typeface_); } remove std::ref, it's not necessary.
The CQ bit was checked by acondor@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2863693002/#ps200001 (title: "removing ref")
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
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by acondor@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2863693002/#ps220001 (title: "SkTypeface complete type")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494438638175010, "parent_rev": "adacb0cb737686f68282011b4a77f50a21f9d6dd", "commit_rev": "03c0b57bb3900cb5f1f481163a40a038e2498f39"}
Message was sent while issue was closed.
Description was changed from ========== Fallback Font Cache and Character Hinting for VRShell - An Android-only font cache was implemented, mimiking Linux's. - In order to increase cache hits, we use ICU scripts and only query one character per script. - Only get a fallback font if the main font is not able to render the character. And always check if previous fallback fonts already support the character. This change solves the issue of extra line spacing that was being introduced by including extra (unnecessary) fallback fonts. BUG=715591 ========== to ========== Fallback Font Cache and Character Hinting for VRShell - An Android-only font cache was implemented, mimiking Linux's. - In order to increase cache hits, we use ICU scripts and only query one character per script. - Only get a fallback font if the main font is not able to render the character. And always check if previous fallback fonts already support the character. This change solves the issue of extra line spacing that was being introduced by including extra (unnecessary) fallback fonts. BUG=715591 Review-Url: https://codereview.chromium.org/2863693002 Cr-Commit-Position: refs/heads/master@{#470706} Committed: https://chromium.googlesource.com/chromium/src/+/03c0b57bb3900cb5f1f481163a40... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/03c0b57bb3900cb5f1f481163a40... |