Bug 160400 - Use Optional members to store parsed MathML attributes
Summary: Use Optional members to store parsed MathML attributes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-01 05:32 PDT by Frédéric Wang (:fredw)
Modified: 2016-08-02 04:29 PDT (History)
3 users (show)

See Also:


Attachments
Patch (23.03 KB, patch)
2016-08-01 06:19 PDT, Frédéric Wang (:fredw)
darin: review+
Details | Formatted Diff | Diff
Patch (23.83 KB, patch)
2016-08-02 01:26 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (23.76 KB, patch)
2016-08-02 01:41 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.