UnscaledFont should handle font serialization

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Text
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

({feature})

51 Branch
mozilla55
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments)

(Assignee)

Description

8 months ago
Created attachment 8857581 [details] [diff] [review]
move font serialization from ScaledFont to UnscaledFont

At present, ScaledFonts are responsible for serialization duty, i.e. GetFontFileData/GetFontDescriptor live within ScaledFont. However, as part of bug 1348980, we added an UnscaledFont which is supposed to represent the shared platform font resource (or some interpretation thereof) that is thus shared by many ScaledFonts. So it is more natural for the serialization work to be done inside UnscaledFont, leaving ScaledFont only to deal with serializing details that relate to instances of the font resource, rather than the font resource itself.

This patch restructures the code as such. GetFontFileData/GetFontDescriptor now live inside UnscaledFont. GetFontInstanceData still lives in ScaledFont in case it has any instance data related to the scaled version itself. Also, UnscaledFont supports GetFontInstanceData in case the UnscaledFont represents a non-pure interpretation of the font resource data for some reason or another - originally because this was meant to support VariationSettings, but Jonathan Kew and I decided to temporarily leave VariationSetting in ScaledFont as part of bug 1348980 until work on VariationSettings for other platforms than Mac resumes.

Deserialization now works such that first NativeFontResourceFoo is made as normal. Instead of NFRs creating ScaledFonts, they now create UnscaledFonts instead. Those UnscaledFonts are then responsible for creating the ScaledFont with whatever instance data.

So for both serialization and deserialization, UnscaledFont is interposed so the flow from NFR -> SF now just becomes NFR -> UF -> SF.

Currently UnscaledFontFoo code is intermingled with ScaledFontFoo code in the ScaledFontFoo.cpp files as they are heavily codependent right now and further refactorings might happen in the future.

Another incidental drive-by cleanup in here is UnscaledFontMac::CreateCGFontWithVariations. Previously, this code was just copied verbatim in two places (NativeFontResourceMac and thebes/gfxMacFont). I moved this into UnscaledFontMac so that both places can use them, as that made most sense (and there was a comment in it begging for it to be cleaned up anyway).

Also some annoying changes in declarations from virtual to override to make static analyzer happy, but I suppose is stylistically better anyway.

There is only partial support for Android, with some bits of UnscaledFontFreeType implemented. However, without an equivalent NativeFontResourceFreeType, full DrawTargetRecording is not there yet - but can be done in a future bug if it turns out we do need it.
Attachment #8857581 - Flags: review?(jmuizelaar)
Comment on attachment 8857581 [details] [diff] [review]
move font serialization from ScaledFont to UnscaledFont

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

::: gfx/thebes/gfxMacFont.cpp
@@ +44,5 @@
> +        mCGFont =
> +            UnscaledFontMac::CreateCGFontWithVariations(
> +                baseFont,
> +                aFontStyle->variationSettings.Length(),
> +                reinterpret_cast<const ScaledFont::VariationSetting*>(

Can we just switch aFontStyle to use ScaledFont::VarationSetting and drop the gfxFontVariation type completely?
Attachment #8857581 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 2

8 months ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Comment on attachment 8857581 [details] [diff] [review]
> move font serialization from ScaledFont to UnscaledFont
> 
> Review of attachment 8857581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxMacFont.cpp
> @@ +44,5 @@
> > +        mCGFont =
> > +            UnscaledFontMac::CreateCGFontWithVariations(
> > +                baseFont,
> > +                aFontStyle->variationSettings.Length(),
> > +                reinterpret_cast<const ScaledFont::VariationSetting*>(
> 
> Can we just switch aFontStyle to use ScaledFont::VarationSetting and drop
> the gfxFontVariation type completely?

In talking with Jonathan, it seems like no matter what we do here, this part will be a tad ugly. gfxFontVariations.h is meant to neither pull in 2D.h nor gfxFont.h, so that users like nsFont.h can be clean of them. There are also some servo bindings that are mucking with gfxFontVariation as well...

This reinterpret_cast may be a lesser evil, all things considered. But I guess an assert here to verify sizeof(VariationSetting) == sizeof(gfxFontVariation) can at least guard against future greater evils.
Why not just create a FontVariations.h in gfx/2d and have the common type in there?
(Assignee)

Comment 4

8 months ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> Why not just create a FontVariations.h in gfx/2d and have the common type in
> there?

Also a possibility, though a bit lame to need a new header for a 2 field type...

You'd still probably need to keep the gfxFontVariations.h header around because we have consumers like servo still using it. Alternatively you could remove that, but have the definition living in moz2d living outside mozilla::gfx and named gfxFontVariation, but would not fit stylistically with moz2d.
(Assignee)

Comment 5

8 months ago
Created attachment 8857757 [details] [diff] [review]
move ScaledFont::VariationSetting into separate header for sharing with thebes
Attachment #8857757 - Flags: review?(jmuizelaar)
Attachment #8857757 - Flags: review?(jmuizelaar) → review+

Comment 6

8 months ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/176ab833ca36
move font serialization from ScaledFont to UnscaledFont. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff13e2532a4
move ScaledFont::VariationSetting into separate header for sharing with thebes. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/176ab833ca36
https://hg.mozilla.org/mozilla-central/rev/9ff13e2532a4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1357009
(Assignee)

Updated

8 months ago
Depends on: 1362117
(Assignee)

Updated

7 months ago
Depends on: 1363492
You need to log in before you can comment on or make changes to this bug.