Bug 28982 – Basic MathML Rendering Support

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Alex Milowski]

Created attachment 39086 [details]
WebKit patch for MathML Support

This patch adds basic MathML DOM & Rendering support for MathML.  The goals for this patch are:

   https://trac.webkit.org/wiki/MathML%20Goals

This patch does not support msqrt/mroot and mtable.

The build for the Mac platform has been updated but the build/projects for other platforms have not.

[Alexey Proskuryakov]

Please use check-webkit-style on the patch - it's difficult to read at least due to tab/space inconsistencies (you can open it in Safari to see what I'm talking about, some lines look indented 4 spaces, and some 8 spaces).

+	} else {
+        m_separators = StringImpl::create(",");
+    }

We don't use braces around single line control clauses.

+    Length pad((int)(style()->fontSize()*0.1),Fixed);

Please use C++-style casts, add spaces around binary operators, and after commas.

+	RenderInline *open = new (renderArena()) RenderInline(document());

Star goes to the left.

+	RefPtr<StringImpl>  m_separators;

One space before variable name.

+        // TODO: parse units

We use FIXME, not TODO.

+/*
+ *  RenderMathInlineContainer.cpp
+ *  WebCore
+ *
+ *  Created by Alex Milowski on 8/26/09.
+ *  Copyright 2009 Alex Milowski. All rights reserved.
+ *

This is different style than existing code, can you use just a single copyright line?

+RenderMathSubSup::RenderMathSubSup(Node *subsup) : RenderMathInlineContainer(subsup),m_scripts(0)

Initializers go on a separate line, indented four spaces:

+RenderMathSubSup::RenderMathSubSup(Node* subsup)
    : RenderMathInlineContainer(subsup)
    , m_scripts(0)
{
}

+void RenderMathInlineContainer::setStyle(PassRefPtr<RenderStyle> style) {

Opening brace goes on a separate line.

+    //newStyle->setBorderBottomColor(Color(255,0,0));
+    //newStyle->setBorderBottomWidth(1);
+    //newStyle->setBorderBottomStyle(SOLID);

We don't check in commented out code, unless there is a very good reason to (such a reason needs to be given in a comment, of course).

I'm sorry that the feedback is so formal - someone more familiar with adding new markup language support to WebKit will need to comment on the substance of the patch. In the meanwhile, bringing the code in compliance with WebKit coding standards will make the substantive review much easier.

Please mark patches that are up for review as "review?" when submitting, or later by clicking on "Details" link to the right of the patch.

[Alex Milowski]

Created attachment 39189 [details]
Updated patch that adheres to the style guide

[Alex Milowski]

Created attachment 39223 [details]
Update patch with support for the msqrt and mroot elements

[Eric Seidel (no email)]

I really think we shoudl try to do this in smaller pieces.  Any patch over about 20k is difficult to review.

[Alex Milowski]

I'm not quite sure how to break this up.  The bulk of this patch is the new rendering code for MathML.  The rest of the changes are to get the MathML build setup or to enable constructing it in the right places.

I could separate the build and enabling changes from the rendering code.  The unfortunate consequence would be that once the patch is applied you still wouldn't have MathML rendering.  

The current patch should succeed in getting mfrac, mfenced, mover, munder, moverunder, msup, msub msubsup, mrro, msqrt, mi, mn, and mtext all to render somewhat properly.

Suggestions?

[Dave Hyatt]

Comment on attachment 39223 [details]
Update patch with support for the msqrt and mroot elements

You've got a lot of style violations still, so let's get that taken care of.

Please fix the CSS file to be more readable:

selector {
   property/value pairs (one per line);
}

We no longer indent class declarations in header files (many of the files violate this still).  They should just be flush left, e.g.,:

namespace WebCore {

class MathMLElement ....

}

Omit braces when you only have a single line for else statements or for stuff inside a loop, e.g.,

else if (localName()=="mfrac") {
          object = new (arena) RenderFraction(this);
      } 

^^^^ That is wrong.  Just drop the braces.

#endifs traditionally say what they are closing, e.g.,

#endif // MathMLElement_h

#endif // namespace WebCore

Make sure to have spaces in comparators and mathematical expressions, e.g.,:

if (localName()=="mfenced")

should be:

if (localName() == "mfenced")

[Alex Milowski]

This patch has been broken down into a number of other smaller patches: 29158 (fixed), 29143 (fixed), 29349 (fixed), and 29529.  The support for mfenced and mroot/msqrt will come in another patch.