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

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from François Sausset <sausset at gmail.com>  2010-11-16 01:31:05 PST ---
(In reply to comment #3)
> (From update of attachment 73481 [details])
> 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.
I'll fill a separate bug.

> > WebCore/mathml/MathMLMathElement.cpp:51
> > +void MathMLMathElement::attributeChanged(Attribute* attr, bool preserveDecls)
> 
> I would write out preserveDeclarations.
Done

> > WebCore/mathml/MathMLMathElement.cpp:54
> > +        RenderMathMLBlock* block = static_cast<RenderMathMLBlock*> (renderer());
> 
> remove the space between > and (
Done

> > WebCore/mathml/MathMLStyleElement.cpp:53
> > +    if (renderer()) {
> 
> I would do
> 
> if (!renderer())
>     return;
Done

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

> > WebCore/mathml/MathMLStyleElement.cpp:55
> > +        if (attr->name() == displaystyleAttr) {
> 
> why are these attributes named like that? why not displayStyleAttr ?
I followed the style used in mathattrs.in. But I agree we could change it as you suggested to be more compliant with the general WebKit style.
I think that it should be done in a separate patch.

> > 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?
A wrong value is ignored and the default value (false) set in the renderMathBlock constructor is used.

> > 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
These values come from the w3c MathML 3 Recommendation.
Comments added and simplification of the code.

> > WebCore/mathml/RenderMathMLRoot.cpp:94
> > +            index->setScriptLevel(scriptLevel() + 2);
> 
> why +2 ? comment
This value comes from the w3c MathML 3 Recommendation.
Comment added.

> > WebCore/mathml/RenderMathMLRoot.cpp:98
> > +            // always add to the index
> 
> Comments starts with capital, ends with dot
Corrected

> > 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?
The default value (false) set in the RenderMathMLBlock constructor is used.

> > 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
Right. I factorized the code.

> > WebCore/mathml/RenderMathMLSubSup.cpp:124
> > +            // Adjust the script placement after we stretch
> 
> Add dot at the end
Done

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

> > WebCore/mathml/RenderMathMLSubSup.cpp:202
> > +//            script->setNeedsLayout(true);
> 
> Don't commit outcommented code
Corrected

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