[webkit-reviews] review granted: [Bug 88476] Inherit style changes in MathML anonymous renderers : [Attachment 147159] 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 147159: Patch
https://bugs.webkit.org/attachment.cgi?id=147159&action=review

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


r=me, with comments.

> I don't know much about custom selectors. Why would that be better? This code
is just saying that in mathematics, underscripts and overscripts are always
centered horizontally.

Custom selectors enables you to specify a value for the object in CSS without
needing the rendering code to know about those details. This removes a lot of
burden as the CSS code already handles everything for you (including if people
want ways to override it) but it's an overkill for now. More something to
remember in the future :)

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:200
> +const char* RenderMathMLBlock::renderName() const

I would advise you to dump if the object is "anonymous". It may not seem
important but CSS is not consistent on that and it makes debugging difficult. I
am a strong advocate of having consistency on that (meaning that we should have
a core function doing the core annotation as it's too easy to miss).

> Source/WebCore/rendering/mathml/RenderMathMLBlock.cpp:203
> +    bool hasPadding = m_intrinsicPaddingBefore || m_intrinsicPaddingAfter ||
m_intrinsicPaddingStart || m_intrinsicPaddingEnd || style()->hasPadding();

Not sure how important is the padding and I will let you weight on that. If I
were in your shoes, I would not dump just "padded" as it would be better to
dump the exact padding values (not that for legacy reason table cells don't do
it...). Dumping the exact values is pretty easy and just involves hacking
RenderTreeAsText.

> Source/WebCore/rendering/mathml/RenderMathMLRow.h:38
> +    static RenderMathMLRow* createAnonymousMRowWithParentRenderer(const
RenderObject*);

The original naming matches the one in the rendering directory. It was chosen
to be generic as you call it:
RendererClass::createAnonymousWithParentRenderer() (note that you know the type
as you call it on the associated class). I would rather keep the original
naming for coherency.