Bug 156795 – Move parsing of mspace attributes to a MathMLSpaceElement class

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

Created attachment 276830 [details]
WIP Patch

This is bug 156536 for the <mspace/> element.

[Frédéric Wang (:fredw)]

Comment on attachment 276830 [details]
WIP Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:66
> +    spaceElement.depthLength().toUserUnits(style(), m_depth);

Note: my plan (not tested yet) was to directly introduce helper function mspaceWidth() mspaceHeight() and mspaceDepth() that would directly call toUserUnits. That way we can just remove m_width, m_height and m_depth members.

[Frédéric Wang (:fredw)]

Created attachment 279108 [details]
WIP Patch

[Frédéric Wang (:fredw)]

Created attachment 279113 [details]
Patch for EWS testing

[Frédéric Wang (:fredw)]

Created attachment 279114 [details]
Patch for EWS

[Frédéric Wang (:fredw)]

Created attachment 279115 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 279241 [details]
Patch

[Brent Fulgham]

Comment on attachment 279241 [details]
Patch

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

This looks good except: (1) I think we could reduce some #include churn if you moved those implementations into the CPP file, and (2) this doesn't apply to the tree at present.

Could you please revise and I'll approve it?

> Source/WebCore/mathml/MathMLElement.h:66
> +        bool dirty = true;

These should be in our standard C++11 style:
LengthType type { LengthType::ParsingFailed; }
float value { 0 };
bool dirty { true };

> Source/WebCore/mathml/MathMLSpaceElement.h:40
> +    const Length& depth() { return cachedMathMLLength(MathMLNames::depthAttr, m_depth); }

If you moved these into the implementation file, you could probably avoid #including MathMLNames.h in the header file. This would be beneficial to build time.

> Source/WebCore/rendering/mathml/RenderMathMLSpace.cpp:-78
> -    setNeedsLayoutAndPrefWidthsRecalc();

Yay!

[Frédéric Wang (:fredw)]

Created attachment 283153 [details]
Patch

Review comments addressed, but this is still blocked by bug 156792.

[Frédéric Wang (:fredw)]

Created attachment 283308 [details]
Patch bug 156792 + 156795 (for EWS testing)

[Brent Fulgham]

Comment on attachment 283153 [details]
Patch

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

Looks good. r=me after you land the patch this depends on.

> Source/WebCore/mathml/MathMLSpaceElement.cpp:39
> +    : MathMLElement(tagName, document)

It's a little unusual for us to inline constructors like this. Did you measure this as a bottleneck of some kind?

[Frédéric Wang (:fredw)]

(In reply to comment #10)
> Comment on attachment 283153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283153&action=review
> 
> Looks good. r=me after you land the patch this depends on.
> 
> > Source/WebCore/mathml/MathMLSpaceElement.cpp:39
> > +    : MathMLElement(tagName, document)
> 
> It's a little unusual for us to inline constructors like this. Did you
> measure this as a bottleneck of some kind?

I don't think I did that on purpose, I'll remove the inline keyword (it seems that I copied it from MathMLMathElement which has had it for a long time).

[Frédéric Wang (:fredw)]

Created attachment 283387 [details]
Patch

[Frédéric Wang (:fredw)]

Committed r203108: <http://trac.webkit.org/changeset/203108>