118600 - parameterize the font used for stretchy characters at their base size

Status: RESOLVED FIXED

(Core :: MathML, defect)

Description

[rbs]

[spin off of bug 118475]
There are several fonts involved in the processing of stretchy characters. In 
addition to fonts used for larger glyphs or partial glyphs, there is the font 
used for the base size (i.e., in the situation where stretching happens to be 
not necessary). However, the handling of this font hasn't received as much 
attention as the others. As a result, there is a piece of code in 
nsMathMLChar.cpp that reads:

  PRUnichar uchar = mData[0];
  if (kSqrChar == uchar) {       // Special to the sqrt char. Due to assumptions
    fontName.Assign(NS_LITERAL_STRING("CMSY10"));   // in the sqrt code, we need
    SetFirstFamily(theFont, fontName);   // to force precedence on this TeX font
  }
  aRenderingContext.SetFont(theFont);

It should be possible to get of rid of this hardcoding by parameterizing the 
handling of the base font as well, thereby allowing a generalization.

My thoughts about implementating this are as follow:

In mathfont.properties & nsMathMLChar.cpp, in addition to supporting
mathfont-family.\uNNNN.parts    = font-family-list #preferred for partial glyphs
mathfont-family.\uNNNN.variants = font-family-list #preferred for larger glyphs

where 'NNNN' is the Unicode point of the stretchy character, add the support for 
mathfont-family.\uNNNN          = font-family-list #preferred for the base size

as well as the support for prefs to override:
pref("font.mathfont-family.\uNNNN", "font-family-list")
pref("font.mathfont-family.\uNNNN.parts", "font-family-list")
pref("font.mathfont-family.\uNNNN.variants", "font-family-list")

This could be implemented by maintaining a nsStringArray gBaseFonts such that  
gBaseFonts[rankOfStretchyChar(\uNNNN)] gives the list of fonts to be used for 
the base size of that particular character.

Comment 1

[rbs]

Or better, mantain a common list of base font names gBaseFontNames[], and have 
pointers to them as nsVoidArray gBaseFonts, to avoid the same string to be 
duplicated for different stretchy characters. This is somewhat how the preferred 
fonts for ".variants" and ".parts" are supported at the moment - the difference 
is that they point to glyph tables (instead of font names).

Comment 2

[rbs]

Attachment: patch using nsDataHashtable - doesn't compile

Seems like some voodoo is needed to use the newish hashtable incatations. Works
okay with the old nsHashtable, but I am getting the following link errors with
nsDataHashtable. Note: there are some leaks in the patch -- not call to Clear()
yet. And BTW, it is up to the client to delete the values, right? I.e., there
is no callback to delete?

   Creating library gklayout.lib and object gklayout.exp
gkmathmlbase_s.lib(nsMathMLChar.obj) : error LNK2001: unresolved external
symbol
 "public: __thiscall nsBaseHashtable<class nsUint32HashKey,unsigned short
*,unsi
gned short *>::~nsBaseHashtable<class nsUint32HashKey,unsigned short *,unsigned

short *>(void)" (??1?$nsBaseHashtable@VnsUint32HashKey@@PAGPAG@@QAE@XZ)
gkmathmlbase_s.lib(nsMathMLChar.obj) : error LNK2001: unresolved external
symbol
 "public: __thiscall nsBaseHashtable<class nsUint32HashKey,unsigned short
*,unsi
gned short *>::nsBaseHashtable<class nsUint32HashKey,unsigned short *,unsigned
s
hort *>(void)" (??0?$nsBaseHashtable@VnsUint32HashKey@@PAGPAG@@QAE@XZ)
gkmathmlbase_s.lib(nsMathMLChar.obj) : error LNK2001: unresolved external
symbol
 "public: int __thiscall nsBaseHashtable<class nsUint32HashKey,unsigned short
*,
unsigned short *>::Init(unsigned int,int)"
(?Init@?$nsBaseHashtable@VnsUint32Has
hKey@@PAGPAG@@QAEHIH@Z)
gkmathmlbase_s.lib(nsMathMLChar.obj) : error LNK2001: unresolved external
symbol
 "public: int __thiscall nsBaseHashtable<class nsUint32HashKey,unsigned short
*,
unsigned short *>::Put(unsigned int const &,unsigned short *)"
(?Put@?$nsBaseHas
htable@VnsUint32HashKey@@PAGPAG@@QAEHABIPAG@Z)
gkmathmlbase_s.lib(nsMathMLChar.obj) : error LNK2001: unresolved external
symbol
 "public: int __thiscall nsBaseHashtable<class nsUint32HashKey,unsigned short
*,
unsigned short *>::Get(unsigned int const &,unsigned short * *)const "
(?Get@?$n
sBaseHashtable@VnsUint32HashKey@@PAGPAG@@QBEHABIPAPAG@Z)
gklayout.dll : fatal error LNK1120: 5 unresolved externals
make[2]: *** [gklayout.dll] Error 96
make[2]: Leaving directory `/cygdrive/d/Mozilla/trunk/mozilla/layout/build'
make[1]: *** [libs] Error 2
make[1]: Leaving directory `/cygdrive/d/Mozilla/trunk/mozilla/layout'
make: *** [all] Error 2

Comment 3

[rbs]

bsmedberg, any clues?

Comment 4

[rbs]

OK bsmedberg, compile fine after updating my xpcom directory to the latest and
greatest from bug 200709 per your email. Now, remains to figure out what to do
with my values re: Clear(). Should I really manually iterate and delete them? No
callback to delete?

Comment 5

[rbs]

I am going back to nsHashtable which has the Reset(aCallback) that I want. The
newish stuff isn't for me at present.

Comment 6

[Benjamin Smedberg]

rbs: Why doesn't the Clear() method work?

