[webkit-reviews] review granted: [Bug 88476] Inherit style changes in MathML anonymous renderers : [Attachment 147420] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Dave Barton
<dbarton at mathscribe.com>'s request for review:
Bug 88476: Inherit style changes in MathML anonymous renderers
https://bugs.webkit.org/show_bug.cgi?id=88476

Attachment 147420: Patch
https://bugs.webkit.org/attachment.cgi?id=147420&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147420&action=review


> Source/WebCore/rendering/RenderTreeAsText.cpp:383
> +	   // We want to check for CSS or intrinsic padding, so we can't just
check o.style()->hasPadding().

I think this would better read:

// We want to check the layout padding (that includes computed and intrinsic
paddings) so we can't ask the renderer's style.

Feel free to tweak it to your needs.

> Source/WebCore/rendering/RenderTreeAsText.cpp:386
> +	       LayoutUnit cssTop = box.computedCSSPaddingTop(), cssRight =
box.computedCSSPaddingRight(), cssBottom = box.computedCSSPaddingBottom(),
cssLeft = box.computedCSSPaddingLeft();

I thought it was a rule in the coding style guide to have one declaration per
line but it doesn't seem to be. For consistency, it would be better to do it
though.

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:207
> +    // We won't reach here currently.

You can enforce that with ASSERT_NOT_REACHED() and give a 'why' comment (like
'We force our display to be one of the above so this shouldn't be reached').