Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(57)

Issue 2863693002: Fallback Font Cache and Character Hinting for VRShell (Closed)

Created:
3 years, 7 months ago by acondor_
Modified:
3 years, 7 months ago
CC:
chromium-reviews, feature-vr-reviews_chromium.org, derat+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+197 lines, -25 lines) Patch
M chrome/browser/android/vr_shell/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A chrome/browser/android/vr_shell/font_fallback.h View 1 2 3 4 5 6 7 1 chunk +33 lines, -0 lines 0 comments Download
A chrome/browser/android/vr_shell/font_fallback.cc View 1 2 3 4 5 6 7 8 9 1 chunk +146 lines, -0 lines 0 comments Download
M chrome/browser/android/vr_shell/textures/ui_texture.cc View 1 2 3 4 5 6 7 8 3 chunks +13 lines, -21 lines 0 comments Download
M ui/gfx/platform_font_linux.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -3 lines 0 comments Download
M ui/gfx/platform_font_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 45 (21 generated)
acondor_
PTAL before reaching ui/gfx OWNERS.
3 years, 7 months ago (2017-05-04 18:25:18 UTC) #2
mthiesse
As discussed offline, we're going to try using ICU's uscript_getScript instead of generating hit characters.
3 years, 7 months ago (2017-05-04 19:28:43 UTC) #4
mthiesse
On 2017/05/04 19:28:43, mthiesse wrote: > As discussed offline, we're going to try using ICU's ...
3 years, 7 months ago (2017-05-04 19:28:55 UTC) #5
acondor_
ICU was introduced. PTAL.
3 years, 7 months ago (2017-05-05 20:48:05 UTC) #7
mthiesse
https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_android.cc File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_android.cc#newcode34 ui/gfx/font_fallback_android.cc:34: base::LazyInstance<ScriptCharMap>::Leaky g_script_char_map = Doesn't look like you need this ...
3 years, 7 months ago (2017-05-08 14:46:35 UTC) #8
acondor_
https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_android.cc File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/20001/ui/gfx/font_fallback_android.cc#newcode34 ui/gfx/font_fallback_android.cc:34: base::LazyInstance<ScriptCharMap>::Leaky g_script_char_map = On 2017/05/08 14:46:35, mthiesse wrote: > ...
3 years, 7 months ago (2017-05-08 16:55:29 UTC) #9
mthiesse
lgtm https://codereview.chromium.org/2863693002/diff/60001/ui/gfx/font_fallback_android.cc File ui/gfx/font_fallback_android.cc (right): https://codereview.chromium.org/2863693002/diff/60001/ui/gfx/font_fallback_android.cc#newcode39 ui/gfx/font_fallback_android.cc:39: // scripts (174). nit: The 174 number will ...
3 years, 7 months ago (2017-05-08 17:09:18 UTC) #10
acondor_
+sadrul PTAL at ui/gfx
3 years, 7 months ago (2017-05-08 17:19:29 UTC) #12
sadrul
font -> asvitkine@
3 years, 7 months ago (2017-05-09 05:02:32 UTC) #14
Alexei Svitkine (slow)
https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android/vr_shell/textures/ui_texture.cc File chrome/browser/android/vr_shell/textures/ui_texture.cc (right): https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android/vr_shell/textures/ui_texture.cc#newcode25 chrome/browser/android/vr_shell/textures/ui_texture.cc:25: namespace { Nit: Add an empty line after this ...
3 years, 7 months ago (2017-05-09 14:52:17 UTC) #15
acondor_
https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android/vr_shell/textures/ui_texture.cc File chrome/browser/android/vr_shell/textures/ui_texture.cc (right): https://codereview.chromium.org/2863693002/diff/100001/chrome/browser/android/vr_shell/textures/ui_texture.cc#newcode25 chrome/browser/android/vr_shell/textures/ui_texture.cc:25: namespace { On 2017/05/09 14:52:16, Alexei Svitkine (slow) wrote: ...
3 years, 7 months ago (2017-05-09 16:21:52 UTC) #16
Alexei Svitkine (slow)
https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_android.h File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_android.h#newcode25 ui/gfx/font_fallback_android.h:25: GFX_EXPORT std::string GetFallbackFontNameForChar( On 2017/05/09 16:21:51, acondor wrote: > ...
3 years, 7 months ago (2017-05-09 16:40:42 UTC) #17
acondor_
https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_android.h File ui/gfx/font_fallback_android.h (right): https://codereview.chromium.org/2863693002/diff/100001/ui/gfx/font_fallback_android.h#newcode25 ui/gfx/font_fallback_android.h:25: GFX_EXPORT std::string GetFallbackFontNameForChar( On 2017/05/09 16:40:41, Alexei Svitkine (slow) ...
3 years, 7 months ago (2017-05-09 18:44:16 UTC) #18
Alexei Svitkine (slow)
lgtm
3 years, 7 months ago (2017-05-09 18:49:37 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863693002/140001
3 years, 7 months ago (2017-05-09 18:53:36 UTC) #22
commit-bot: I haz the power
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_arm64_dbg_recipe/builds/264072) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, ...
3 years, 7 months ago (2017-05-09 19:00:32 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863693002/160001
3 years, 7 months ago (2017-05-09 20:00:57 UTC) #27
commit-bot: I haz the power
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_swarming_rel/builds/174101)
3 years, 7 months ago (2017-05-09 21:08:19 UTC) #29
acondor_
mthiesse, please have a second look to the runtime error fix. Apparently SkString::c_str is not ...
3 years, 7 months ago (2017-05-10 15:13:42 UTC) #33
mthiesse
lgtm https://codereview.chromium.org/2863693002/diff/180001/ui/gfx/platform_font_linux.h File ui/gfx/platform_font_linux.h (right): https://codereview.chromium.org/2863693002/diff/180001/ui/gfx/platform_font_linux.h#newcode56 ui/gfx/platform_font_linux.h:56: sk_sp<SkTypeface> typeface() const { return std::ref(typeface_); } remove ...
3 years, 7 months ago (2017-05-10 15:38:00 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863693002/200001
3 years, 7 months ago (2017-05-10 16:07:23 UTC) #37
commit-bot: I haz the power
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_chromium_compile_only_ng/builds/336203) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 7 months ago (2017-05-10 16:27:20 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2863693002/220001
3 years, 7 months ago (2017-05-10 17:52:24 UTC) #42
commit-bot: I haz the power
3 years, 7 months ago (2017-05-10 21:09:39 UTC) #45
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/03c0b57bb3900cb5f1f481163a40...

Powered by Google App Engine
This is Rietveld 408576698