Comment 7

[rbs]

My hashtable is:
+  static nsDataHashtable<nsUint32HashKey, PRUnichar*> gBaseFonts;

So, I have to delete the actual string to this |PRUnichar*| point to.

Such an explicit deletion wouldn't be needed if the hashtable was, for example,
static nsDataHashtable<nsUint32HashKey, PRUnichar> gBaseFonts;

Comment 8

[rbs]

Actually, maybe I should use two calls:

gBaseFonts.EnumerateEntries(myDeleteCallback)
gBaseFonts.Clear().

[Suggesting to have a Clear(aCallback) since that's what people have been used too]

Comment 9

[Benjamin Smedberg]

rbs: when bug 201034 is in (soon) you can do it in one step... just call
Enumerate() with a callback that deletes the string and returns PL_DHASH_REMOVE

Comment 10

[rbs]

I am running into all sorts of compiler troubles when trying EnumerateEntries()

I have:

static nsDataHashtable<nsUint32HashKey, PRUnichar*> gBaseFonts;

And wish to do:

  gBaseFonts.EnumerateEntries(FreeBaseFontCallback, nsnull);
  gBaseFonts.Clear();

How to I declare |FreeBaseFontCallback|?

This doesn't work:
nsDataHashtable<nsUint32HashKey,PRUnichar*>::Enumerator
FreeBaseFontCallback(nsDataHashtable<nsUint32HashKey,PRUnichar*>* aTable, 
                     void* aData) 
{
  return PL_DHASH_NEXT;
}

Nor does this one:
PR_STATIC_CALLBACK(PLDHashOperator)
FreeBaseFontCallback(nsDataHashtable<nsUint32HashKey,PRUnichar*>* aTable, 
                     void* aEntry) 
{
  return PL_DHASH_NEXT;
}

Comment 11

[rbs]

Attachment: patch in progress - doesn't compile

patch which you can apply to your tree for experimentations. Simply cd to
layout/mathml/base/src and apply the patch & make from there.

Comment 12

[rbs]

Comment on attachment 120901 [details] [diff] [review]
patch in progress - doesn't compile

Didn't notice that there were too much cruft for my own testing in the patch...
I will attach another one.

Comment 13

[rbs]

Attachment: intended patch for experimentation - doesn't compile

Comment 14

[rbs]

Attachment: compile/link okay (from top layout/) with enumerate commented out

Comment 15

[rbs]

Attachment: working patch with nsDoubleHashtable

I tried hard to use the newish hashtable stuff, but no luck. Too much time on
it without gain. Going back to the older variant that works.

Comment 16

[rbs]

Comment on attachment 121033 [details] [diff] [review]
working patch with nsDoubleHashtable

ready for r/sr.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sanity check: what does this extra flexibility really buy us? Who is really
going to need to change these prefs?

Comment 18

[rbs]

Some people are reporting all kinds of troubles with certain fonts/characters.
These troubles seem to happen depending on their OS/GDI or font engines (e.g.,
FreeType, Xft):

Here are some excerpts from bug 120198: [trk] Rendering errors on MathML demos:

"less than symbol doesn't render correctly, example #11 on torture page"
http://bugzilla.mozilla.org/attachment.cgi?id=80132&action=view

"Screenshot showing black boxes instead of "/" symbols"
http://bugzilla.mozilla.org/attachment.cgi?id=92558&action=view

"right part of the underbrace doesn't render correctly, mathml torture page #22"
http://bugzilla.mozilla.org/attachment.cgi?id=99389&action=view

If things are just hard-coded, users are stuck until the next release (which in
the case of Nav7 takes years, sadly). Whereas a pref will provides a way out,
especially in those cases where it is unreproducible on my box and would have
been hard to believe without such screenshots.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 121033 [details] [diff] [review]
working patch with nsDoubleHashtable

Why is this in the internal properties path but not in the user properties
path?

(0 == key.Find("font.mathfont-family.\\u"))

Couldn't this pref handling be factored out to handle both the user and
internal cases?

Other than that, it looks good. Marking r+sr in anticipation of persuasive
answers to the above questions :-)

Comment 20

[rbs]

> (0 == key.Find("font.mathfont-family.\\u"))

There is nice pref API that is alredy doing the pruning in the pref tree:
+  prefBranch->GetChildList("font.mathfont-family.", &count, &allKey);    

> Couldn't this pref handling be factored out to handle both the user and
> internal cases?

The pref system loads user prefs for free, whereas I have to manually load the
internal settings (while giving precedence to user prefs). However, once the
hashtable is populated with the precedence respected, the rest becomes straight
lookups.

Comment 21

[rbs]

Checked in.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> There is nice pref API

Yeah, but that's not the same because it doesn't check for "\u". You're in the
strange situation of doing more validation on your internal properties than on
the user-specified properties.

> The pref system loads user prefs for free

I meant specifically that the code that parses the pref name to get the unicode
char and the extension could have been factored out.

Comment 23

[rbs]

Attachment: extra consolidation patch

>Yeah, but that's not the same because it doesn't check for "\u". You're in the

>strange situation of doing more validation on your internal properties than on

>the user-specified properties.

It is "font.mathfont-family." vs "font.mathfont-family.\\u"
My check is string-based and so I can afford the extra "\\u", whereas the pref
API is (pref)tree-based. Not a big deal, though.

>I meant specifically that the code that parses the pref name to get the
unicode
>char and the extension could have been factored out.

Ops... got what you meant now. Done. Will check the requested fixup when the
tree opens.

Comment 24

[rbs]

Comment on attachment 121286 [details] [diff] [review]
extra consolidation patch

Update was checked in.

Comment 25

[rbs]

*** Bug 205613 has been marked as a duplicate of this bug. ***