Bug 128907 - Migrate the MathML stretchy code from UChar to Glyph
Migrate the MathML stretchy code from UChar to Glyph
 Status: RESOLVED FIXED None WebKit Unclassified MathML (show other bugs) 528+ (Nightly build) All All P2 Normal Frédéric Wang (:fredw) 115786 122297 Show dependency tree / graph

 Reported: 2014-02-17 06:38 PST by Frédéric Wang (:fredw) 2014-03-14 01:36 PDT (History) 15 users (show) andersca bfulgham buildbot cfleizach commit-queue darin dbarton esprehn+autocc glenn gyuyoung.kim kondapallykalyan macpherson menard mrobinson rniwa

Attachments
WIP Patch (20.87 KB, patch)
2014-02-17 06:38 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (21.59 KB, patch)
2014-02-27 10:16 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (22.07 KB, patch)
2014-03-06 09:00 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch + 115787 + 115786 for testing (96.17 KB, patch)
2014-03-11 02:47 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for review (22.07 KB, patch)
2014-03-11 04:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (22.43 KB, patch)
2014-03-12 04:20 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (24.72 KB, patch)
2014-03-13 13:09 PDT, Frédéric Wang (:fredw)
cfleizach: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (786.25 KB, application/zip)
2014-03-13 21:36 PDT, Build Bot
no flags Details
Patch (24.89 KB, patch)
2014-03-13 23:52 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

 Note You need to log in before you can comment on or make changes to this bug.
 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 [itex] 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) 2014-02-27 10:16:35 PST Created attachment 225390 [details] Patch 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. 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. Frédéric Wang (:fredw) 2014-03-11 02:47:31 PDT Created attachment 226406 [details] Patch + 115787 + 115786 for testing Frédéric Wang (:fredw) 2014-03-11 04:34:41 PDT Created attachment 226416 [details] Patch for review Frédéric Wang (:fredw) 2014-03-12 04:20:18 PDT Created attachment 226493 [details] Patch 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 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. 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? 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. Frédéric Wang (:fredw) 2014-03-13 13:09:12 PDT Created attachment 226615 [details] Patch 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) 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. 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 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 Frédéric Wang (:fredw) 2014-03-13 23:52:24 PDT Created attachment 226659 [details] Patch Frédéric Wang (:fredw) 2014-03-14 01:36:41 PDT Committed r165608: