RESOLVED FIXED 160400
Use Optional members to store parsed MathML attributes
https://bugs.webkit.org/show_bug.cgi?id=160400
Summary Use Optional members to store parsed MathML attributes
Frédéric Wang (:fredw)
Reported 2016-08-01 05:32:24 PDT
Attachments
Patch (23.03 KB, patch)
2016-08-01 06:19 PDT, Frédéric Wang (:fredw)
darin: review+
Patch (23.83 KB, patch)
2016-08-02 01:26 PDT, Frédéric Wang (:fredw)
no flags
Patch (23.76 KB, patch)
2016-08-02 01:41 PDT, Frédéric Wang (:fredw)
no flags
Frédéric Wang (:fredw)
Comment 1 2016-08-01 06:19:14 PDT
Darin Adler
Comment 2 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.
Frédéric Wang (:fredw)
Comment 3 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.
Frédéric Wang (:fredw)
Comment 4 2016-08-02 01:41:28 PDT
Frédéric Wang (:fredw)
Comment 5 2016-08-02 02:44:26 PDT
Frédéric Wang (:fredw)
Comment 6 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.
Note You need to log in before you can comment on or make changes to this bug.