Bug 124841

Created attachment 217807 [details] WIP Patch

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 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.

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?

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)

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 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 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

(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 on attachment 219406 [details] Patch V1 Clearing flags on attachment: 219406 Committed r160711: <http://trac.webkit.org/changeset/160711>

All reviewed patches have been landed. Closing bug.

(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 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.