Cache ScaledFonts in gfxFont::GetScaledFont

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

56 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 attachments)

gfxFont::GetScaledFont is currently a mess. Some subclasses of gfxFont override it and therein cache the result, so that we don't repeatedly create new ScaledFonts every time it is called. Other implementations fall back to the gfxFont default, which is to call into gfxPlatform::GetScaledFontForFont, which exists only for this purpose, and then do ugly manual switching by checking the font type to determine what type of ScaledFont to create.

The combination of all these things complicate future WebRender integration changes we wish to make. We want to be able to associate font instance keys with ScaledFonts, but if ScaledFonts are not stable and being recreated all the time, this becomes difficult and costly.

So let's clean this up so that all gfxFont subclasses just directly implement the desired behavior in GetScaledFont, removing all the silly manual switching, and also clean up those implementations to always cache. gfxPlatform::GetScaledFontForFont can then go away.

With that in place, ScaledFont lifetimes will now agree with gfxFont lifetimes and associating keys with them will make sense.
I need to be able to set the cairo_scaled_font_t on a ScaledFontDWrite after it has been already created. But since ScaledFontBase is not exported outside of Moz2D, the easier thing here is just to make these virtual on ScaledFont so that they can thus be directly used from ScaledFont without casting to ScaledFontBase.

Didn't use override keyword in ScaledFontBase here because that would cause the static analyzer to get angry and require all the other functions to use override as well. Trying not to rock the boat.
Attachment #8890992 - Flags: review?(jmuizelaar)
Since we removed the old gfxFontconfigFonts implementation, gfxFontconfigFontBase has no more reason to exist. Getting rid of this to simplify the question of where the GetScaledFont implementation should live for gfxFcPlatformFontList, and also to remove stale layers of code.
Attachment #8890995 - Flags: review?(jfkthame)
gfxFont::GetCairoScaledFont already exists to access gfxFont::mScaledFont. Some subclasses were implementing their own accessor named CairoScaledFont for this mScaledFont field. This is just confusing, and there's no good reason for it anymore. Simplify the story here by getting rid of superfluous CairoScaledFont accessors and making everyone use GetCairoScaledFont.
Attachment #8890998 - Flags: review?(jfkthame)
This does the real grunt work of the bug which is to make sure every gfxFont subclass actually implements GetScaledFont. Once that is in place, there are no callers left of gfxPlatform::GetScaledFontForFont, so we can safely remove that.

All implementations now consistently cache the result in the mAzureScaledFont field, like some implementations were already doing.
Attachment #8891000 - Flags: review?(jfkthame)
Attachment #8890992 - Flags: review?(jmuizelaar) → review+
Attachment #8890995 - Flags: review?(jfkthame) → review+
Comment on attachment 8890992 [details] [diff] [review]
make ScaledFont::SetCairoScaledFont virtual so it can be accessed from outside Moz2D

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

::: gfx/2d/2D.h
@@ +823,5 @@
>  
>    const RefPtr<UnscaledFont>& GetUnscaledFont() const { return mUnscaledFont; }
>  
> +  virtual cairo_scaled_font_t* GetCairoScaledFont() { return nullptr; }
> +  virtual void SetCairoScaledFont(cairo_scaled_font_t* font) {}

Should these be wrapped in #ifdef USE_CAIRO_SCALED_FONT, like they are in ScaledFontBase.h?
(In reply to Lee Salzman [:lsalzman] from comment #3)
> Created attachment 8890998 [details] [diff] [review]
> remove redundant CairoScaledFont accessor from gfxFont subclasses
> 
> gfxFont::GetCairoScaledFont already exists to access gfxFont::mScaledFont.
> Some subclasses were implementing their own accessor named CairoScaledFont
> for this mScaledFont field. This is just confusing, and there's no good
> reason for it anymore. Simplify the story here by getting rid of superfluous
> CairoScaledFont accessors and making everyone use GetCairoScaledFont.

gfxFont::GetCairoScaledFont is virtual, whereas the CairoScaledFont in subclasses isn't. So this will introduce a virtual call where we currently don't use one. Can we avoid that?
Ping re comment 6: I'd like us to at least consider if we can address this, before going ahead with the review here.
Flags: needinfo?(lsalzman)
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Comment on attachment 8890992 [details] [diff] [review]
> make ScaledFont::SetCairoScaledFont virtual so it can be accessed from
> outside Moz2D
> 
> Review of attachment 8890992 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/2D.h
> @@ +823,5 @@
> >  
> >    const RefPtr<UnscaledFont>& GetUnscaledFont() const { return mUnscaledFont; }
> >  
> > +  virtual cairo_scaled_font_t* GetCairoScaledFont() { return nullptr; }
> > +  virtual void SetCairoScaledFont(cairo_scaled_font_t* font) {}
> 
> Should these be wrapped in #ifdef USE_CAIRO_SCALED_FONT, like they are in
> ScaledFontBase.h?

USE_CAIRO_SCALED_FONT is mainly defined in ScaledFontBase.h, which is not exposed in 2D.h at all. For that matter, the Factory interface, being exposed in 2D.h, does not do any #ifdefing against USE_CAIRO_SCALED_FONT either, despite that it extensively uses cairo_scaled_font_t. The virtual definitions here should cause no real issues such as they are, nor do we use Moz2D ever without Cairo right now, so it would be mostly just theater to introduce the #ifdefs here for that.
I guess it should also be pointed out that USE_CAIRO_SCALED_FONT is defined if USE_SKIA is true, and given that we always require Skia now for building, it is always actually true. So we no longer actually support building without USE_CAIRO_SCALED_FONT.
It was easier to layer this as a patch over all the other patches, as they first cleaned up GetCairoScaledFont and who was calling it.

The only subclass of gfxFont that actually overrode GetCairoScaledFont was gfxDWriteFont. Given that the only remaining callers of this were also in gfxDWriteFonts.cpp, it made most sense to rename the version in gfxDWriteFont and fix the callers to just call that one instead.

With that out of the way, it is safe to then remove the virtual attribute from GetCairoScaledFont.
Flags: needinfo?(lsalzman)
Attachment #8894566 - Flags: review?(jfkthame)
Attachment #8890998 - Flags: review?(jfkthame) → review+
Attachment #8891000 - Flags: review?(jfkthame) → review+
Comment on attachment 8894566 [details] [diff] [review]
remove virtual from gfxFont::GetCairoScaledFont

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

LGTM, thanks.
Attachment #8894566 - Flags: review?(jfkthame) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f43ef75f6d2
make ScaledFont::SetCairoScaledFont virtual so it can be accessed from outside Moz2D. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5db6588684
get rid of gfxFontconfigUtils.h since gfxFontconfigFontBase is unnecessary. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/24c8d3ee219f
remove redundant CairoScaledFont accessor from gfxFont subclasses. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f2be9ad554
require implementation of gfxFont::GetScaledFont and remove unnecessary gfxPlatform::GetScaledFontForFont. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffda55accd9a
remove virtual from gfxFont::GetCairoScaledFont. r=jfkthame
You need to log in before you can comment on or make changes to this bug.