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

[bugzilla-daemon at webkit.org]

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





--- Comment #21 from chris fleizach <cfleizach at apple.com>  2013-08-21 16:40:45 PST ---
(From update of attachment 208636)
View in context: https://bugs.webkit.org/attachment.cgi?id=208636&action=review

starting to look at this. it will take me a bit of time to make it through everything. thanks

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3460
> +    return (toRenderMathMLBlock(m_renderer)->isRenderMathMLScripts() && !isMathMultiscript());

parentheses not needed here

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:48
> +// BaseWrapper SubSupPairWrapper* (mprescripts SubSupPairWrapper*)*

i'm not sure which render tree structure you're saying you're going to use here. Are you using the above version for valid elements, or the below version for valid and invalid elements?

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:54
> +bool isMPrescripts(RenderObject *renderObject)

isMathPrescript is a better name, or just isPrescript. 
static should be on the same line as bool ...
* is in wrong place

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:96
> +    scriptsStyle->setJustifyContent(m_kind == Sub ? JustifyFlexStart : m_kind == Super ? JustifyFlexEnd : JustifySpaceBetween);

i don't see anything explicit about non-wrapped comments but these will be easer to read if the comments wrap after a reasonable number of characters

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:122
> +        for (subSupPair = subSupPair->nextSibling(); subSupPair && !isMPrescripts(subSupPair); subSupPair = subSupPair->nextSibling())

didn't you already hit the prescript tag in the first loop? is there a reason to check for it again?

> Source/WebCore/rendering/mathml/RenderMathMLScripts.cpp:140
> +void RenderMathMLScripts::addChildInternal(bool normalInsertion, RenderObject* child, RenderObject* beforeChild)

i'm not clear what "normal" means in this context. it sounds a bit subjective. can you clarify when that flag is to be used

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:58
> +    virtual bool isRenderMathMLScriptsWrapper() const { return true; }

OVERRIDE FINAL

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:110
> +    virtual bool isRenderMathMLScripts() const { return true; }

OVERRIDE

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