Bug 49309 (scriptlevel) - [MathML] Scriptlevel attribute implementation
Summary: [MathML] Scriptlevel attribute implementation
Status: UNCONFIRMED
Alias: scriptlevel
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 3251 mathml-in-mathjax 118738
  Show dependency treegraph
 
Reported: 2010-11-10 01:12 PST by François Sausset
Modified: 2016-07-07 09:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (976.78 KB, patch)
2010-11-10 01:25 PST, François Sausset
no flags Details | Formatted Diff | Diff
Regenerated patch (976.76 KB, patch)
2010-11-10 03:47 PST, François Sausset
no flags Details | Formatted Diff | Diff
Revised patch (976.77 KB, patch)
2010-11-16 01:32 PST, François Sausset
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description François Sausset 2010-11-10 01:12:11 PST
Scriptlevel attribute should be implemented. Other related attributes like: displaystyle, scriptminsize, scriptsizemultiplier, etc should be implemented at the same time.
It needs to refactor token element renderers.
Others adjustments in msup, msub msubsup, mfrac, mroot, etc are also needed.
Comment 1 François Sausset 2010-11-10 01:25:46 PST
Created attachment 73470 [details]
Patch
Comment 2 François Sausset 2010-11-10 03:47:15 PST
Created attachment 73481 [details]
Regenerated patch

The webcore xcodeproj file was updated just after the patch creation and before the patch submission.
This new patch should be OK.
Comment 3 Kenneth Rohde Christiansen 2010-11-12 05:18:29 PST
Comment on attachment 73481 [details]
Regenerated patch

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
Comment 4 François Sausset 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
Comment 5 François Sausset 2010-11-16 01:32:07 PST
Created attachment 73973 [details]
Revised patch
Comment 6 Eric Seidel 2010-12-09 23:45:44 PST
Comment on attachment 73973 [details]
Revised patch

There is a huge psychological barrier (if nothing else) to reviewing a 1MB patch.  In this case, it might be better to separate out the test changes.  The code changes look rather large too though, so this may just need to be broken down in general.
Comment 7 Brent Fulgham 2013-11-26 12:14:13 PST
This patch is very old. Is it still relevant? At the very least, it should be rebased so that it can apply against current sources.