Bug 78180 – MathML internals for bug 52444 fix - type checking, PassRefPtr

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

[Dave Barton]

MathML internals for bug 52444 fix - type checking, PassRefPtr

[Dave Barton]

Created attachment 126190 [details]
Patch

[Dave Barton]

Wait - I didn't pick up the new ChangeLog entry in my patch. Sorry, I will try again.

[Dave Barton]

Created attachment 126195 [details]
Patch

[Dave Barton]

Wait - I didn't pick up the new ChangeLog entry in my patch. Sorry, I will try again.

[Dave Barton]

The second patch (at 17:06 PST) has the ChangeLog entry and can be reviewed. Thanks for your patience. I am learning (I think). :-)

[WebKit Review Bot]

Comment on attachment 126195 [details]
Patch

Attachment 126195 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11473004

New failing tests:
platform/chromium/compositing/layout-width-change.html

[Alexey Proskuryakov]

Comment on attachment 126195 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:37
> -    RenderMathMLUnderOver(Node* expression);
> +    RenderMathMLUnderOver(Element*);

Are you sure that names like this one add no useful information?

[Dave Barton]

Comment on attachment 126195 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:37
>> +    RenderMathMLUnderOver(Element*);
> 
> Are you sure that names like this one add no useful information?

In all these RenderMathML classes, the argument to the constructor is a MathML element that corresponds to that RenderMathML class. Sometimes the current (old) code calls it "container" or "expression" or even "fraction" (including when it's not a fraction). In this case, the argument is supposed to be an <munder>, <mover>, or <munderover> element. We could call the argument something like underOver, but that name equally applies to the RenderMathMLUnderOver rendering object that's being constructed. I think in reading the code, the context implies munder/mover/munderover, and the clearest thing is to call the munder/mover/munderover element "element", and the rendering object that corresponds to it has some other name ("this" in this case). The important thing for type safety of later casts is that the argument is an Element*, not just a Node*.

I will change the argument name to "expression" if you insist, but it makes the code less clear to me. The RenderMathMLUnderOver object equally represents a mathematical expression. The difference is that one is a DOM element of type Element*, and the other in a rendering object of type RenderMathMLUnderOver*. So I really find the variable name "element" to be clearer than "expression".

[Eric Seidel (no email)]

Comment on attachment 126195 [details]
Patch

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

LGTM.  Keep them coming.  This code needs lots of love.

> Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:83
> +PassRefPtr<RenderStyle> RenderMathMLFenced::makeOperatorStyle()

This should probably be renamed createOperatorStyle() (to follow the CreateRule, which the ReftPtr (and Mac OS X style docs) talk about.  That doesn't have to be today, or in this patch.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:48
> -RenderMathMLOperator::RenderMathMLOperator(Node* container, UChar operatorChar)
> -    : RenderMathMLBlock(container)
> +RenderMathMLOperator::RenderMathMLOperator(Node* node, UChar operatorChar)

Odd that these took nodes to begin with, since it would never make sense for a Comment or Document node to be involved in a MathML rendering tree.  (Nodes can't even have style, so they couldn't be styled to look like mathtml..)

[WebKit Review Bot]

Comment on attachment 126195 [details]
Patch

Clearing flags on attachment: 126195

Committed r107263: <http://trac.webkit.org/changeset/107263>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.