Bug 124841 – Inferred mrows

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

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

Created attachment 217794 [details]
testcase

Some elements should have inferred mrows:

http://www.w3.org/TR/MathML/chapter3.html#presm.inferredmrow

In particular in the attached testcase, all the bars should stretch.

This will also be needed to implement spacing in mrow.

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

Created attachment 217807 [details]
WIP Patch

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

I have attached a patch that solves the problem for all the elements with inferred mrow that we currently support, except mtd.

For mtd (table-cell) I have tried to do as for msqrt: create an anonymous mrow child (flexbox). However, the vertical bar still does not stretch. Any idea about why that does not work?

BTW, I'm no longer able to use xcodebodge.py to edit the XCode file, so any help would be much appreciated.

[Brent Fulgham]

Comment on attachment 217807 [details]
WIP Patch

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

This looks like a good start. I think we should include andersca/kling regarding move semantics. The entire MathML hierarchy should probably be revised to take advantage of more C++11 features.

> Source/WebCore/mathml/MathMLElement.cpp:45
> +}

I believe that this inline will only trigger inside the MathMLElement.cpp file. Uses of "isMathMLToken" outside this compilation unit will not take the 'inline' into account. You should consider moving this to the header file if you want it to be inlined in all compilation units.

> Source/WebCore/mathml/MathMLElement.cpp:131
> +RenderElement* MathMLElement::createRenderer(PassRef<RenderStyle> style)

We should probably do something like "...::createRenderer(RenderStyle&& style)", though I'm not sure how the other instances of this method are defined.

[Brent Fulgham]

andersca/kling: I'm seeing a number of PassRefPtr arguments, and I'm wondering if we can't start using move operators in these places.  Thoughts?

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

While we are here, we may also consider fixing this:

> // FIXME: Change all these createAnonymous... routines to return a PassOwnPtr<>.
> RenderMathMLRow* RenderMathMLRow::createAnonymousWithParentRenderer(const RenderObject* parent)

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

Created attachment 219406 [details]
Patch V1

So I've extracted the easy code changes from the previous patch and added tests for stretchiness and baseline alignment. Actually, I think mstyle was the missing case that is the most likely to happen in practice. As a comparison, Gecko can not stretch operators in mtd at the moment and thus to workaround that bug, most MathML tools/authors wrap the children into an explicit mrow. So let's handle that more involved case in a follow-up patch. It would still be good to know if we can refactor the pointer/ref/move/anonymous stuff.

[Darin Adler]

Comment on attachment 219406 [details]
Patch V1

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

> Source/WebCore/mathml/mathtags.in:13
> +merror interfaceName=MathMLInlineContainerElement

Which interface is used is visible to the DOM, so that should be standardized and tested. We should not use a different interface simply to make rendering different. I can’t tell from this patch if these changes to the element type are correct or incorrect.

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

Comment on attachment 219406 [details]
Patch V1

(In reply to comment #7)
> Which interface is used is visible to the DOM, so that should be standardized and tested. We should not use a different interface simply to make rendering different. I can’t tell from this patch if these changes to the element type are correct or incorrect.

How do you access the interface from the DOM? Last time I tried, it was not possible to distinguish the MathML types. MathML had a DOM in version 2 but it was dropped in version 3 because it has never been implemented in Gecko (this would have added a lot of code & classes) and nobody really complained about that since one can use core DOM anyway.

Here all the elements are containers like mrow, except that they add some additional features (invisibility for mphantom, error style for merror and more attributes for mstyle) so I just used the same container as mrow for consistency.

However, I'm not sure why there is an MathMLInlineContainerElement class at all and why MathMLElement allows presentation attributes on all elements (in general they should only be allowed on math, mstyle and token elements). As a comparison, Gecko only has one nsMathMLElement class and only verifies the tag names to know which attribute should be applied: http://dxr.mozilla.org/mozilla-central/source/content/mathml/content/src/nsMathMLElement.cpp#160

[Darin Adler]

(In reply to comment #8)
> How do you access the interface from the DOM? Last time I tried, it was not possible to distinguish the MathML types.

OK. I didn’t realize MathML was different in this respect.

[WebKit Commit Bot]

Comment on attachment 219406 [details]
Patch V1

Clearing flags on attachment: 219406

Committed r160711: <http://trac.webkit.org/changeset/160711>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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

(In reply to comment #3)
> > Source/WebCore/mathml/MathMLElement.cpp:45
> > +}
> 
> I believe that this inline will only trigger inside the MathMLElement.cpp file. Uses of "isMathMLToken" outside this compilation unit will not take the 'inline' into account. You should consider moving this to the header file if you want it to be inlined in all compilation units.

Will be done in the patch of bug 100626.

[Darin Adler]

Comment on attachment 217807 [details]
WIP Patch

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

>> Source/WebCore/mathml/MathMLElement.cpp:45
>> +}
> 
> I believe that this inline will only trigger inside the MathMLElement.cpp file. Uses of "isMathMLToken" outside this compilation unit will not take the 'inline' into account. You should consider moving this to the header file if you want it to be inlined in all compilation units.

For future reference, the issue in a case like this goes beyond just getting sufficient inlining. It is not legal to call isMathMLToken anywhere outside MathMLElement.cpp if it’s marked inline. With some tool chains it won’t link. Marking it inline and putting it inside the MathMLElement.cpp file is a fine technique when all the callers are inside the MathMLElement.cpp file, though.