|
|
Created:
3 years, 6 months ago by tapted Modified:
3 years, 6 months ago Reviewers:
Alexei Svitkine (slow) 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. |
DescriptionUse 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 #
Messages
Total messages: 47 (35 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use CTFontCreateForString for RenderTextHarfbuzz on Mac And stop using the CTFontCopyDefaultCascadeListForLanguages() private API. cl format Noice cl format First pass BUG=696867, 619684 ========== to ========== Use CTFontCreateForString for RenderTextHarfbuzz on Mac And stop using the CTFontCopyDefaultCascadeListForLanguages() private API. CTFontCreateForString() gives better fallbacks and requires RenderTextHarfbuzz to do less work. BUG=696867, 619684 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Use CTFontCreateForString for RenderTextHarfbuzz on Mac And stop using the CTFontCopyDefaultCascadeListForLanguages() private API. CTFontCreateForString() gives better fallbacks and requires RenderTextHarfbuzz to do less work. BUG=696867, 619684 ========== to ========== 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. BUG=696867, 619684 ==========
tapted@chromium.org changed reviewers: + asvitkine@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
Hi Alexei, please take a look
lgtm https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1519: Nit: Remove empty line. https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1557: #if defined(OS_WIN) || defined(OS_MACOSX) Nit: Suggest moving preferred_fallback_family outside the ifdefs above and then these ifdefs can be removed. (Because preferred_fallback_family will be empty for other platforms and then no need to ifdef here.)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1519: On 2017/06/20 14:59:34, Alexei Svitkine (very slow) wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/2927953003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:1557: #if defined(OS_WIN) || defined(OS_MACOSX) On 2017/06/20 14:59:34, Alexei Svitkine (very slow) wrote: > Nit: Suggest moving preferred_fallback_family outside the ifdefs above and then > these ifdefs can be removed. > > (Because preferred_fallback_family will be empty for other platforms and then no > need to ifdef here.) Nice - Done. Collapsed the 3 `if`.
The CQ bit was unchecked by tapted@chromium.org
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2927953003/#ps80001 (title: "Respond to comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498008025694170, "parent_rev": "dcdf6225ca8644f520cefebe4845ff2948fad8b2", "commit_rev": "e320771e9c78d909f8fe58ad1e8fb60c35248d06"}
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498008025694170, "parent_rev": "f2e8626b26aea8478adde6952501a49e26cff421", "commit_rev": "62cbc10d4dbfeed614eb23c3d996793814a35334"}
Message was sent while issue was closed.
Description was changed from ========== 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. BUG=696867, 619684 ========== to ========== 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. BUG=696867, 619684 Review-Url: https://codereview.chromium.org/2927953003 Cr-Commit-Position: refs/heads/master@{#481062} Committed: https://chromium.googlesource.com/chromium/src/+/62cbc10d4dbfeed614eb23c3d996... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/62cbc10d4dbfeed614eb23c3d996...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 481062 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
On 2017/06/21 02:50:30, findit-for-me wrote: > Findit (https://goo.gl/kROfz5) identified this CL at revision 481062 as the > culprit for > failures in the build cycles as shown on: > https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... ngh. Why do we support so many versions of macOS :/. I'll revert while I investigate. This is a views_unittest failing on 10.10 and 10.11 (not on 10.9 or 10.12) -- LabelSelectionTest.MouseDragSingleLineRTL, so not the test that was added :/
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2950013002/ by tapted@chromium.org. The reason for reverting is: Causes views_unittest LabelSelectionTest.MouseDragSingleLineRTL to fail on 10.10 and 10.11.
Message was sent while issue was closed.
Description was changed from ========== 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. BUG=696867, 619684 Review-Url: https://codereview.chromium.org/2927953003 Cr-Commit-Position: refs/heads/master@{#481062} Committed: https://chromium.googlesource.com/chromium/src/+/62cbc10d4dbfeed614eb23c3d996... ========== to ========== 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. BUG=696867, 619684 Previously landed in r481062, but tickled some OS-specific tests. ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. BUG=696867, 619684 Previously landed in r481062, but tickled some OS-specific tests. ========== to ========== 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 on 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. ==========
Description was changed from ========== 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 on 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. ========== to ========== 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. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Alexei PTAL - updated label_unittest.cc to cater for the discrepancy tickled by the changed font
lgtm
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498175455698880, "parent_rev": "5e1c61bdb08f0704518a64144592ce1a2ee31cf7", "commit_rev": "7d814671483a3cde6b0c9e7c616ceb1eebfd3f17"}
Message was sent while issue was closed.
Description was changed from ========== 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. ========== to ========== 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/+/7d814671483a3cde6b0c9e7c616c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/7d814671483a3cde6b0c9e7c616c... |