Support font variations in the Mac font backend

RESOLVED FIXED in Firefox 53

Status

()

Core
Graphics: Text
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(6 attachments, 2 obsolete attachments)

12.13 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.87 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.30 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
1.36 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.33 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
24.19 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
The new OpenType Variations technology is already supported (at least for TrueType-flavored fonts) in CoreGraphics/CoreText, as it is the same technology that Apple originally implemented in QuickDraw GX and then AAT fonts.

Therefore, we should be able to render variations specified via CSS font-variation-settings (bug 1321022) on macOS by hooking this up to the Core Text variations support.
(Assignee)

Comment 1

2 years ago
Created attachment 8815378 [details] [diff] [review]
pt 1 - Implement support for variations in gfxMacFont, passing settings through to Core Text
Attachment #8815378 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8815379 [details] [diff] [review]
pt 2 - Preserve variations settings in ScaledFontMac when creating a new CTFont
Attachment #8815379 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

2 years ago
Created attachment 8815380 [details] [diff] [review]
pt 3 - Preserve variations settings when creating a new CTFont in SkFontHost_mac
Attachment #8815380 - Flags: review?(jmuizelaar)
(Assignee)

Comment 4

2 years ago
Created attachment 8815381 [details] [diff] [review]
pt 4 - Preserve variations settings if gfxCoreTextShaper needs to instantiate a new CTFont
Attachment #8815381 - Flags: review?(jmuizelaar)
(Assignee)

Comment 5

2 years ago
Try build with these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=730390619f2d8043506ceb33ea47f5396ca7d42f. With this, examples like [1] or [2] should work on a current macOS system.

[1] https://jsfiddle.net/k6pkvj66/1/
[2] https://people-mozilla.org/~jkew/tests/skia.html
Attachment #8815380 - Flags: review?(jmuizelaar) → review?(lsalzman)
(Assignee)

Comment 6

2 years ago
Testing with the try build from comment 5 reveals that variations are not handled properly when printing. We need to preserve variation settings in a couple more places... within cairo-quartz-font.c (fixes the printed output when e10s is disabled), and in a Moz2d recording (required for parent-process printing when e10s is enabled).
(Assignee)

Comment 7

2 years ago
Created attachment 8817879 [details] [diff] [review]
pt 5 - Preserve any variation settings present in the CGFont when the cairo quartz backend creates a new CTFont
Attachment #8817879 - Flags: review?(jmuizelaar)
(Assignee)

Comment 8

2 years ago
Created attachment 8817880 [details] [diff] [review]
pt 6 - Include variation settings in moz2d recording stream when using the native Mac font backend
Attachment #8817880 - Flags: review?(bas)
(Assignee)

Comment 10

2 years ago
Created attachment 8817895 [details] [diff] [review]
pt 6 - Include variation settings in moz2d recording stream when using the native Mac font backend

Updated patch to fix build error on Windows. (No actual implementation of variations in the Windows font backends yet.)
Attachment #8817895 - Flags: review?(bas)
(Assignee)

Updated

2 years ago
Attachment #8817880 - Attachment is obsolete: true
Attachment #8817880 - Flags: review?(bas)
Attachment #8815380 - Flags: review?(lsalzman) → review+
Comment on attachment 8817895 [details] [diff] [review]
pt 6 - Include variation settings in moz2d recording stream when using the native Mac font backend

Review of attachment 8817895 [details] [diff] [review]:
-----------------------------------------------------------------

It would be good if jrmuizel reviewed the CoreFont parts of this.

