Bug 156795

Summary: Move parsing of mspace attributes to a MathMLSpaceElement class
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, darin
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 156792    
Bug Blocks: 156536    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
Patch for EWS testing
none
Patch for EWS
none
Patch
none
Patch
bfulgham: review-
Patch
bfulgham: review+
Patch bug 156792 + 156795 (for EWS testing)
none
Patch none

Description Frédéric Wang (:fredw) 2016-04-20 09:39:04 PDT
Created attachment 276830 [details]
WIP Patch

This is bug 156536 for the <mspace/> element.
Comment 1 Frédéric Wang (:fredw) 2016-04-23 02:14:58 PDT
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.
Comment 2 Frédéric Wang (:fredw) 2016-05-17 03:16:00 PDT
Created attachment 279108 [details]
WIP Patch
Comment 3 Frédéric Wang (:fredw) 2016-05-17 05:30:35 PDT
Created attachment 279113 [details]
Patch for EWS testing
Comment 4 Frédéric Wang (:fredw) 2016-05-17 05:32:07 PDT
Created attachment 279114 [details]
Patch for EWS
Comment 5 Frédéric Wang (:fredw) 2016-05-17 06:06:02 PDT
Created attachment 279115 [details]
Patch
Comment 6 Frédéric Wang (:fredw) 2016-05-18 06:33:13 PDT
Created attachment 279241 [details]
Patch
Comment 7 Brent Fulgham 2016-07-07 12:20:57 PDT
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!
Comment 8 Frédéric Wang (:fredw) 2016-07-08 07:04:21 PDT
Created attachment 283153 [details]
Patch

Review comments addressed, but this is still blocked by bug 156792.
Comment 9 Frédéric Wang (:fredw) 2016-07-11 02:25:23 PDT
Created attachment 283308 [details]
Patch bug 156792 + 156795 (for EWS testing)
Comment 10 Brent Fulgham 2016-07-11 13:48:13 PDT
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?
Comment 11 Frédéric Wang (:fredw) 2016-07-11 21:55:14 PDT
(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).
Comment 12 Frédéric Wang (:fredw) 2016-07-11 21:57:01 PDT
Created attachment 283387 [details]
Patch
Comment 13 Frédéric Wang (:fredw) 2016-07-11 23:00:46 PDT
Committed r203108: <http://trac.webkit.org/changeset/203108>