[Webkit-unassigned] [Bug 99618] [MathML] Implement the mmultiscripts tag

[bugzilla-daemon at webkit.org]

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





--- Comment #33 from chris fleizach <cfleizach at apple.com>  2013-09-13 10:47:14 PST ---
(From update of attachment 210184)
View in context: https://bugs.webkit.org/attachment.cgi?id=210184&action=review

a few comments inline. 
once you fix the failing layout test i think we should move forward

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:298
> +    for (bool isPostScript = true; subSupPair; subSupPair = toRenderMathMLBlock(subSupPair->nextSibling())) {

we should probably initialize isPostScript outside of the for loop. It's a bit crowded to stick that initialization in there too

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:333
> +    newPadding = Length(topPadding, Fixed);

seems like you can define and declare at the same time
Length newPadding(topPadding, Fixed)

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:364
> +RenderMathMLScriptsWrapper* RenderMathMLScriptsWrapper::createAnonymousWrapper(RenderMathMLScripts* renderObject, WrapperType type)

can we make this a PassRefPtr?

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:423
> +            destroy();

is this going to destroy() this object?

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:440
> +    for (RenderObject* previousSibling = subSupPair->previousSibling(); subSupPair != this; subSupPair = toRenderMathMLScriptsWrapper(previousSibling), previousSibling = previousSibling->previousSibling()) {

i would move this to the end of the loop
subSupPair = toRenderMathMLScriptsWrapper(previousSibling)
because it's not directly related to iterated the for loop, and this for () is a bit crowded

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:486
> +    for (RenderObject* nextSibling = subSupPair->nextSibling(); nextSibling && !isPrescript(nextSibling); subSupPair = toRenderMathMLScriptsWrapper(nextSibling), nextSibling = nextSibling->nextSibling()) {

ditto about for loop

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:45
> +    RenderMathMLScriptsWrapper(Element* element, WrapperType kind) : RenderMathMLBlock(element), m_kind(kind) { };

i don't know about style guidelines for this, but i would bet we should put the code on different lines

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:48
> +    virtual void removeChild(RenderObject* child) OVERRIDE;

child parameter name not necessary

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:98
> +    virtual void removeChild(RenderObject* child) OVERRIDE;

child param name not necessary

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