Bug 128907 – Migrate the MathML stretchy code from UChar to Glyph

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Created attachment 224362 [details]
WIP Patch

We want to move towards using only fonts with the MATH table so that one only need to specify such a font on the <math> tag (see https://bugzilla.mozilla.org/show_bug.cgi?id=947654 for the Gecko equivalent). The MathML stretchy code currently uses UChar to stretch by parts. This will only work for a few characters that have Unicode constructions like parenthesis or braces but in general we should use Glyph index.

The attached patch tries to replace UChar with Glyph. This seems to work for some fonts but I'll need to test it more. Some remarks:

- I'm assuming that all the characters will use the same SimpleFontData (determined from the base character). In particular, we won't be able to use pieces from different fonts and indeed we don't want that or otherwise the pieces may not glue together perfectly. For fonts with a MATH table, all the pieces come from the same font so that's what we want. However, this won't work with non-MATH fonts like STIX General or MathJax. I don't think we can get the SimpleFontData from a glyph index, so there is not any other choice than using the base character anyway since pieces may be non-Unicode glyphs.

- At the moment, the table of UChar is still used to described construction. However, STIX Math (perhaps others) don't map the glyph to Unicode code points (https://github.com/mathjax/MathJax/wiki/STIX-feedback#wiki-technical-issues) so this method won't work. So we will probably need to use either the MATH table on Linux or some hardcoded tables on other ports.

- We will need to switch to the MATH fonts. I'm assuming that STIX Word (including STIX Math) is installed on Mac OS Lion but perhaps https://github.com/khaledhosny/xits-math/ would be a better choice as it fixes many bugs in STIX. STIX General could be dropped. I'm wondering what is the status on iOS, but Symbol is not a MATH font and should not be used.

- The patch exposes a new drawGlyphs method. I hope it works on all platforms for non-Unicode glyphs.

[Frédéric Wang (:fredw)]

Created attachment 225390 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 225390 [details]
Patch

I'm removing the review request since I'll make this depend on bug 115786.

[Frédéric Wang (:fredw)]

Created attachment 225997 [details]
Patch

This one now applies on top of bug 115786.

[Frédéric Wang (:fredw)]

Created attachment 226406 [details]
Patch + 115787 + 115786 for testing

[Frédéric Wang (:fredw)]

Created attachment 226416 [details]
Patch for review

[Frédéric Wang (:fredw)]

Created attachment 226493 [details]
Patch

[chris fleizach]

Comment on attachment 226493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:353
> +        void drawGlyphs(const Font&, const SimpleFontData*, const GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1);

seems like the order of arguments should be the same as in Font.h

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1285
> +FloatRect RenderMathMLOperator::boundsForGlyph(GlyphData data)

can this be passed as reference

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1290
> +float RenderMathMLOperator::heightForGlyph(GlyphData data)

ditt about reference

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1425
> +    m_stretchyCharacter = assembly;

It's unclear that this method will modify an ivar when you call it. 
I think either the method name should be updated or you should return by reference assembly, so you can store it in m_stretchyCharacter

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:123
> +    GlyphAssembly m_stretchyCharacter;

should probably rename this ivar to something like m_stretchyGlyphAssembly

[Frédéric Wang (:fredw)]

(In reply to comment #7)
> (From update of attachment 226493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:353
> > +        void drawGlyphs(const Font&, const SimpleFontData*, const GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1);
> 
> seems like the order of arguments should be the same as in Font.h
> 

This is to allow default argument values from and numGlyphs. This function was already defined in Font.h, but for some reason the argument were not in the same order as drawText. I wonder if I should update all the calls to drawGlyphs or just give up with trying to have default argument values for GraphicsContext::drawGlyphs...

> > Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1425
> > +    m_stretchyCharacter = assembly;
> 
> It's unclear that this method will modify an ivar when you call it. 
> I think either the method name should be updated or you should return by reference assembly, so you can store it in m_stretchyCharacter
> 
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:123
> > +    GlyphAssembly m_stretchyCharacter;
> 
> should probably rename this ivar to something like m_stretchyGlyphAssembly

I'll try to see what I can do. Note that in bug 122297 this becomes m_stretchyGlyph = a union of (pointer to) a glyph assembly and a glyph variant. Also, findAcceptable* will decide whether glyph assembly or glyph variant is used and will set m_stretchyGlyph appropriately.

[Darin Adler]

Comment on attachment 226493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226493&action=review

r=me but I presume you are going to make some refinements based on Chris’s comments.

>>> Source/WebCore/platform/graphics/GraphicsContext.h:353
>>> +        void drawGlyphs(const Font&, const SimpleFontData*, const GlyphBuffer&, const FloatPoint&, int from = 0, int numGlyphs = 1);
>> 
>> seems like the order of arguments should be the same as in Font.h
> 
> This is to allow default argument values from and numGlyphs. This function was already defined in Font.h, but for some reason the argument were not in the same order as drawText. I wonder if I should update all the calls to drawGlyphs or just give up with trying to have default argument values for GraphicsContext::drawGlyphs...

When adding a new function, we should use references when possible unless something can be null. So it should be const SimpleFontData& unless a null value is legal.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:57
> +static StretchyCharacter stretchyCharacters[14] = {

Why not const?

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1285
>> +FloatRect RenderMathMLOperator::boundsForGlyph(GlyphData data)
> 
> can this be passed as reference

I am not sure that a const GlyphData& is better here. In the past we almost always passed structures by const& but more recently Anders has said that we can get good performance copying simple structures as long as they are truly simple and GlyphData is one of those super-simple structures. Anders, care to comment?

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:73
> +    // FIXME: Ideally (for MATH fonts) we would only need to store the Glyph indices here.

Why is MATH all capitals here? Is that a specific technical term that is always all capitals like that?

[Frédéric Wang (:fredw)]

(In reply to comment #9)
> > Source/WebCore/rendering/mathml/RenderMathMLOperator.h:73
> > +    // FIXME: Ideally (for MATH fonts) we would only need to store the Glyph indices here.
> 
> Why is MATH all capitals here? Is that a specific technical term that is always all capitals like that?

"MATH fonts" means "OpenType fonts with a MATH table", as used in Microsoft's document "The MATH table and OpenType Features for Math Processing" (not public yet) and in the call for proposal http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/call-proposals-isoiec-14496-22-open-font-format-color-font. 

I think M, A, T, H are just the four letters used as the OpenType tag of the corresponding table.

[Frédéric Wang (:fredw)]

Created attachment 226615 [details]
Patch

[chris fleizach]

Comment on attachment 226615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=226615&action=review

thanks

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:111
> +        void setNormal()

I would call these method set*Mode() (setNormalMode(), etc)

[Frédéric Wang (:fredw)]

(In reply to comment #9)
> I am not sure that a const GlyphData& is better here. In the past we almost always passed structures by const& but more recently Anders has said that we can get good performance copying simple structures as long as they are truly simple and GlyphData is one of those super-simple structures. Anders, care to comment?

I'll wait tomorrow, in case Anders has a comment on these.

[Build Bot]

Comment on attachment 226615 [details]
Patch

Attachment 226615 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5275410250072064

New failing tests:
mathml/very-large-stretchy-operators.html
mathml/presentation/mo-stretch.html

[Build Bot]

Created attachment 226649 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Frédéric Wang (:fredw)]

Created attachment 226659 [details]
Patch

[Frédéric Wang (:fredw)]

Committed r165608: <http://trac.webkit.org/changeset/165608>