Bug 160400

Summary: Use Optional members to store parsed MathML attributes
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dbarton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
darin: review+
Patch
none
Patch none

Description Frédéric Wang (:fredw) 2016-08-01 05:32:24 PDT
See bug 160301 comment 2
Comment 1 Frédéric Wang (:fredw) 2016-08-01 06:19:14 PDT
Created attachment 285008 [details]
Patch
Comment 2 Darin Adler 2016-08-01 10:43:00 PDT
Comment on attachment 285008 [details]
Patch

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

I think this is an improvement.

Unfortunately it’s not quite as good for the tri-state booleans where we need to be able to specify "not computed", "unspecified", true, and false. It’s annoying to have to convert Optional<MathMLElement::BooleanValue> to Optional<bool> and have the missing value have two different meanings!

Also, in the older WebKit code we have a type called TriState that we use for the same purpose as MathMLElement::BooleanValue. I think in new code I would actually use Optional<bool>, but I don’t think that Optional<Optional<bool>> is a good choice!

> Source/WebCore/mathml/MathMLElement.cpp:592
> +    return specifiedDisplayStyle == BooleanValue::Default ? Optional<bool>() : (specifiedDisplayStyle == BooleanValue::True);

Seems like we should have a helper function that will convert a MathMLElement::BooleanValue to Optional<bool> rather than writing it out like this.

> Source/WebCore/mathml/MathMLElement.h:97
>      Optional<MathMLElement::MathVariant> specifiedMathVariant();

This MathMLElement:: prefix here is not needed, I think.

> Source/WebCore/mathml/MathMLFractionElement.cpp:56
>      String thickness = attributeWithoutSynchronization(linethicknessAttr);

This would be more efficient if the type was const AtomicString& instead of String; could be written auto& and that should work.

> Source/WebCore/mathml/MathMLFractionElement.cpp:59
> +        m_lineThickness.value().type = LengthType::UnitLess;
> +        m_lineThickness.value().value = .5;

Could have written this instead:

    m_lineThickness.value() = { LengthType::UnitLess, .5 };

> Source/WebCore/mathml/MathMLFractionElement.cpp:76
>      String value = attributeWithoutSynchronization(name);

This would be more efficient if the type was const AtomicString& instead of String; could be written auto& and that should work.

> Source/WebCore/mathml/MathMLFractionElement.cpp:99
> +        m_lineThickness = Optional<Length>();

Could use Nullopt here instead of this. Or { }. I like those better than the one where we explicitly cite the type.

    m_lineThickness = Nullopt;

Or:

    m_lineThickness = { };

> Source/WebCore/mathml/MathMLFractionElement.cpp:101
> +        m_numeratorAlignment = Optional<FractionAlignment>();

Ditto.

> Source/WebCore/mathml/MathMLFractionElement.cpp:103
> +        m_denominatorAlignment = Optional<FractionAlignment>();

Ditto.

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:93
> +        m_displayStyle = Optional<BooleanValue>();

Ditto.

> Source/WebCore/mathml/MathMLInlineContainerElement.cpp:95
> +        m_mathVariant = Optional<MathVariant>();

Ditto.

> Source/WebCore/mathml/MathMLMathElement.cpp:75
> +        m_displayStyle = Optional<BooleanValue>();

Ditto.

> Source/WebCore/mathml/MathMLMathElement.cpp:77
> +        m_mathVariant = Optional<MathVariant>();

Ditto.

> Source/WebCore/mathml/MathMLPaddedElement.cpp:76
> +        m_width = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLPaddedElement.cpp:78
> +        m_height = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLPaddedElement.cpp:80
> +        m_depth = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLPaddedElement.cpp:82
> +        m_lspace = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLPaddedElement.cpp:84
> +        m_voffset = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLScriptsElement.cpp:61
> +        m_subscriptShift = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLScriptsElement.cpp:63
> +        m_superscriptShift = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLSpaceElement.cpp:66
> +        m_width = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLSpaceElement.cpp:68
> +        m_height = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLSpaceElement.cpp:70
> +        m_depth = Optional<Length>();

Ditto.

> Source/WebCore/mathml/MathMLTextElement.cpp:68
> +        m_mathVariant = Optional<MathVariant>();

Ditto.

> Source/WebCore/mathml/MathMLUnderOverElement.cpp:61
> +        m_accent = Optional<BooleanValue>();

Ditto.

> Source/WebCore/mathml/MathMLUnderOverElement.cpp:63
> +        m_accentUnder = Optional<BooleanValue>();

Ditto.
Comment 3 Frédéric Wang (:fredw) 2016-08-02 01:26:29 PDT
Created attachment 285084 [details]
Patch

(In reply to comment #2)
> > Source/WebCore/mathml/MathMLFractionElement.cpp:59
> > +        m_lineThickness.value().type = LengthType::UnitLess;
> > +        m_lineThickness.value().value = .5;
> 
> Could have written this instead:

Let's see if all bots are happy with the compact syntax.
Comment 4 Frédéric Wang (:fredw) 2016-08-02 01:41:28 PDT
Created attachment 285085 [details]
Patch
Comment 5 Frédéric Wang (:fredw) 2016-08-02 02:44:26 PDT
Committed r204021: <http://trac.webkit.org/changeset/204021>
Comment 6 Frédéric Wang (:fredw) 2016-08-02 02:48:42 PDT
(In reply to comment #2)
> Unfortunately it’s not quite as good for the tri-state booleans where we
> need to be able to specify "not computed", "unspecified", true, and false.
> It’s annoying to have to convert Optional<MathMLElement::BooleanValue> to
> Optional<bool> and have the missing value have two different meanings!
> 
> Also, in the older WebKit code we have a type called TriState that we use
> for the same purpose as MathMLElement::BooleanValue. I think in new code I
> would actually use Optional<bool>, but I don’t think that
> Optional<Optional<bool>> is a good choice!

Maybe all the default boolean values could be resolved on the DOM side and in that case the Default value won't be needed anymore (that's the case for the boolean operator properties in bug 160190). However, I'm not sure it's easy as some MathML rules are a bit complex. So let's keep it like this for now.