::: gfx/2d/NativeFontResourceMac.cpp
@@ +177,5 @@
>    }
>  
> +  if (aVariationCount > 0) {
> +    MOZ_ASSERT(aVariations);
> +    CFDictionaryRef varDict =

Why not use AutoRelease here?

::: gfx/2d/RecordedEvent.cpp
@@ +1512,5 @@
>  
>  RecordedFontData::~RecordedFontData()
>  {
>    delete[] mData;
> +  delete[] mVariations;

While we're in this code let's make sure these are initialized to nullptr so we don't risk anything when getting to the destructor without SetFontData being called.

@@ +1565,5 @@
> +  mFontDetails.variationCount = aVariationCount;
> +  if (aVariationCount > 0) {
> +    mVariations = new ScaledFont::VariationSetting[aVariationCount];
> +    memcpy(mVariations, aVariations, varDataSize);
> +  } else {

Then afaict you will be able to remove these else clauses.
Attachment #8817895 - Flags: review?(jmuizelaar)
Attachment #8817895 - Flags: review?(bas)
Attachment #8817895 - Flags: review+
(Assignee)

Comment 12

2 years ago
Created attachment 8818940 [details] [diff] [review]
pt 6 - Include variation settings in moz2d recording stream when using the native Mac font backend

Updated following Bas's comments (above); tagging Jeff for an additional review, as suggested.
Attachment #8818940 - Flags: review?(jmuizelaar)
(Assignee)

Updated

2 years ago
Attachment #8817895 - Attachment is obsolete: true
Attachment #8817895 - Flags: review?(jmuizelaar)
(Assignee)

Comment 13

2 years ago
jrmuizel: r? ping... thx!
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4ae57d5358f14e6df0aa848eed3191a900f9adc
Bug 1321031 pt 1 - Implement support for variations in gfxMacFont, passing settings through to Core Text. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/c551913ae89236623ee320537d6f204e3eeec7fb
Bug 1321031 pt 2 - Preserve variations settings in ScaledFontMac when creating a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc58e479eb58556445e95d194e6bf30e7585c2c0
Bug 1321031 pt 3 - Preserve variations settings when creating a new CTFont in SkFontHost_mac. r=lsalzman

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb4242dc636a9be72f992e57c6d7251c280600b
Bug 1321031 pt 4 - Preserve variations settings if gfxCoreTextShaper needs to instantiate a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba071046f8abffbc8207e1d8cdf184b70f757dfd
Bug 1321031 pt 5 - Preserve any variation settings present in the CGFont when the cairo quartz backend creates a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0be4f5390fbc8189584e8c5af18523403e8bb1a
Bug 1321031 pt 6 - Include variation settings in moz2d recording stream when using the native Mac font backend. r=bas,jrmuizel

Comment 16

2 years ago
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31063a2cd3d
Backed out 6 changesets for bustage.
(Assignee)

Comment 17

2 years ago
Argh, sorry... I guess that'll be due to changes from bug 1309205, which landed since last time I pushed this to try. :(
(Assignee)

Updated

2 years ago
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4290bb5169142fe9423304458b5b1360a699f0ec
Bug 1321031 pt 1 - Implement support for variations in gfxMacFont, passing settings through to Core Text. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/829377878aad6f9ba5f790d782292a5cead54af0
Bug 1321031 pt 2 - Preserve variations settings in ScaledFontMac when creating a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/83046b21acd7887cf14ccde82df900e4e61d3f0e
Bug 1321031 pt 3 - Preserve variations settings when creating a new CTFont in SkFontHost_mac. r=lsalzman

https://hg.mozilla.org/integration/mozilla-inbound/rev/53365854908ea9138fe130f6aade217851272ebc
Bug 1321031 pt 4 - Preserve variations settings if gfxCoreTextShaper needs to instantiate a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/77f41c0e5d6fd011c7494da45607716ff7351909
Bug 1321031 pt 5 - Preserve any variation settings present in the CGFont when the cairo quartz backend creates a new CTFont. r=jrmuizel

https://hg.mozilla.org/integration/mozilla-inbound/rev/273ce98bb9e831b70d9ce2b34590859ea2ce80b8
Bug 1321031 pt 6 - Include variation settings in moz2d recording stream when using the native Mac font backend. r=bas,jrmuizel

Comment 20

2 years ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/eefd48f36d82
Follow-up to fix build bustage on the graphics branch. r=jrmuizel?
(Assignee)

Updated

a year ago
Depends on: 1331683

Updated

a year ago
Depends on: 1335522
(Assignee)

Updated

a year ago
Depends on: 1342315
You need to log in before you can comment on or make changes to this bug.