Closed Bug 1066043 Opened 5 years ago Closed 5 years ago

gfxFont.cpp/.h is way too big; split it into several parts

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(1 file, 1 obsolete file)

(See bug 1065002 comment 4.)

gfxFont.cpp is 8K lines; the gfxFont.h header is nearly 4K! That's far too big.

I propose to split this into several more manageable files; something like this:

gfxFontEntry.cpp
- the gfxFontEntry and gfxFontFamily classes.

gfxFont.cpp
- the gfxFont and gfxFontCache classes.

gfxGlyphExtents.cpp
- exactly what it says on the tin.

gfxTextRun.cpp
- the gfxShapedText, gfxShapedWord and gfxTextRun classes; also gfxFontGroup, which uses things defined in gfxTextRun. (Or maybe split that out later, too?)

There are some other smaller helper structs and classes, which will stay in the most appropriate of the above files.

We'll need to update #include lists in various places to work with this; in particular, code that wants to use textruns or fontgroups will need to start including gfxTextRun.h in addition to (or instead of) gfxFont.h.

I've got a patch along the above lines that builds fine on OS X (in both unified and non-unified modes), but need to handle #include updates needed for other platforms as well before posting for review.
Blocks: 1065002
Sounds great!
This splits up gfxFont.cpp and gfxFont.h as outlined above, and fixes up #includes and forward-declarations as needed to build across all platforms.
Attachment #8488876 - Flags: review?(jdaggett)
Try runs to confirm this builds successfully:
https://tbpl.mozilla.org/?tree=Try&rev=0c5b35d06461 (non-unified build)
https://tbpl.mozilla.org/?tree=Try&rev=1cf56194990a (unified build)
It turns out to be more sensible to keep gfxShapedText and gfxShapedWord in gfxFont.cpp/.h, because of gfxFont's shaped-word cache, so I moved them back from gfxTextRun.cpp/.h. New try build: https://tbpl.mozilla.org/?tree=Try&rev=b5335dd5ed10.
Attachment #8489117 - Flags: review?(jdaggett)
Attachment #8488876 - Attachment is obsolete: true
Attachment #8488876 - Flags: review?(jdaggett)
Attachment #8489117 - Flags: review?(jdaggett) → review+
Blocks: 998869
It would be helpful to get this landed. I have a bunch of patches for bug 998869 that will need to be reworked for the new set of files.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9528e6149978

Yes, this will break gfxFont-related patches currently in flight. In most cases, I've found that it's pretty easy to "rebase" by manually adding/changing diff headers for the chunks of the patch, depending which classes are actually being modified, as the actual code isn't changed, just moved to a different file.
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.