Bug 155406 – MathMLMencloseElement style should not depend on rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Antti Koivisto]

Style shouldn't depend on rendering.

[Antti Koivisto]

Created attachment 273889 [details]
patch

[Frédéric Wang (:fredw)]

See also the complete refactoring in bug 155019, that will probably make this unnecessary.

[Antti Koivisto]

Yeah, looks much better. We could probably just disable mathml/presentation/menclose-notation-attribute-change-value.html until that lands.

[Frédéric Wang (:fredw)]

(In reply to comment #3)
> Yeah, looks much better. We could probably just disable
> mathml/presentation/menclose-notation-attribute-change-value.html until that
> lands.

Yes, I guess we can do it either way. Just disabling that test is probably the easiest way so that it does not block the work on bug 155298.

[Darin Adler]

Comment on attachment 273889 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273889&action=review

> Source/WebCore/mathml/MathMLMencloseElement.cpp:134
>      String closingBrace(")", String::ConstructFromLiteral);
>      TextRun run(closingBrace);

We do not need a String to construct a TextRun, which takes a StringView. We can write something more like this:

    LChar rightParenthesis = ')';
    TextRun run({ &rightParenthesis, 1 });

I took the liberty of calling this what the Unicode standard calls it, rather than calling it "closing brace", which seems like it could be the name of some other characters.

> Source/WebCore/mathml/MathMLMencloseElement.cpp:136
> +    const FontCascade& font = parentStyle.fontCascade();
> +    padding.appendNumber(font.width(run));

I don’t think we need a local variable when we are just using it once. In fact, I would write this:

    LChar rightParenthesis = ')';
    padding.appendNumber(parentStyle.fontCascade().width(TextRun{ &rightParenthesis, 1 }));

Might need an extra type name or set of curly brackets in there, but something like that would work. The version with more lines of code seems no more clear to me.

> Source/WebCore/mathml/MathMLMencloseElement.cpp:143
> +String MathMLMencloseElement::longDivLeftPadding() const

This could return a const String& instead of a String to avoid unneeded reference count churn.

[Darin Adler]

Oh, I see, this code is going away. Oops, my comments are overkill.

[Frédéric Wang (:fredw)]

Resolving since this was fixed by https://trac.webkit.org/changeset/199980