Closed Bug 1321031 Opened 3 years ago Closed 3 years ago

Support font variations in the Mac font backend

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 2 obsolete files)

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
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: nobody → jfkthame
Status: NEW → ASSIGNED
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)
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).
Updated patch to fix build error on Windows. (No actual implementation of variations in the Windows font backends yet.)
Attachment #8817895 - Flags: review?(bas)
Attachment #8817880 - Attachment is obsolete: true
Attachment #8817880 - Flags: review?(bas)
Attachment #8815380 - Flags: review?(lsalzman) → review+
Attachment #8815379 - Flags: review?(jmuizelaar) → review+
Attachment #8815381 - Flags: review?(jmuizelaar) → review+
Attachment #8817879 - Flags: review?(jmuizelaar) → review+
Attachment #8815378 - Flags: review?(jmuizelaar) → 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+
Updated following Bas's comments (above); tagging Jeff for an additional review, as suggested.
Attachment #8818940 - Flags: review?(jmuizelaar)
Attachment #8817895 - Attachment is obsolete: true
Attachment #8817895 - Flags: review?(jmuizelaar)
jrmuizel: r? ping... thx!
Flags: needinfo?(jmuizelaar)
Attachment #8818940 - Flags: review?(jmuizelaar) → review+
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
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31063a2cd3d
Backed out 6 changesets for bustage.
Argh, sorry... I guess that'll be due to changes from bug 1309205, which landed since last time I pushed this to try. :(
Flags: needinfo?(jmuizelaar)
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
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?
Depends on: 1331683
Depends on: 1335522
Depends on: 1342315
You need to log in before you can comment on or make changes to this bug.