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

Issue 2927953003: Use CTFontCreateForString for RenderTextHarfbuzz on Mac (Closed)

Created:
3 years, 6 months ago by tapted
Modified:
3 years, 6 months ago
CC:
chromium-reviews, derat+watch_chromium.org, mac-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use CTFontCreateForString for RenderTextHarfbuzz on Mac The approach ends up pretty close to what's already done for Windows. On Mac, CTFontCreateForString() gives better fallbacks (i.e. they more often match what native UI does) and requires RenderTextHarfbuzz to do less work. However, it doesn't guarantee that the returned font can display every glyph in the provided string. So still use CTFontCopyDefaultCascadeListForLanguages (which is now a public API - hooray) to provide a last-resort list of fallback fonts. This CL causes a font used for Hebrew text in a test to change, which tickled a pre-existing bug/corner case in the test for some macOS versions. BUG=696867, 619684, 735346 Previously landed in r481062, but tickled some OS-specific tests due to a valid discrepancy between rounding directions. Review-Url: https://codereview.chromium.org/2927953003 Cr-Commit-Position: refs/heads/master@{#481764} Committed: https://chromium.googlesource.com/chromium/src/+/7d814671483a3cde6b0c9e7c616ceb1eebfd3f17

Patch Set 1 #

Patch Set 2 : Self review #

Patch Set 3 : rebase #

Patch Set 4 : Keep last resort fallbacks #

Total comments: 4

Patch Set 5 : Respond to comments #

Patch Set 6 : Fix views::Label tests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+104 lines, -43 lines) Patch
M ui/gfx/font_fallback.h View 1 2 3 2 chunks +14 lines, -0 lines 0 comments Download
M ui/gfx/font_fallback_mac.mm View 1 2 3 2 chunks +43 lines, -13 lines 0 comments Download
M ui/gfx/font_fallback_mac_unittest.cc View 1 2 3 3 chunks +18 lines, -1 line 0 comments Download
M ui/gfx/font_fallback_win.h View 1 chunk +0 lines, -8 lines 0 comments Download
M ui/gfx/font_fallback_win.cc View 1 chunk +1 line, -1 line 0 comments Download
M ui/gfx/render_text_harfbuzz.cc View 1 2 3 4 5 chunks +12 lines, -17 lines 0 comments Download
M ui/views/controls/label_unittest.cc View 1 2 3 4 5 1 chunk +16 lines, -3 lines 0 comments Download

Messages

Total messages: 47 (35 generated)
tapted
Hi Alexei, please take a look
3 years, 6 months ago (2017-06-20 05:08:37 UTC) #18
Alexei Svitkine (slow)
lgtm https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harfbuzz.cc#newcode1519 ui/gfx/render_text_harfbuzz.cc:1519: Nit: Remove empty line. https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harfbuzz.cc#newcode1557 ui/gfx/render_text_harfbuzz.cc:1557: #if defined(OS_WIN) ...
3 years, 6 months ago (2017-06-20 14:59:34 UTC) #19
tapted
https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harfbuzz.cc File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harfbuzz.cc#newcode1519 ui/gfx/render_text_harfbuzz.cc:1519: On 2017/06/20 14:59:34, Alexei Svitkine (very slow) wrote: > ...
3 years, 6 months ago (2017-06-20 23:52:57 UTC) #22
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/2927953003/80001
3 years, 6 months ago (2017-06-21 01:20:48 UTC) #26
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/62cbc10d4dbfeed614eb23c3d996793814a35334
3 years, 6 months ago (2017-06-21 01:26:33 UTC) #30
findit-for-me
Findit (https://goo.gl/kROfz5) identified this CL at revision 481062 as the culprit for failures in the ...
3 years, 6 months ago (2017-06-21 02:50:30 UTC) #31
tapted
On 2017/06/21 02:50:30, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) identified this CL at revision 481062 as ...
3 years, 6 months ago (2017-06-21 02:56:18 UTC) #32
tapted
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2950013002/ by tapted@chromium.org. ...
3 years, 6 months ago (2017-06-21 02:57:35 UTC) #33
tapted
Hi Alexei PTAL - updated label_unittest.cc to cater for the discrepancy tickled by the changed ...
3 years, 6 months ago (2017-06-21 09:16:25 UTC) #41
Alexei Svitkine (slow)
lgtm
3 years, 6 months ago (2017-06-22 14:22:53 UTC) #42
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/2927953003/100001
3 years, 6 months ago (2017-06-22 23:51:27 UTC) #44
commit-bot: I haz the power
3 years, 6 months ago (2017-06-23 01:37:33 UTC) #47
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7d814671483a3cde6b0c9e7c616c...

Powered by Google App Engine
This is Rietveld 408576698