Bug 81387 – MathML internals - factor code for almost anonymous blocks

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

[Dave Barton]

MathML internals - factor code for almost anonymous blocks

[Dave Barton]

Created attachment 132346 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 132346 [details]
Patch

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

Nice to clean this up, thanks.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:52
> +RenderMathMLBlock* RenderMathMLBlock::createAlmostAnonymousBlock(EDisplay display)

Normally these sorts of functions would return PassOwnPtr, but the rendering tree code isn't very consistent about that, sadly.

[Dave Barton]

Thanks for the review and the comment! I am learning. :)

[Eric Seidel (no email)]

There is some documentation on PassRefPtr/RefPtr, PassOwnPtr/OwnPtr here:
http://www.webkit.org/coding/technical-articles.html
(in the RefPtr doc), but admittedly, WebKit's documentation (or even its own ability to follow it's own best practices) is limited.

The wiki (http://trac.webkit.org/wiki) also has more information.  I'm currently working on trying to come up; with better documentation for some of the trickier parts, like the render tree.

Thanks for the patch!

[Julien Chaffraix]

Comment on attachment 132346 [details]
Patch

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

A couple of comments too (mostly consistency questions). Not worth holding the change though.

>> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:52
>> +RenderMathMLBlock* RenderMathMLBlock::createAlmostAnonymousBlock(EDisplay display)
> 
> Normally these sorts of functions would return PassOwnPtr, but the rendering tree code isn't very consistent about that, sadly.

Unfortunately indeed. Shouldn't we name this function createAlmostAnonymousMathMLBlock? (at some point you will want to create anonymous RenderMathMLBlock not RenderBlock or am I missing something?)

> Source/WebCore/rendering/mathml/RenderMathMLFraction.cpp:106
> +    Length pad(static_cast<int>(style()->fontSize() * gHorizontalPad), Fixed);

I guess this is equivalent here but it used to be row->style()->fontSize().

> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:87
> +            RefPtr<RenderStyle> scriptsStyle = RenderStyle::createAnonymousStyle(style());
>              scriptsStyle->setDisplay(INLINE_BLOCK);
>              scriptsStyle->setVerticalAlign(TOP);
>              scriptsStyle->setMarginLeft(Length(gSubsupScriptMargin, Fixed));
>              scriptsStyle->setTextAlign(LEFT);
>              // Set this wrapper's font-size for its line-height & baseline position.
>              scriptsStyle->setBlendedFontSize(static_cast<int>(0.75 * style()->fontSize()));
> -            m_scripts->setStyle(scriptsStyle.release());
> +            m_scripts = new (renderArena()) RenderMathMLBlock(node());
> +            m_scripts->setStyle(scriptsStyle);

Shouldn't m_scripts also use createAlmostAnonymousBlock() or would you rather wait to do that?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:69
> +    RenderBlock* row = createAnonymousBlock();

Shouldn't we add a new createAnonymousMathMLBlock() as you would rather keep it a MathML block if you want to add more logic into them?

[Dave Barton]

Boy you guys are good (thorough).

I'd read the technical articles, but it's good to read again. I'd also be happy to read your (Eric) new wiki documentation, and give newbie comments to you if you want.

Actually I didn't realize RenderObject isn't reference counted, so I'd originally coded using PassRefPtr<>. But none of the other functions use PassOwnPtr<> for RenderObjects, they just return a raw new pointer that ends up going to addChild(), so I did that.

Julien, soon we'll switch to an anonymous RenderBlock, I think. The best answer ultimately might be some new RenderPadded or RenderWrapped or whatever that might not even derive from RenderBlock, as we discussed. I think (though I might be proved wrong later) that these (almost) anonymous renderers that just add some padding or a vertical-align or whatever aren't specific to MathML, so shouldn't derive from RenderMathMLBlock. I only do that now to not change (almost) all the test dump tree output, to not change some broken code in RenderMathMLFraction.cpp in this patch, and so m_scripts in RenderMathMLSubSup could call this RenderMathMLBlock::createAlmostAnonymousBlock() member function (but in the end it doesn't - see below). We could make createAlmostAnonymousBlock() a member function of RenderBlock, but I didn't since this whole "almost anonymous" concept is a temporary hack just for MathML.

style()->fontSize() == row->style()->fontSize(), and the former seemed simpler and better to me with the new code.

I tried m_scripts->createAlmostAnonymousBlock() but tests failed. The problem is that the setStyle(), or some styleDidChange() or something, must happen *after* some of the style changes here (for scriptsStyle) that are more than just e.g. setting a padding. The code here seemed simplest.

Let me know if you still think I should change some of this in the next patch.

[WebKit Review Bot]

Comment on attachment 132346 [details]
Patch

Clearing flags on attachment: 132346

Committed r111083: <http://trac.webkit.org/changeset/111083>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Julien Chaffraix]

> Julien, soon we'll switch to an anonymous RenderBlock, I think. The best answer ultimately might be some new RenderPadded or RenderWrapped or whatever that might not even derive from RenderBlock, as we discussed. 

I had in mind that RenderMathMLBlock would get this padding as this was the natural continuation of the existing code and would enable most wrappers to be anonymous. Adding new renderers for some intrinsic padding is definitely cleaner though. The other comments were mostly around this line of thoughts so likely off.

> I tried m_scripts->createAlmostAnonymousBlock() but tests failed. The problem is that the setStyle(), or some styleDidChange() or something, must happen *after* some of the style changes here (for scriptsStyle) that are more than just e.g. setting a padding. The code here seemed simplest.

It's unfortunate indeed and I don't see any way around that as you need your style setup for styleDidChange.