Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
678683003:
Make RenderTextHarfBuzz examine full font list. (Closed)
|
Created:
6 years, 2 months ago by Daniel Erat Modified:
6 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMake RenderTextHarfBuzz examine full font list.
When evaluating which font to use for a run,
RenderTextHarfBuzz should examine all fonts in the font list
that was supplied to it. Previously, it only tried the
primary font before resorting to its fallback logic.
BUG=none
Committed: https://crrev.com/2e3cf521ff59c2a3f657a72af04892f4a5783d6a
Cr-Commit-Position: refs/heads/master@{#301284}
Patch Set 1 #
Total comments: 10
Patch Set 2 : avoid calling Draw() #Patch Set 3 : disable test on windows #
Messages
Total messages: 14 (2 generated)
derat@chromium.org changed reviewers: + ckocagil@chromium.org, msw@chromium.org
as suggested by mike at https://codereview.chromium.org/674683003/ https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2331: TEST_F(RenderTextTest, FontListFallback) { not sure yet whether this will work on all platforms. i may also need to change it to explicitly ask for RenderTextHarfBuzz.
Just some test nits/qs; thanks! https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2331: TEST_F(RenderTextTest, FontListFallback) { On 2014/10/24 15:43:23, Daniel Erat wrote: > not sure yet whether this will work on all platforms. i may also need to change > it to explicitly ask for RenderTextHarfBuzz. Acknowledged. https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2341: // "⊕" (CIRCLED PLUS) should be rendered with Symbol rather than falling back Can you actually include this unicode char in this (ASCII?) file? https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2347: render_text->Draw(&canvas); You shouldn't need to Draw, GetFontSpansForTesting will EnsureLayout. https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2351: EXPECT_EQ("Symbol", spans[0].first.GetFontName()); Does this actually fail without your change?
https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2341: // "⊕" (CIRCLED PLUS) should be rendered with Symbol rather than falling back On 2014/10/24 16:09:11, msw wrote: > Can you actually include this unicode char in this (ASCII?) file? the web is inconclusive here, but i think it's fine for us within comments. i only did it because i saw that an earlier test in this file includes "®" (REGISTERED SIGN) in a comment. :-P https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2347: render_text->Draw(&canvas); On 2014/10/24 16:09:11, msw wrote: > You shouldn't need to Draw, GetFontSpansForTesting will EnsureLayout. cool, thanks. done. https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2351: EXPECT_EQ("Symbol", spans[0].first.GetFontName()); On 2014/10/24 16:09:11, msw wrote: > Does this actually fail without your change? yep, wrote the test first. :-P at least on my gtrusty workstation, RTHB falls back to DejaVu Sans instead of using Symbol without my change.
lgtm
i might end up needing to make this test linux-only; it failed on the win_chromium_rel bot -- looks like Arial got used instead. maybe Arial actually covers this codepoint there? (the test's assertion that Symbol is available didn't fail, so presumably it's not like we used Arial for lack of any better alternatives...) http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/... RenderTextTest.FontListFallback (run #1): [ RUN ] RenderTextTest.FontListFallback e:\b\build\slave\win\build\src\ui\gfx\render_text_unittest.cc(2351): error: Value of: spans[0].first.GetFontName() Actual: "Arial" Expected: "Symbol" [ FAILED ] RenderTextTest.FontListFallback (22 ms) sounds like i can talk stevenjb@ into building this for windows for me so i can at least figure out what's going on, though.
LGTM (even with RTHB-only for now). On 2014/10/24 16:44:05, Daniel Erat wrote: > i might end up needing to make this test linux-only; it failed on the > win_chromium_rel bot -- looks like Arial got used instead. maybe Arial actually > covers this codepoint there? (the test's assertion that Symbol is available > didn't fail, so presumably it's not like we used Arial for lack of any better > alternatives...) > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/... > > RenderTextTest.FontListFallback (run #1): > [ RUN ] RenderTextTest.FontListFallback > e:\b\build\slave\win\build\src\ui\gfx\render_text_unittest.cc(2351): error: > Value of: spans[0].first.GetFontName() > Actual: "Arial" > Expected: "Symbol" > [ FAILED ] RenderTextTest.FontListFallback (22 ms) > > sounds like i can talk stevenjb@ into building this for windows for me so i can > at least figure out what's going on, though. Acknowledged. https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.cc File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2341: // "⊕" (CIRCLED PLUS) should be rendered with Symbol rather than falling back On 2014/10/24 16:25:47, Daniel Erat wrote: > On 2014/10/24 16:09:11, msw wrote: > > Can you actually include this unicode char in this (ASCII?) file? > > the web is inconclusive here, but i think it's fine for us within comments. i > only did it because i saw that an earlier test in this file includes "®" > (REGISTERED SIGN) in a comment. :-P Hmm, the style guide might suggest only UTF-8 and not UTF-16? https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#... Otherwise, if pre-submit and lint don't complain, this is good to know, and there are probably many other cases then where I've included the UTF-8 or escaped chars in comments, but an actual symbol would be so much more useful! https://codereview.chromium.org/678683003/diff/1/ui/gfx/render_text_unittest.... ui/gfx/render_text_unittest.cc:2351: EXPECT_EQ("Symbol", spans[0].first.GetFontName()); On 2014/10/24 16:25:47, Daniel Erat wrote: > On 2014/10/24 16:09:11, msw wrote: > > Does this actually fail without your change? > > yep, wrote the test first. :-P at least on my gtrusty workstation, RTHB falls > back to DejaVu Sans instead of using Symbol without my change. Acknowledged.
On 2014/10/24 16:44:05, Daniel Erat wrote: > i might end up needing to make this test linux-only; it failed on the > win_chromium_rel bot -- looks like Arial got used instead. maybe Arial actually > covers this codepoint there? (the test's assertion that Symbol is available > didn't fail, so presumably it's not like we used Arial for lack of any better > alternatives...) > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/... > > RenderTextTest.FontListFallback (run #1): > [ RUN ] RenderTextTest.FontListFallback > e:\b\build\slave\win\build\src\ui\gfx\render_text_unittest.cc(2351): error: > Value of: spans[0].first.GetFontName() > Actual: "Arial" > Expected: "Symbol" > [ FAILED ] RenderTextTest.FontListFallback (22 ms) > > sounds like i can talk stevenjb@ into building this for windows for me so i can > at least figure out what's going on, though. Looks related: https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/platform_fo... https://code.google.com/p/chromium/issues/detail?id=327287
On 2014/10/24 18:21:28, ckocagil wrote: > On 2014/10/24 16:44:05, Daniel Erat wrote: > > i might end up needing to make this test linux-only; it failed on the > > win_chromium_rel bot -- looks like Arial got used instead. maybe Arial > actually > > covers this codepoint there? (the test's assertion that Symbol is available > > didn't fail, so presumably it's not like we used Arial for lack of any better > > alternatives...) > > > > > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/... > > > > RenderTextTest.FontListFallback (run #1): > > [ RUN ] RenderTextTest.FontListFallback > > e:\b\build\slave\win\build\src\ui\gfx\render_text_unittest.cc(2351): error: > > Value of: spans[0].first.GetFontName() > > Actual: "Arial" > > Expected: "Symbol" > > [ FAILED ] RenderTextTest.FontListFallback (22 ms) > > > > sounds like i can talk stevenjb@ into building this for windows for me so i > can > > at least figure out what's going on, though. > > Looks related: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/platform_fo... > https://code.google.com/p/chromium/issues/detail?id=327287 so on Windows, the font that's returned for Symbol doesn't cover that code point: RenderTextTest.FontListFallback (run #1): [ RUN ] RenderTextTest.FontListFallback [3220:2700:1024/143735:2666837:ERROR:render_text_harfbuzz.cc(1080)] XXX evaluating Arial [3220:2700:1024/143735:2666852:ERROR:render_text_harfbuzz.cc(1085)] XXX 1 missing glyph(s) [3220:2700:1024/143735:2666852:ERROR:render_text_harfbuzz.cc(1080)] XXX evaluating Symbol [3220:2700:1024/143735:2666852:ERROR:render_text_harfbuzz.cc(1085)] XXX 1 missing glyph(s) [3220:2700:1024/143735:2666868:ERROR:render_text_harfbuzz.cc(1080)] XXX evaluating Microsoft Sans Serif [3220:2700:1024/143735:2666868:ERROR:render_text_harfbuzz.cc(1085)] XXX 1 missing glyph(s) [3220:2700:1024/143735:2666868:ERROR:render_text_harfbuzz.cc(1133)] XXX best was Arial e:\b\build\slave\win\build\src\ui\gfx\render_text_unittest.cc(2349): error: Value of: spans[0].first.GetFontName() Actual: "Arial" Expected: "Symbol" [ FAILED ] RenderTextTest.FontListFallback (25 ms) (from https://codereview.chromium.org/677803002) but this could be consistent with the bug that you pointed out: maybe Symbol is missing on windows and e.g. Arial is being used instead, but due to GetActualFontNameForTest() being broken the assert doesn't catch this.
filed http://crbug.com/427184 and ifdef-ed the test out on windows.
The CQ bit was checked by derat@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/678683003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2e3cf521ff59c2a3f657a72af04892f4a5783d6a Cr-Commit-Position: refs/heads/master@{#301284} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
