Bug 124841 - Inferred mrows
Summary: Inferred mrows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on:
Blocks: 120164 125860 99620
  Show dependency treegraph
 
Reported: 2013-11-25 05:26 PST by Frédéric Wang (:fredw)
Modified: 2013-12-20 09:56 PST (History)
14 users (show)

See Also:


Attachments
testcase (981 bytes, text/html)
2013-11-25 05:26 PST, Frédéric Wang (:fredw)
no flags Details
WIP Patch (15.05 KB, patch)
2013-11-25 08:13 PST, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch V1 (10.26 KB, patch)
2013-12-17 02:42 PST, 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.
Description Frédéric Wang (:fredw) 2013-11-25 05:26:33 PST
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.
Comment 1 Frédéric Wang (:fredw) 2013-11-25 08:13:51 PST
Created attachment 217807 [details]
WIP Patch
Comment 2 Frédéric Wang (:fredw) 2013-11-25 08:15:48 PST
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.
Comment 3 Brent Fulgham 2013-11-26 16:33:43 PST
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.
Comment 4 Brent Fulgham 2013-11-26 16:34:38 PST
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?
Comment 5 Frédéric Wang (:fredw) 2013-11-26 22:09:03 PST
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)
Comment 6 Frédéric Wang (:fredw) 2013-12-17 02:42:56 PST
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.
Comment 7 Darin Adler 2013-12-17 07:52:18 PST
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.
Comment 8 Frédéric Wang (:fredw) 2013-12-17 08:25:21 PST
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
Comment 9 Darin Adler 2013-12-17 09:25:04 PST
(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.
Comment 10 WebKit Commit Bot 2013-12-17 09:53:06 PST
Comment on attachment 219406 [details]
Patch V1

Clearing flags on attachment: 219406

Committed r160711: <http://trac.webkit.org/changeset/160711>
Comment 11 WebKit Commit Bot 2013-12-17 09:53:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Frédéric Wang (:fredw) 2013-12-20 08:01:13 PST
(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.
Comment 13 Darin Adler 2013-12-20 09:56:59 PST
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.