Closed
Bug 762737
Opened 12 years ago
Closed 12 years ago
[Azure] Refactor the gfxFT2* font classes to separate better between FreeType and Cairo
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
INVALID
People
(Reporter: ajones, Assigned: gw280)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Attachment #631221 -
Flags: review?(karlt)
Reporter | ||
Updated•12 years ago
|
Attachment #631222 -
Flags: review?(karlt)
Reporter | ||
Updated•12 years ago
|
Attachment #631221 -
Flags: review?(karlt)
Reporter | ||
Updated•12 years ago
|
Attachment #631222 -
Flags: review?(karlt)
Assignee | ||
Comment 2•12 years ago
|
||
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•12 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)
Comment 4•12 years ago
|
||
Attachment #642262 -
Flags: review?
Comment 5•12 years ago
|
||
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?).
Assignee | ||
Comment 6•12 years ago
|
||
(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•12 years ago
|
Assignee: ajones → gwright
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Comment 9•12 years ago
|
||
Closing as we decided against doing this.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Updated•12 years ago
|
Attachment #637692 -
Flags: review?(karlt)
Attachment #637692 -
Flags: review?(jfkthame)
Updated•11 years ago
|
Attachment #642262 -
Flags: review?
You need to log in
before you can comment on or make changes to this bug.
Description
•