1355931 - UnscaledFont should handle font serialization

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement, P3)

Description

[Lee Salzman [:lsalzman]]

Attachment: 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.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 2

[Lee Salzman [:lsalzman]]

(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.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Why not just create a FontVariations.h in gfx/2d and have the common type in there?

Comment 4

[Lee Salzman [:lsalzman]]

(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.

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: move ScaledFont::VariationSetting into separate header for sharing with thebes

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/176ab833ca36
https://hg.mozilla.org/mozilla-central/rev/9ff13e2532a4