[Azure] Refactor the gfxFT2* font classes to separate better between FreeType and Cairo

RESOLVED INVALID

Status

()

RESOLVED INVALID
7 years ago
5 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: gw280)

Tracking

(Depends on: 1 bug)

Trunk
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 631221 [details] [diff] [review]
Separate FreeType and Cairo classes
Created attachment 631222 [details] [diff] [review]
Keep Cairo fonts hanging around so we don't need to keep recreating them
Attachment #631221 - Flags: review?(karlt)
Attachment #631222 - Flags: review?(karlt)
Blocks: 761895
Attachment #631221 - Flags: review?(karlt)
Depends on: 738014
Attachment #631222 - Flags: review?(karlt)
Created attachment 637692 [details] [diff] [review]
refactor fonts

Hopefully something close to what I can land.

Pushed a full try build with talos to https://tbpl.mozilla.org/?tree=Try&rev=d3d311ee87fe
Attachment #631221 - Attachment is obsolete: true
Attachment #631222 - Attachment is obsolete: true
Attachment #637692 - Flags: review?(karlt)
Attachment #637692 - Flags: review?(jdaggett)

Comment 3

6 years ago
Comment on attachment 637692 [details] [diff] [review]
refactor fonts

I think it might be better for Jonathan to review this.
Attachment #637692 - Flags: review?(jdaggett) → review?(jfkthame)
(Assignee)

Updated

6 years ago
Depends on: 703042
Created attachment 642262 [details] [diff] [review]
Small patch in order to fix compilation with Qt port with pango-disabled
Attachment #642262 - Flags: review?
Comment on attachment 637692 [details] [diff] [review]
refactor fonts

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

Not a review yet (sorry), just some thoughts for possible consideration. I'm still struggling to get my head around the tangle of classes involved here (I don't mean just this patch, but our existing mess...)

It seems to me that gfxFT2Extents is mis-named, as it's not just an object that represents font (or glyph) extents, or even provides an interface (just) for extents - it's more like a gfxFT2FontAccessor, as it provides char/glyph mapping and font table access as well as extents. However, it's not obvious to me why we'd want to split this out from gfxFT2FontBase as a separate object at all.

I'm a bit confused too as to why gfxCairoFTFont is a separate subclass of gfxFont, rather than being a subclass of gfxFT2FontBase; ISTM that we should be able to have gfxFT2FontBase to handle everything that can be shared by all FreeType-based font backends, like getting tables and metrics from the FT_Face, and then gfxCairoFTFont would be a subclass of this, along with any other FT2-based implementations (do we need gfxSkiaFT2Font?).
(In reply to Jonathan Kew (:jfkthame) from comment #5)
> Comment on attachment 637692 [details] [diff] [review]
> refactor fonts
> 
> Review of attachment 637692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not a review yet (sorry), just some thoughts for possible consideration. I'm
> still struggling to get my head around the tangle of classes involved here
> (I don't mean just this patch, but our existing mess...)
> 
> It seems to me that gfxFT2Extents is mis-named, as it's not just an object
> that represents font (or glyph) extents, or even provides an interface
> (just) for extents - it's more like a gfxFT2FontAccessor, as it provides
> char/glyph mapping and font table access as well as extents. However, it's
> not obvious to me why we'd want to split this out from gfxFT2FontBase as a
> separate object at all.

I think ultimately I just want this class to allow us to use the current backend's metrics calculators. Ideally we want to push this functionality into Azure but it's a little too early for that, so this was the compromise I came up with. The reason we can't just calculate the metrics ourselves using FreeType directly is that we suffer a huge performance hit if we don't use our current rendering backend to calculate the metrics; this is because the backend will cache it internally and we need that. Even if we cache it ourselves, the backend is still going to calculate/cache it when we ask it to render, so it seems sensible to let it handle it all the time for us.

All of the non-extents methods in the extents class can/should be moved into gfxFT2Utils, and the callers appropriately modified to call it there instead.

> I'm a bit confused too as to why gfxCairoFTFont is a separate subclass of
> gfxFont, rather than being a subclass of gfxFT2FontBase; ISTM that we should
> be able to have gfxFT2FontBase to handle everything that can be shared by
> all FreeType-based font backends, like getting tables and metrics from the
> FT_Face, and then gfxCairoFTFont would be a subclass of this, along with any
> other FT2-based implementations (do we need gfxSkiaFT2Font?).

That's the way the class setup used to be in a previous iteration of the patch, but off-hand I can't remember why it switched to the way it is now. I suspect it was due to extents calculation before I had the Extents class. I think we should be able to make the hierarchy like that again, which would make far more sense.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 738014
(Assignee)

Updated

6 years ago
Assignee: ajones → gwright
(In reply to George Wright (:gw280) from comment #6)
> (In reply to Jonathan Kew (:jfkthame) from comment #5)
> > Comment on attachment 637692 [details] [diff] [review]
> > refactor fonts
> > 
> > Review of attachment 637692 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Not a review yet (sorry), just some thoughts for possible consideration. I'm
> > still struggling to get my head around the tangle of classes involved here
> > (I don't mean just this patch, but our existing mess...)
> > 
> > It seems to me that gfxFT2Extents is mis-named, as it's not just an object
> > that represents font (or glyph) extents, or even provides an interface
> > (just) for extents - it's more like a gfxFT2FontAccessor, as it provides
> > char/glyph mapping and font table access as well as extents. However, it's
> > not obvious to me why we'd want to split this out from gfxFT2FontBase as a
> > separate object at all.
> 
> I think ultimately I just want this class to allow us to use the current
> backend's metrics calculators. Ideally we want to push this functionality
> into Azure but it's a little too early for that, so this was the compromise
> I came up with. The reason we can't just calculate the metrics ourselves
> using FreeType directly is that we suffer a huge performance hit if we don't
> use our current rendering backend to calculate the metrics; this is because
> the backend will cache it internally and we need that. Even if we cache it
> ourselves, the backend is still going to calculate/cache it when we ask it
> to render, so it seems sensible to let it handle it all the time for us.

Hrm, thinking about this more, I'm starting to think that your suggestion of just renaming the class would be a better idea. I feel this is mainly because all the operations require the FT_Face object, and the different backends will have different ways of accessing the FT_Face represented by the native object; specifically, cairo requires that we lock the scaled font, use the face, then unlock it, and discard our reference to that face. Also things such as GetGlyph we will likely want to use the backend to do, rather than freetype, for caching reasons.
Closing as we decided against doing this.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
Attachment #637692 - Flags: review?(karlt)
Attachment #637692 - Flags: review?(jfkthame)
Attachment #642262 - Flags: review?
You need to log in before you can comment on or make changes to this bug.