Bug 49309 (scriptlevel) - [MathML] Scriptlevel attribute implementation
[MathML] Scriptlevel attribute implementation
 Status: UNCONFIRMED scriptlevel WebKit Unclassified MathML (show other bugs) 528+ (Nightly build) Macintosh OS X 10.6 P2 Normal Nobody 3251 mathml-in-mathjax 118738 Show dependency tree / graph

 Reported: 2010-11-10 01:12 PST by François Sausset 2019-05-10 04:36 PDT (History) 9 users (show) alex allan.jensen bfulgham donggwan.kim fred.wang mmaxfield rwlbuis syoichi webmaster

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.
 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. François Sausset 2010-11-10 01:25:46 PST Created attachment 73470 [details] Patch 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. 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 (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 (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 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 (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 (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 François Sausset 2010-11-16 01:32:07 PST Created attachment 73973 [details] Revised patch Eric Seidel (no email) 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. 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. Frédéric Wang (:fredw) 2019-05-10 04:36:44 PDT (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