Bug 78180

Created attachment 126190 [details] Patch

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

Created attachment 126195 [details] Patch

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

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

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

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?

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

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

Comment on attachment 126195 [details] Patch Clearing flags on attachment: 126195 Committed r107263: <http://trac.webkit.org/changeset/107263>

All reviewed patches have been landed. Closing bug.