Last Comment Bug 648197 - Remove ALERT_MISSING_FONTS code from nsMathMLChar.cpp
: Remove ALERT_MISSING_FONTS code from nsMathMLChar.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MathML (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Zack Weinberg (:zwol)
:
Mentors:
Depends on: 648271
Blocks: 174055
  Show dependency treegraph
 
Reported: 2011-04-06 20:54 PDT by Zack Weinberg (:zwol)
Modified: 2011-04-14 10:50 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: remove all #ifdef ALERT_MISSING_FONTS code (7.86 KB, patch)
2011-04-06 21:27 PDT, Zack Weinberg (:zwol)
karlt: review+
Details | Diff | Review
part 2: make nsIDeviceContext::FirstExistingFont and ::GetLocalFontName into complete stubs; remove the alias table. (10.61 KB, patch)
2011-04-06 21:33 PDT, Zack Weinberg (:zwol)
roc: review+
Details | Diff | Review

Description Zack Weinberg (:zwol) 2011-04-06 20:54:27 PDT
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.
Comment 1 Zack Weinberg (:zwol) 2011-04-06 20:54:59 PDT
I meant to mention that if this bug is accepted, bug 309090 should be WONTFIXed.
Comment 2 Zack Weinberg (:zwol) 2011-04-06 21:27:31 PDT
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.
Comment 3 Zack Weinberg (:zwol) 2011-04-06 21:33:23 PDT
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.
Comment 4 John Daggett (:jtd) 2011-04-11 10:27:57 PDT
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...
Comment 5 Zack Weinberg (:zwol) 2011-04-13 13:22:41 PDT
FYI, the other use of FirstExistingFont, that I mentioned in comment 3, was removed in bug 648271.

Note You need to log in before you can comment on or make changes to this bug.