WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
See
bug 160301 comment 2
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1
2016-08-01 06:19:14 PDT
Created
attachment 285008
[details]
Patch
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
Created
attachment 285085
[details]
Patch
Frédéric Wang (:fredw)
Comment 5
2016-08-02 02:44:26 PDT
Committed
r204021
: <
http://trac.webkit.org/changeset/204021
>
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.
Top of Page
Format For Printing
XML
Clone This Bug