Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
674233003:
RenderTextHarfBuzz: Try fallback fonts of the Uniscribe font while shaping (Closed)
DescriptionRenderTextHarfBuzz: Try fallback fonts of the Uniscribe font while shaping
This CL also fixes a Linux issue where a fallback font could be skipped.
BUG=426052
R=msw
Committed: https://crrev.com/31227048b11f0bc84a295ff82b5056599afa07f7
Cr-Commit-Position: refs/heads/master@{#301920}
Patch Set 1 #
Total comments: 4
Patch Set 2 : added test #Patch Set 3 : comments addressed #
Total comments: 2
Patch Set 4 : skip first font #
Total comments: 12
Patch Set 5 : comments addressed #
Total comments: 4
Patch Set 6 : comments addressed 2 #Patch Set 7 : rebased #Patch Set 8 : fix include #
Total comments: 2
Messages
Total messages: 32 (9 generated)
msw@chromium.org changed reviewers: + derat@chromium.org
Have you confirmed that this fixes the bug? Can you write a test? (maybe check width against missing_glyph_width*length)? https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1133: // Try fonts in the fallback list of the Uniscribe font. Shouldn't this be in an #if defined(OS_WIN) block?
https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1137: for (size_t i = 1; i < fallback_families.size(); ++i) { there's a bunch of duplicated code here. is the GetFallbackFontFamilies() call expensive, or could you just append this list to the primary font's fallbacks and have a single loop?
On 2014/10/27 17:28:51, msw wrote: > Have you confirmed that this fixes the bug? Yes. > Can you write a test? Done. https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1133: // Try fonts in the fallback list of the Uniscribe font. On 2014/10/27 17:28:51, msw wrote: > Shouldn't this be in an #if defined(OS_WIN) block? Done. https://codereview.chromium.org/674233003/diff/1/ui/gfx/render_text_harfbuzz.... ui/gfx/render_text_harfbuzz.cc:1137: for (size_t i = 1; i < fallback_families.size(); ++i) { On 2014/10/27 18:49:24, Daniel Erat wrote: > there's a bunch of duplicated code here. is the GetFallbackFontFamilies() call > expensive, or could you just append this list to the primary font's fallbacks > and have a single loop? Done.
https://codereview.chromium.org/674233003/diff/40001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1128: uniscribe_fallbacks.begin(), uniscribe_fallbacks.end()); patch set 2 skipped the first uniscribe fallback font, but this version doesn't appear to. do you need to? i.e. does GetFallbackFontFamilies() also return |uniscribe_font| in the first position here? render_text_win.cc also skip the first uniscribe fallback font, so i suspect you may need to update the range that you insert here. it'd probably be cleaner to also update the earlier |fallback_families| assignment as well so the loop below doesn't need to start at index 1.
https://codereview.chromium.org/674233003/diff/40001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/40001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1128: uniscribe_fallbacks.begin(), uniscribe_fallbacks.end()); On 2014/10/28 14:32:59, Daniel Erat wrote: > patch set 2 skipped the first uniscribe fallback font, but this version doesn't > appear to. do you need to? i.e. does GetFallbackFontFamilies() also return > |uniscribe_font| in the first position here? Yes, fixed. > also update the earlier |fallback_families| assignment as well so the loop below > doesn't need to start at index 1. That'd require shifting vector elements which I'd like to avoid here.
lgtm with a question https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1129: uniscribe_fallbacks.begin() + 1, uniscribe_fallbacks.end()); can the returned vector ever be empty? if so, you probably need to check empty() here to avoid a crash.
https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1129: uniscribe_fallbacks.begin() + 1, uniscribe_fallbacks.end()); On 2014/10/28 18:00:36, Daniel Erat wrote: > can the returned vector ever be empty? if so, you probably need to check empty() > here to avoid a crash. It looks like all impls should return at least one element. That said, a check is still a good idea. https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Skip the first fallback font, which is |primary_font|. I'm trying to determine if this is true for font_fallback_linux.cc, which only adds the primary font if FC doesn't yield any fonts. Is FC guaranteed to include the argument font as the first in its (otherwise potentially empty) list? https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2337: // fonts, forcing RTHB to try the Uniscribe font and its fallbacks. I'm not sure this really tests that the Uniscribe font's fallbacks are tested, have you ensured that it fails without your fix? https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2339: Font font(font_win); nit: inline this in the line below?
https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Skip the first fallback font, which is |primary_font|. On 2014/10/28 18:17:56, msw wrote: > I'm trying to determine if this is true for font_fallback_linux.cc, which only > adds the primary font if FC doesn't yield any fonts. Is FC guaranteed to include > the argument font as the first in its (otherwise potentially empty) list? yeah, you're right -- this seems wrong. i don't think there are any guarantees about what Fontconfig will return here. it could be a completely different set of fonts. rather than skipping the first font, we should probably just skip duplicates by comparing family names against the primary font's family.
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1129: uniscribe_fallbacks.begin() + 1, uniscribe_fallbacks.end()); On 2014/10/28 18:17:56, msw wrote: > On 2014/10/28 18:00:36, Daniel Erat wrote: > > can the returned vector ever be empty? if so, you probably need to check > empty() > > here to avoid a crash. > > It looks like all impls should return at least one element. That said, a check > is still a good idea. We now add all fonts and skip if it's primary_font or uniscribe_font. https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_harfb... ui/gfx/render_text_harfbuzz.cc:1133: // Skip the first fallback font, which is |primary_font|. On 2014/10/28 18:20:53, Daniel Erat wrote: > On 2014/10/28 18:17:56, msw wrote: > > I'm trying to determine if this is true for font_fallback_linux.cc, which only > > adds the primary font if FC doesn't yield any fonts. Is FC guaranteed to > include > > the argument font as the first in its (otherwise potentially empty) list? > > yeah, you're right -- this seems wrong. i don't think there are any guarantees > about what Fontconfig will return here. it could be a completely different set > of fonts. > > rather than skipping the first font, we should probably just skip duplicates by > comparing family names against the primary font's family. Done. https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2337: // fonts, forcing RTHB to try the Uniscribe font and its fallbacks. On 2014/10/28 18:17:56, msw wrote: > I'm not sure this really tests that the Uniscribe font's fallbacks are tested, > have you ensured that it fails without your fix? Yes, it fails without this fix. https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2339: Font font(font_win); On 2014/10/28 18:17:56, msw wrote: > nit: inline this in the line below? "FontList font_list(Font(font_win));" gives a syntax error due to what looks like an instance of https://en.wikipedia.org/wiki/Most_vexing_parse Error below: d:\gitchromium\repo\src\ui\gfx\render_text_unittest.cc(2341) : error C2664: 'void gfx::RenderText::SetFontList(const gfx::FontList &)' : cannot convert argument 1 from 'gfx::FontList (__cdecl *)(gfx::Font)' to 'const gfx::FontList &' Reason: cannot convert from 'overloaded-function' to 'const gfx::FontList' No constructor could take the source type, or constructor overload resolution was ambiguous
please also update the change description to mention that this also fixes a potential issue on linux where the first fallback font could be incorrectly skipped. https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2339: Font font(font_win); On 2014/10/29 01:07:20, ckocagil wrote: > On 2014/10/28 18:17:56, msw wrote: > > nit: inline this in the line below? > > "FontList font_list(Font(font_win));" gives a syntax error due to what looks > like an instance of https://en.wikipedia.org/wiki/Most_vexing_parse > > Error below: > > d:\gitchromium\repo\src\ui\gfx\render_text_unittest.cc(2341) : error C2664: > 'void gfx::RenderText::SetFontList(const gfx::FontList &)' : cannot convert > argument 1 from 'gfx::FontList (__cdecl *)(gfx::Font)' to 'const gfx::FontList > &' > Reason: cannot convert from 'overloaded-function' to 'const > gfx::FontList' > No constructor could take the source type, or constructor overload > resolution was ambiguous you can just work around this with an extra set of parentheses around the argument, right? https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1133: for (size_t i = 0; i < fallback_families.size(); ++i) { nit: for (auto family : fallback_families) { https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_win.... ui/gfx/render_text_win.cc:1073: for (size_t i = 1; i < fonts.size(); ++i) { should this also iterate over all families instead of skipping the first one?
On 2014/10/29 01:18:11, Daniel Erat wrote: > please also update the change description to mention that this also fixes a > potential issue on linux where the first fallback font could be incorrectly > skipped. Done. https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/674233003/diff/60001/ui/gfx/render_text_unitt... ui/gfx/render_text_unittest.cc:2339: Font font(font_win); On 2014/10/29 01:18:10, Daniel Erat wrote: > On 2014/10/29 01:07:20, ckocagil wrote: > > On 2014/10/28 18:17:56, msw wrote: > > > nit: inline this in the line below? > > > > "FontList font_list(Font(font_win));" gives a syntax error due to what looks > > like an instance of https://en.wikipedia.org/wiki/Most_vexing_parse > > > > Error below: > > > > d:\gitchromium\repo\src\ui\gfx\render_text_unittest.cc(2341) : error C2664: > > 'void gfx::RenderText::SetFontList(const gfx::FontList &)' : cannot convert > > argument 1 from 'gfx::FontList (__cdecl *)(gfx::Font)' to 'const gfx::FontList > > &' > > Reason: cannot convert from 'overloaded-function' to 'const > > gfx::FontList' > > No constructor could take the source type, or constructor overload > > resolution was ambiguous > > you can just work around this with an extra set of parentheses around the > argument, right? Oh, that works! Thanks! https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1133: for (size_t i = 0; i < fallback_families.size(); ++i) { On 2014/10/29 01:18:10, Daniel Erat wrote: > nit: for (auto family : fallback_families) { Done. (yay c++11) https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): https://codereview.chromium.org/674233003/diff/100001/ui/gfx/render_text_win.... ui/gfx/render_text_win.cc:1073: for (size_t i = 1; i < fonts.size(); ++i) { On 2014/10/29 01:18:10, Daniel Erat wrote: > should this also iterate over all families instead of skipping the first one? It doesn't have to, LinkedFontsIterator guarantees to return the original font as the first element.
lgtm
lgtm, thanks!
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/674233003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/75275) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/80310) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/674233003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by ckocagil@chromium.org
The CQ bit was checked by ckocagil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/674233003/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/31227048b11f0bc84a295ff82b5056599afa07f7 Cr-Commit-Position: refs/heads/master@{#301920}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/674233003/diff/160001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/160001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1135: for (auto family : fallback_families) { This should be `const auto&` according to the style guide. As is, this loop copies a bunch os trings aroudn for no good reason.
Message was sent while issue was closed.
https://codereview.chromium.org/674233003/diff/160001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/674233003/diff/160001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1135: for (auto family : fallback_families) { On 2014/11/21 17:52:40, Nico wrote: > This should be `const auto&` according to the style guide. As is, this loop > copies a bunch os trings aroudn for no good reason. I will fix this in https://codereview.chromium.org/738363002/. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
