Remove ALERT_MISSING_FONTS code from nsMathMLChar.cpp

RESOLVED FIXED

Status

()

Core
MathML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: zwol, Assigned: zwol)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
The #ifdef ALERT_MISSING_FONTS logic in nsMathMLChar.cpp has been disabled for I don't know how long; would not have done anything useful even if enabled since bug 563114 landed; and is the sole remaining user of two nsIDeviceContext methods (GetLocalFontName and CheckFontExistence) that have never actually done what they claim to do in the Thebes implementation.  I propose to remove it, and to remove the surprisingly large amount of code in nsThebesDeviceContext.cpp that implements those methods, in the process causing device context instances to cost quite a bit more to create than necessary.
(Assignee)

Comment 1

6 years ago
I meant to mention that if this bug is accepted, bug 309090 should be WONTFIXed.
(Assignee)

Comment 2

6 years ago
Created attachment 524344 [details] [diff] [review]
part 1: remove all #ifdef ALERT_MISSING_FONTS code

Does slightly more than what it says on the tin: the CheckFontExistence() helper function was not under #ifdef ALERT_MISSING_FONTS and was called in one place outside those #ifdefs, but thanks to nsThebesDeviceContext::CheckFontExistence() always returning NS_OK, it did nothing constructive.  Possibly more code in nsGlyphTable::ElementAt can be removed, I don't know.
Assignee: nobody → zackw
Status: NEW → ASSIGNED
Attachment #524344 - Flags: review?(karlt)
(Assignee)

Comment 3

6 years ago
Created attachment 524346 [details] [diff] [review]
part 2: make nsIDeviceContext::FirstExistingFont and ::GetLocalFontName into complete stubs; remove the alias table.

I overstated the value of removing this code -- the alias table is not created unless GetLocalFontName or FirstExistingFont is called, so it's not bulking up *every* device context in memory (well, only by one pointer).  Still, it's a lot of code doing nothing useful.

There is one other caller of FirstExistingFont in the tree: accessible/src/msaa/nsTextAccessibleWrap.cpp.  Thanks to CheckFontExistence already being a stub, this change does not change the actual behavior of FirstExistingFont and therefore should not cause problems for that code.  I suspect there are better ways for that to do what it's trying to do, but I don't know what they are and it's not on the critical path here.
Attachment #524346 - Flags: review?(jdaggett)

Comment 4

6 years ago
Comment on attachment 524346 [details] [diff] [review]
part 2: make nsIDeviceContext::FirstExistingFont and ::GetLocalFontName into complete stubs; remove the alias table.

Switching the review to Jeff...
Attachment #524346 - Flags: review?(jdaggett) → review?(bugzmuiz)
(Assignee)

Updated

6 years ago
Blocks: 174055
(Assignee)

Comment 5

6 years ago
FYI, the other use of FirstExistingFont, that I mentioned in comment 3, was removed in bug 648271.
Depends on: 648271
Attachment #524344 - Flags: review?(karlt) → review+
Attachment #524346 - Flags: review?(bugzmuiz) → review+
(Assignee)

Comment 6

6 years ago
http://hg.mozilla.org/mozilla-central/rev/838238a0df33
http://hg.mozilla.org/mozilla-central/rev/0be26016e06a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.