Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1317833003:
Fix fallback glyph lookup (Closed)
|
Created:
5 years, 3 months ago by kochi Modified:
5 years, 3 months ago CC:
blink-reviews, krit, drott+blinkwatch_chromium.org, Rik, dshwang, jbroman, Justin Novosad, danakj, pdr+graphicswatchlist_chromium.org, f(malita), Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix fallback glyph lookup
On Japanese locale Android which has Motoya Maruberi
as the system default font (up until 5.1), the font
fallback path mistakenly picked up ".notdef" glyph
for (U+00A0).
This happened because in the fallback path it still
ended up in looking up non-existent glyph in the same
font file and fell back to the "missingGlyphData".
This is because Blink looks up the glyph in
normalized codepoint (for U+00A0, normalized to U+0020)
but still it tries to look up the original codepoint.
This bug was exposed by the following change, to make
not to normalize to space before looking up glyph.
https://codereview.chromium.org/705163003
BUG=454108
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201348
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix pageNumber for characterToRender and caching glyph #
Total comments: 1
Patch Set 3 : fix nit #Messages
Total messages: 22 (5 generated)
kochi@chromium.org changed reviewers: + eae@chromium.org, kojii@chromium.org
This works for me. I'm not sure in the fallback path, |c| is used and not |characterToRender| is used. It might be intentional and I may well be wrong. https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.cpp File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.... Source/platform/fonts/Font.cpp:516: characterToRender = Character::normalizeSpaces(characterToRender); is translated to U+00A0, but at this point it's converted back to 0x20. After this line, c = 0xa0 characterToRender = 0x20 https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.... Source/platform/fonts/Font.cpp:528: GlyphData data = fallbackPage && fallbackPage->glyphForCharacter(characterToRender) ? fallbackPage->glyphDataForCharacter(characterToRender) : characterFontData->missingGlyphData(); On this line, fallbackPage->glyphForCharacter(0xa0) fails as the font does not have the glyph for it, so it falls back to characterFontData->missingGlyphData() path.
https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.cpp File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.... Source/platform/fonts/Font.cpp:528: GlyphData data = fallbackPage && fallbackPage->glyphForCharacter(characterToRender) ? fallbackPage->glyphDataForCharacter(characterToRender) : characterFontData->missingGlyphData(); On 2015/08/26 12:02:59, Takayoshi Kochi wrote: > On this line, fallbackPage->glyphForCharacter(0xa0) fails as the font does not > have the glyph for it, so it falls back to characterFontData->missingGlyphData() > path. Yeah, this code path looks ok, so the caller should get missingGlyphData, which is glyph == 0. When Font returns glyph == 0, why Linux does not render the glyph while Android renders it is still a mystery. What are the CSS in the reproducing pages? Do they specify the font family, 'serif', or nothing?
Interesting, thanks for looking into this. Seems reasonable to me.
Hm, I thought this patch might undo the effect of http://crbug.com/423539, but maybe I overlooked?
I think it will.
https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.cpp File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/1317833003/diff/1/Source/platform/fonts/Font.... Source/platform/fonts/Font.cpp:528: GlyphData data = fallbackPage && fallbackPage->glyphForCharacter(characterToRender) ? fallbackPage->glyphDataForCharacter(characterToRender) : characterFontData->missingGlyphData(); On 2015/08/26 13:45:09, kojii wrote: > On 2015/08/26 12:02:59, Takayoshi Kochi wrote: > > On this line, fallbackPage->glyphForCharacter(0xa0) fails as the font does not > > have the glyph for it, so it falls back to > characterFontData->missingGlyphData() > > path. > > Yeah, this code path looks ok, so the caller should get missingGlyphData, which > is glyph == 0. > > When Font returns glyph == 0, why Linux does not render the glyph while Android > renders it is still a mystery. > > What are the CSS in the reproducing pages? Do they specify the font family, > 'serif', or nothing? I think on Linux fallbackPage->glyphForCharacter(c) succeeds, and missingGlyphData() is never called (of course, it depends on the fontconfig settings, though). On Android (JA locale), fallbackPage also contains MotoyaLMaru and if it's given 0xa0, lookup fails again (this is hypothesis, needs verification).
On 2015/08/26 15:49:26, eae wrote: > I think it will. No - if the glyph for U+00A0 is found above my change, it is used. This change only affects fallback path, when looking up glyph in the primary font failed. In the fallback path, it calls Character::normalizeSpaces(characterToRender) (I don't know the real intention, but anyway) and U+00A0 is normalized to U+0020, then lookup fallback font that can provide glyph for *U+0020*, but then, why does the code try to get glyph for *U+00A0* again??
Ah, right, this is already in the system font fallback code. Though, I'm not
certain if scripts that wants to render ngsp glyphs want the behavior in system
fallback too or not...
What about:
if (c has the glyph)
use it;
+ else if (characterToRender has the glyph)
+ use it;
else
missingGlyphData();
The current logic, getting the font for characterToRender then getting glyph for
c from the font, looks suspicious, so your fix might be right. I failed to track
git log why we do this due to several file moves, but this looks safer to me.
WDYT?
The CQ bit was checked by kochi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1317833003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1317833003/20001
On 2015/08/27 02:25:42, kojii wrote: > Ah, right, this is already in the system font fallback code. Though, I'm not > certain if scripts that wants to render ngsp glyphs want the behavior in system > fallback too or not... > > What about: > if (c has the glyph) > use it; > + else if (characterToRender has the glyph) > + use it; > else > missingGlyphData(); Hmm, then in the fast path loop, if glyph lookup for |c| fails, always lookup |characterToRender| if |c != characterToRender|? It may make sense, but I'm wondering if it is worth the cost (not measured though). > The current logic, getting the font for characterToRender then getting glyph for > c from the font, looks suspicious, so your fix might be right. I failed to track > git log why we do this due to several file moves, but this looks safer to me. > WDYT? I tried digging the history as much as possible, but the code historically has done the same. Normalization was added in the following: For zero width space: a02a93162a46094fd504206943c88da13082d13f For normal space: dc1dc5c81ef2f7ea63b77624ec20ee5b88092cc9 Even in the latter, it seems to me that the code retrieves FontData* from normalized codepoint, but still gets the glyph from the original codepoint. I'll see if any test fails, and if all look good, I'd like to go with patchset2.
On 2015/08/27 05:23:12, Takayoshi Kochi wrote: > On 2015/08/27 02:25:42, kojii wrote: > > Ah, right, this is already in the system font fallback code. Though, I'm not > > certain if scripts that wants to render ngsp glyphs want the behavior in > system > > fallback too or not... > > > > What about: > > if (c has the glyph) > > use it; > > + else if (characterToRender has the glyph) > > + use it; > > else > > missingGlyphData(); > > Hmm, then in the fast path loop, if glyph lookup for |c| fails, > always lookup |characterToRender| if |c != characterToRender|? > It may make sense, but I'm wondering if it is worth the cost > (not measured though). Oops, obviously it can be done outside the loop.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
non-owner LGTM
LGTM w/nit https://codereview.chromium.org/1317833003/diff/20001/Source/platform/fonts/F... File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/1317833003/diff/20001/Source/platform/fonts/F... Source/platform/fonts/Font.cpp:527: unsigned pageNumberForRendering = (characterToRender / GlyphPage::size); Remove unnecessary parentheses.
The CQ bit was checked by kochi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org, eae@chromium.org Link to the patchset: https://codereview.chromium.org/1317833003/#ps40001 (title: "fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1317833003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1317833003/40001
? ? ? Hi, I'm currently on leave. For urgent matters, please contact my manager Vivek Hebbar. Thanks! Rik
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201348 |
