Bug 160400

Created attachment 285008 [details] Patch

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.

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.

Created attachment 285085 [details] Patch

Committed r204021: <http://trac.webkit.org/changeset/204021>

(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.