[Webkit-unassigned] [Bug 62098] Update layout for msub, msup, msubsup to handle script changes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62098





--- Comment #12 from Alex Milowski <alex at milowski.com>  2011-06-05 10:36:17 PST ---
(In reply to comment #11)
> (From update of attachment 96050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96050&action=review
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:66
> > +    unsigned int position = 0;
> 
> unsigned int is wrong for WebKit style. We use just unsigned.

OK.  That passed the style check script.  I don't see that on the style guide.

> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:70
> > +    for (Node* current = child->node(); current; current = current->previousSibling()) {
> > +        if (current->isElementNode())
> > +            position++;
> > +    }
> 
> Looping through all the child elements is unnecessary. A cleaner way to do it would be to call previousElementSibling a small number of times.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:72
> > +    if (position == 1) {
> 
> For example, the check here could be if !previousElement.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:105
> > +            if (position == 2) 
> 
> And the check here could just be if (previousElement && !previousElement->previousElementSibling()).


Actually, no.  The number of child element is very small (2 or 3).  While someone could construct a pathological MathML element, that is probably unlikely.  

Meanwhile,  previousElementSibling() loops through the previous siblings just as I've done.  Adding two calls to previousElementSibling would require looping twice.  Also, that is only available on the Element class and that would require a static cast.

I think the code is much cleaner as it stands.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.