Bug 49309 (scriptlevel)

Created attachment 73470 [details] Patch

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 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

(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

Created attachment 73973 [details] Revised patch

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.

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.

(In reply to Brent Fulgham from comment #7) > 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. Probably the way forward will be to implement math-script-level/math-style and just map scriptlevel/displaystyle to these CSS properties. See https://github.com/w3c/csswg-drafts/issues/3746

*** Bug 118738 has been marked as a duplicate of this bug. ***

MathML Core describes scriptlevel here, it's based on math-depth and interacts with the math-style attribute implemented in bug 216702: https://w3c.github.io/mathml-core/#the-displaystyle-and-scriptlevel-attributes https://w3c.github.io/mathml-core/#the-math-script-level-property

Pull request: https://github.com/WebKit/WebKit/pull/53028