Bug 128907

Summary: Migrate the MathML stretchy code from UChar to Glyph
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, bfulgham, buildbot, cfleizach, commit-queue, darin, dbarton, esprehn+autocc, glenn, gyuyoung.kim, kondapallykalyan, macpherson, menard, mrobinson, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 115786    
Bug Blocks: 122297    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch
none
Patch + 115787 + 115786 for testing
none
Patch for review
none
Patch
none
Patch
cfleizach: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Patch none

Description Frédéric Wang (:fredw) 2014-02-17 06:38:03 PST
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.
Comment 1 Frédéric Wang (:fredw) 2014-02-27 10:16:35 PST
Created attachment 225390 [details]
Patch
Comment 2 Frédéric Wang (:fredw) 2014-03-06 08:46:27 PST
Comment on attachment 225390 [details]
Patch

I'm removing the review request since I'll make this depend on bug 115786.
Comment 3 Frédéric Wang (:fredw) 2014-03-06 09:00:33 PST
Created attachment 225997 [details]
Patch

This one now applies on top of bug 115786.
Comment 4 Frédéric Wang (:fredw) 2014-03-11 02:47:31 PDT
Created attachment 226406 [details]
Patch + 115787 + 115786 for testing
Comment 5 Frédéric Wang (:fredw) 2014-03-11 04:34:41 PDT
Created attachment 226416 [details]
Patch for review
Comment 6 Frédéric Wang (:fredw) 2014-03-12 04:20:18 PDT
Created attachment 226493 [details]
Patch
Comment 7 chris fleizach 2014-03-13 09:13:19 PDT
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
Comment 8 Frédéric Wang (:fredw) 2014-03-13 09:28:09 PDT
(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.
Comment 9 Darin Adler 2014-03-13 11:05:08 PDT
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?
Comment 10 Frédéric Wang (:fredw) 2014-03-13 11:17:47 PDT
(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.
Comment 11 Frédéric Wang (:fredw) 2014-03-13 13:09:12 PDT
Created attachment 226615 [details]
Patch
Comment 12 chris fleizach 2014-03-13 13:22:05 PDT
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)
Comment 13 Frédéric Wang (:fredw) 2014-03-13 13:28:18 PDT
(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.
Comment 14 Build Bot 2014-03-13 21:35:57 PDT
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
Comment 15 Build Bot 2014-03-13 21:36:02 PDT
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
Comment 16 Frédéric Wang (:fredw) 2014-03-13 23:52:24 PDT
Created attachment 226659 [details]
Patch
Comment 17 Frédéric Wang (:fredw) 2014-03-14 01:36:41 PDT
Committed r165608: <http://trac.webkit.org/changeset/165608>