Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1344213004:
Avoid CTFontCreateCopyWithAttributes. (Closed)
|
Created:
5 years, 3 months ago by bungeman-skia Modified:
5 years, 3 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAvoid CTFontCreateCopyWithAttributes.
It appears that CTFontCreateCopyWithAttributes and
CTFontCreateCopyWithSymbolicTraits share similar issues with the default
font on OSX10.10. CTFontCreateWithFontDescriptor cannot be used as
it will not work with webfonts. Since this is all low-level use, create the
CTFonts from the underlying CGFonts directly.
BUG=chromium:524646
Committed: https://skia.googlesource.com/skia/+/d3296717b9d0930237be3502b82ab94d0b4d177f
Committed: https://skia.googlesource.com/skia/+/7cbeaae8bb42c386145cd020f714f0eac8021dc2
Patch Set 1 : This one breaks non-system fonts. #Patch Set 2 : Doesn't break webfonts. #
Total comments: 4
Patch Set 3 : Convert other uses as well. #Patch Set 4 : Refactor. #
Total comments: 4
Patch Set 5 : Address comment. #Patch Set 6 : Fix synthetic bold italic. #Messages
Total messages: 42 (16 generated)
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/1
The CQ bit was unchecked by bungeman@google.com
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/16 19:49:53, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. This has the right behavior on my retina 10.11 machine.
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1344213004/diff/20001/src/ports/SkFontHost_ma... File src/ports/SkFontHost_mac.cpp (right): https://codereview.chromium.org/1344213004/diff/20001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:775: fCTUnrotatedFont.reset(CTFontCreateWithGraphicsFont(baseCGFont, textSize, nullptr, nit: The textSize parameter here looks redundant with the attribute in line 767 - are they both necessary?
lgtm
https://codereview.chromium.org/1344213004/diff/20001/src/ports/SkFontHost_ma... File src/ports/SkFontHost_mac.cpp (right): https://codereview.chromium.org/1344213004/diff/20001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:775: fCTUnrotatedFont.reset(CTFontCreateWithGraphicsFont(baseCGFont, textSize, nullptr, On 2015/09/16 20:50:28, erikchen wrote: > nit: The textSize parameter here looks redundant with the attribute in line 767 > - are they both necessary? It does, doesn't it. On the other hand, under the hood they appear to set different things. This is probably somewhat belt and suspenders, but I think various versions treat these somewhat differently. For the time being, I'm going to set them both because this seems to work and should work across versions. (Almost all of the comments and around a third of the code in this file is to work around issues in various versions of coretext/coregraphics, so I'm a bit paranoid in general here.) Eventually (probably when 10.6 support is dropped) I'm going to go through all of this file on supported versions and see what can be ripped out. https://codereview.chromium.org/1344213004/diff/20001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:1397: font = CTFontCreateCopyWithAttributes(fCTFont, 1, &xform, nullptr); Will need to fix this use. https://codereview.chromium.org/1344213004/diff/20001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:1593: AutoCFRelease<CTFontRef> ctFont(CTFontCreateCopyWithAttributes( And this use.
bungeman@google.com changed reviewers: + mtklein@google.com, reed@google.com
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/18 22:31:06, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. reed, mtklein: Ping? We're trying to get this fix merged to M46, so an expedited review would be appreciated.
lgtm
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/1344213004/#ps60001 (title: "Refactor.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/d3296717b9d0930237be3502b82ab94d0b4d177f
Message was sent while issue was closed.
https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... File src/ports/SkFontHost_mac.cpp (right): https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:715: // CTFontCreateCopyWithAttributes or CTFontCreateCopyWithSymbolicTraits cannot be used on 10.10 The comment says 10.10 but the BUG= is about 10.11. Is this right? https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:1380: if (fRec.fFlags & SkScalerContext::kSubpixelPositioning_Flag) { Can this be if (fDoSubPosition)? It’s harder to reason about whether the “need to release” font on line 1403 is properly balanced by the CFSafeRelease() on line 1420 when the conditions that guard them are different.
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1354323002/ by bungeman@google.com. The reason for reverting is: Causes issues with synthetic bold italic on all versions of OSX, with OSX10.10 resulting in many measurement related changes (this is expected though). fast/text/atsui-multiple-renderers.html fast/css/font-face-synthetic-bold-italic.html https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_9__... https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_10/....
https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... File src/ports/SkFontHost_mac.cpp (right): https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:715: // CTFontCreateCopyWithAttributes or CTFontCreateCopyWithSymbolicTraits cannot be used on 10.10 On 2015/09/19 03:18:41, Mark Mentovai - August is over wrote: > The comment says 10.10 but the BUG= is about 10.11. Is this right? After letting this run through the bots and seeing the massive difference on 10.10 vs previous versions, it sure looks like 10.10 has at least similar issues. https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... src/ports/SkFontHost_mac.cpp:1380: if (fRec.fFlags & SkScalerContext::kSubpixelPositioning_Flag) { On 2015/09/19 03:18:41, Mark Mentovai - August is over wrote: > Can this be if (fDoSubPosition)? It’s harder to reason about whether the “need > to release” font on line 1403 is properly balanced by the CFSafeRelease() on > line 1420 when the conditions that guard them are different. Yes, that should be changed, so why not change it now? Done.
On 2015/09/19 06:25:47, bungeman1 wrote: > https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... > File src/ports/SkFontHost_mac.cpp (right): > > https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... > src/ports/SkFontHost_mac.cpp:715: // CTFontCreateCopyWithAttributes or > CTFontCreateCopyWithSymbolicTraits cannot be used on 10.10 > On 2015/09/19 03:18:41, Mark Mentovai - August is over wrote: > > The comment says 10.10 but the BUG= is about 10.11. Is this right? > > After letting this run through the bots and seeing the massive difference on > 10.10 vs previous versions, it sure looks like 10.10 has at least similar > issues. So are the current test expectations for 10.10 wrong? Do we need to add automatic-rebaselines? > > https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... > src/ports/SkFontHost_mac.cpp:1380: if (fRec.fFlags & > SkScalerContext::kSubpixelPositioning_Flag) { > On 2015/09/19 03:18:41, Mark Mentovai - August is over wrote: > > Can this be if (fDoSubPosition)? It’s harder to reason about whether the “need > > to release” font on line 1403 is properly balanced by the CFSafeRelease() > on > > line 1420 when the conditions that guard them are different. > > Yes, that should be changed, so why not change it now? Done.
On 2015/09/21 18:05:47, erikchen wrote: > On 2015/09/19 06:25:47, bungeman1 wrote: > > > https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... > > File src/ports/SkFontHost_mac.cpp (right): > > > > > https://codereview.chromium.org/1344213004/diff/60001/src/ports/SkFontHost_ma... > > src/ports/SkFontHost_mac.cpp:715: // CTFontCreateCopyWithAttributes or > > CTFontCreateCopyWithSymbolicTraits cannot be used on 10.10 > > On 2015/09/19 03:18:41, Mark Mentovai - August is over wrote: > > > The comment says 10.10 but the BUG= is about 10.11. Is this right? > > > > After letting this run through the bots and seeing the massive difference on > > 10.10 vs previous versions, it sure looks like 10.10 has at least similar > > issues. > So are the current test expectations for 10.10 wrong? Do we need to add > automatic-rebaselines? Yes, the test expectations for 10.10 are probably wrong and will need rebaselined. That's not really an issue, the current thing I'm investigating is that with this method synthetic bold-italic is the right amount of bold, but somewhat less italic when done this way (on all versions). This is rather confusing, as synthetic bold looks fine and synthetic italic looks fine, it's just bold italic that's less oblique. I'm currently looking to see if there is any way around this.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1344213004/#ps100001 (title: "Fix synthetic bold italic.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1344213004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1344213004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/7cbeaae8bb42c386145cd020f714f0eac8021dc2
Message was sent while issue was closed.
On 2015/09/22 16:55:00, commit-bot: I haz the power wrote: > Committed patchset #6 (id:100001) as > https://skia.googlesource.com/skia/+/7cbeaae8bb42c386145cd020f714f0eac8021dc2 Note that this uncovered yet another bug in CoreGraphics, which is worked around in https://codereview.chromium.org/1362053003/ . |
