Closed
Bug 67091
Opened 24 years ago
Closed 24 years ago
[static ctor]remove static ctors from nsMathMLChar.cpp
Categories
(Core :: MathML, defect, P3)
Core
MathML
Tracking
()
RESOLVED
FIXED
mozilla0.8
People
(Reporter: dbaron, Assigned: rbs)
Details
Attachments
(2 files)
16.58 KB,
patch
|
Details | Diff | Splinter Review | |
107.42 KB,
text/plain
|
Details |
I'm working on a patch to remove static constructors from nsMathMLChar.cpp. My boilerplate description of why static constructors are bad: Static constructorsare against the C++ portability guidelines http://www.mozilla.org/hacking/portable-cpp.html , cause objects to show up as a leak, and prevent us from running on OpenBSD (bug 49575), and prevents us from doing module unloading (see bug 60709).
Reporter | ||
Updated•24 years ago
|
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla0.8
Grabbing the bug... In fact, I have already removed them from my tree as part of a wider work. I would like to land this work and get rid of these static ctors in my next check-in. Here is the post that I sent to n.p.m.mathml to the effect under the header "Coming up: MathFont Property Files". "Roger B. Sidje" wrote: > > Changes in my tree include a revamp of the handling of the Operator > Dictionary (nsMathMLOperators) and the stretchy characters (nsMathMLChar). > The changes consist of the total removal of the hard-coding that were > in these files. With the current code, the Operator Dictionary is there > sitting in memory even if the user never hits a page with MathML markup. > With the changes, the Operator Dictionary is now described in an external > "Mozilla MathFont Property File" (mathfont.properties) which is loaded lazily > upon occurrence of a first operator. Moreover, since this is not hard-coded > anymore, users can add their own customizations, such as changing attributes > to fit their taste, or adding tailor-made operators. The readable format used > in the dictionnary makes this simple. For example, the parenthesis (U+0028) > is listed as: > operator.\u0028.prefix = fence:true stretchy:vertical lspace:0em rspace:0em # ( > > Also out are the static ctors and the hard-coding of the mutable characters > in nsMathMLChar.cpp. Rather than including the list of stretchy chars in > nsMathMLCharList.h at compile-time, users can list the variants of larger > sizes or the partial glyphs needed to build up a character in external Mozilla > MathFont Property Files. For example, the glyph data for the "Symbol" font and the > "MT Extra" font are in mathfontSymbol.properties and mathfontMTExtra.properties. > (The format of these files closely follows the scheme used in nsMathMLCharList.h.) > > These changes also enable to externally specify the overall ordered list of > fonts to try when stretching a character, as well as the preferred extension > fonts for a particular character. For example, to specify that the parenthesis > (U+0028) should only be stretched with partial glyphs from the Mathematica > Math4 font, the mathfont.properties will include the line: > extension.\u0028.parts = Math4 > > Similarly, to specify that larger variants of the sqrt (U+221A) should > only be taken from CMEX10, the mathfont.properties will include the line: > extension.\u221A.variants = CMEX10 > > Thus customizations can be made globally as well as down to the character > level. But unlike authored css files, the customizations are on the user' > side (user specific). A property is ignored if the given font is not installed. > The default properties could be tuned depending on the default shipped fonts. > > Code-wise, the setup needed for these is built lazily, and rely on > light-weight keys. Thus no overhead is incurred, compared to the current > setup. On the contrary, since things are now created lazily, a user that > never hits a MathML-authored page is freed from any unnecessary impact. > > The Mozilla MathFont Property Files will live in the "path-to/bin/res/fonts/" > directory, accessible under the URI: "resource:/res/fonts/". > --- > RBS
Assignee: dbaron → rbs
Status: ASSIGNED → NEW
Reporter | ||
Comment 2•24 years ago
|
||
dbaron, you have been quick and pro-active... I have attached some of the work I was referring to. Thanks anyway. Let me know if you have some comments about the patch. The work is much involved, but any comments on the style (and portability issues) will be okay too.
Reporter | ||
Comment 5•24 years ago
|
||
When I attached the patch I hadn't even seen your comment. :-) One quick comment on your patch - it would probably be better to do the shutdown based on the layout module destructor (see my patch) rather than an XPCOM shutdown observer. It would also be good to put these classes into the leak logs (I was going to add that to the patch I attached). This can be done in the following way: MOZ_DECL_CTOR_COUNTER(nsFoo) nsFoo::nsFoo() { MOZ_COUNT_CTOR(nsFoo); } nsFoo::~nsFoo() { MOZ_COUNT_DTOR(nsFoo); }
I was refraining from adding more bindings to core layout since MathML is intended to be a "drop-in" component someday. I was even hoping to remove the nsMathMLOperators::[Add/Release]Table() from the layout module, since I have reworked that code to allow lazy instantiation of the Operator Dictionary. But the problem is yet again with the release. It is a pity that there is no way to register to layout and be notified of its shutdown. Do you anticipate any problems if I leave the current xpcom shutdown listener in nsMathMLChar in view of registering to layout someday.
Reporter | ||
Comment 7•24 years ago
|
||
Not really, although if we ever go to unloading modules when they're not used (this has been proposed, see bug 45613, then it might be better to use the module destructor. Regarding switching MathML to a separate module - pulling the code out of the factory is just one of the things that will have to happen then. It always must be done when modules are split or combined. Then again, I don't really care, I just prefer module destructors over XPCOM shutdown observers because it seems like less code and less time spent doing it...
dbaron, I have implemented your preference. It is indeed at lot simpler... Just had to add a oneliner function FreeGlobals() to call gGlyphTableList->Finalize() that I already had... However, since some changes are added in layout, I may spend the rest of the month waiting for layout folks to have a look at it :-) So I will be check-in what I have (the non-layout dependent xpcom version), knowing that I can later switch over to the other version, and that there could be a mathmlStartUp() & mathmlShutDown() that will someday provide single entry points for the global request and release of math-related resources.
Code was checked in. Resolving as FIXED.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•