Bug 99620 – [MathML] Implement the MathML Operator Dictionary

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Dave Barton]

The operator dictionary includes default spacing values (CSS margins?) around various infix, prefix, and postfix operators. See <http://www.w3.org/TR/MathML3/appendixc.html>.

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

Created attachment 217299 [details]
WIP Patch

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

Created attachment 217441 [details]
WIP Patch

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

So I've attached a WIP patch for this bug.

I'd like to get feedback about how to create the Operator Dictionary. IIRC, there are ~1000 entries so I've used a hash table. I have an XSLT stylesheet to extract them from the big unicode.xml of http://www.w3.org/TR/xml-entity-names/ and I can easily convert into whatever formats.

I wonder if I should hardcode the table in some (probably separate) C++ file (using macros?) or in a text file that is parsed at init. Should I include the generation of the dictionary in the build system? What is the standard way to do such thing in WebKit?

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

Created attachment 217574 [details]
WIP Patch

[Darin Adler]

I think a sorted constant array compiled into the binary, where we do lookups with a binary search, would use much less memory than a hash table, be much smaller than the giant set-up function, not require a special shell script or dependency on xslt, and still have quite good performance.

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

Created attachment 217656 [details]
WIP Patch

Thanks Darin, I've rewritten the code using a hardcoded table and binary search.

My next issue is to find how to detect the operator form automatically (infix, prefix, postfix). At the moment, it seems to always be postfix when it is not explicitly specified in the MathML code. I'm guessing that when the <mo> renderer is being constructed, it is appended at the end of its parent <mrow> renderer and so does not have any next siblings yet, and so is "postfix". It looks like it will be time to refactor the code with updateFromElement and to make addChild/removeChild works with dynamic changes. I'll come back to this next week but any suggestion is welcome.

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

Created attachment 217657 [details]
script to generate the operator dictionary

For the record, here is the script that generates the operator dictionary.

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

Created attachment 217786 [details]
script to generate the operator dictionary

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

Created attachment 217787 [details]
WIP Patch

Here is an updated patch.

I have tried to use updateFromElement on mrow-like containers. That seems to update the operator form correctly, but the CSS style seems to be lost at the update. Hence I'll open a new bug to do code refactoring for the token elements first and will come back to this bug later. I also suspect all the mrow-like elements do not behave correctly at the moment.

This patch also adds several new tests and references to follow-up bugs.

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

I think I'll extract the changes and ignore the lspace/rspace support until I get feedback on bug 124838. So the op dict will only be used for the stretchy value and forms to start with. This means that many of the tests that relied on the spacing will be removed too.

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

Created attachment 224610 [details]
Patch V1

[WebKit Commit Bot]

Attachment 224610 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/mathml/RenderMathMLFenced.h:50:  The parameter name "form" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/rendering/mathml/RenderMathMLFenced.h:50:  The parameter name "flag" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

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

Created attachment 224614 [details]
Patch V2

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

Created attachment 224622 [details]
Patch V3

[chris fleizach]

Comment on attachment 224622 [details]
Patch V3

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

looks pretty good overall to me. some minor comments inside

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:66
> +namespace MathMLOpDict {

I think it might be better to use the unabbreviated forms for op, so that this is
MathMLOperatorDictionary

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:70
> +inline UChar ExtractChar(const Entry* aEntry) { return aEntry->character; }

parameter name for these methods should probably just be "entry" instead of aEntry, which isn't used so much in WebCore

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1145
> +    if (element().getAttribute(name) == "true")

should be fastGetAttribute(). you might want to cache the value so you don't have to call it again for the false case

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1152
> +void RenderMathMLOperator::setOperatorPropertiesFromOpDictEntry(const MathMLOpDict::Entry *entry)

* on wrong side

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1179
> +        if (element().getAttribute(MathMLNames::formAttr) == "prefix")

fastGetAttribute and cache the result in a variable

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1239
> +        String size = element().getAttribute(MathMLNames::minsizeAttr);

fastGetAttr

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:1241
> +        size = element().getAttribute(MathMLNames::maxsizeAttr);

ditto

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:119
> +    bool m_isFenceOp;

this should be isFenceOperator I think

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

Created attachment 224657 [details]
Patch V4

[chris fleizach]

Comment on attachment 224657 [details]
Patch V4

thanks

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

Committed r164418: <http://trac.webkit.org/changeset/164418>

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

Mass change: add WebExposed keyword to help MDN documentation.