[Webkit-unassigned] [Bug 49309] [MathML] Scriptlevel attribute implementation

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2010-11-12 05:18:29 PST ---
(From update of attachment 73481)
View in context: https://bugs.webkit.org/attachment.cgi?id=73481&action=review

> WebCore/mathml/MathMLInlineContainerElement.cpp:59
> -    if (hasLocalName(MathMLNames::mrowTag))
> +    if (hasLocalName(mrowTag))

This seems like a separate change which could be in its own patch, to make it more clear what was changed.

> WebCore/mathml/MathMLMathElement.cpp:51
> +void MathMLMathElement::attributeChanged(Attribute* attr, bool preserveDecls)

I would write out preserveDeclarations.

> WebCore/mathml/MathMLMathElement.cpp:54
> +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());

remove the space between > and (

> WebCore/mathml/MathMLStyleElement.cpp:53
> +    if (renderer()) {

I would do

if (!renderer())
    return;

It makes it more clear, it is common webkit style and the code doesnt get unnecessarily indented

> WebCore/mathml/MathMLStyleElement.cpp:54
> +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());

remove space between > and (

> WebCore/mathml/MathMLStyleElement.cpp:55
> +        if (attr->name() == displaystyleAttr) {

why are these attributes named like that? why not displayStyleAttr ?

> WebCore/mathml/MathMLStyleElement.cpp:59
> +            if (attr->value() == "true")
> +                block->setDisplayStyle(true);
> +            else if (attr->value() == "false" || attr->value().isEmpty())
> +                block->setDisplayStyle(false);

What is someone set something wrong here? Shouldn't that be handled?

> WebCore/mathml/MathMLStyleElement.cpp:64
> +                block->setScriptSizeMultiplier(0.71);

Where does 0.71 come from? comment?

> WebCore/mathml/MathMLStyleElement.cpp:70
> +                block->setScriptMinSize(newLengthArray("8pt", size)[0].value());

8pt? I don't like hardcoding these things

> WebCore/mathml/RenderMathMLBlock.cpp:47
> +    , m_scriptSizeMultiplier(0.71)

You seem to be using this 0.71 in more places.

> WebCore/mathml/RenderMathMLBlock.cpp:50
> +    m_scriptMinSize = newLengthArray("8pt", size)[0].value();

And the 8pt as well

> WebCore/mathml/RenderMathMLRoot.cpp:94
> +            index->setScriptLevel(scriptLevel() + 2);

why +2 ? comment

> WebCore/mathml/RenderMathMLRoot.cpp:98
> +            // always add to the index

Comments starts with capital, ends with dot

> WebCore/mathml/RenderMathMLStyle.cpp:53
> +        String displayStyle = style->getAttribute(displaystyleAttr);
> +        if (equalIgnoringCase(displayStyle, "true"))
> +            setDisplayStyle(true);
> +        else if (equalIgnoringCase(displayStyle, "false"))
> +            setDisplayStyle(false);
> +        

What if it is neither true or false?

> WebCore/mathml/RenderMathMLStyle.cpp:73
> +        if (!level.isEmpty()) {
> +            if (level.startsWith("+")) {
> +                level.remove(0);
> +                setScriptLevel(scriptLevel() + level.toInt());
> +            } else if (level.startsWith("-")) {
> +                level.remove(0);
> +                setScriptLevel(scriptLevel() - level.toInt());
> +            } else
> +            setScriptLevel(level.toInt());
> +        }

I tihnk I saw very similar code elsewhere in the patch

> WebCore/mathml/RenderMathMLSubSup.cpp:124
> +            // Adjust the script placement after we stretch

Add dot at the end

> WebCore/mathml/RenderMathMLSubSup.cpp:163
> +    RenderObject* base = firstChild();
> +    if (base) {

Why not 

if (RenderObject* base = firstChild()) {

> WebCore/mathml/RenderMathMLSubSup.cpp:202
> +//            script->setNeedsLayout(true);

Don't commit outcommented code

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