Bug 161084

Summary: MathML renderers should use struct to pass long list of LayoutUnits
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: commit-queue, dbarton, esprehn+autocc, glenn, kondapallykalyan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch darin: review+

Description Frédéric Wang (:fredw) 2016-08-23 09:04:08 PDT
This was suggested by Darin in a previous bug.
Comment 1 Frédéric Wang (:fredw) 2016-08-23 09:09:28 PDT
Created attachment 286718 [details]
Patch
Comment 2 Darin Adler 2016-08-27 17:36:02 PDT
Comment on attachment 286718 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLFraction.h:69
> +    void getFractionParameters(FractionParameters&);

Given modern C++ and return value optimizations, you should consider just having this be:

    FractionParameters fractionParameters() const;

No reason it needs to be a get function; move is quite efficient.

> Source/WebCore/rendering/mathml/RenderMathMLFraction.h:75
> +    void getStackParameters(StackParameters&);

Ditto.

> Source/WebCore/rendering/mathml/RenderMathMLScripts.h:65
> +    void getScriptMetricsAndLayoutIfNeeded(RenderBox* base, RenderBox* script, ScriptMetrics&);

Ditto. Return value instead of get style. Also, can these arguments be references instead of pointers?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:69
> +    void getVerticalParameters(VerticalParameters&) const;

Same here.
Comment 3 Frédéric Wang (:fredw) 2016-08-28 10:38:00 PDT
Committed r205105: <http://trac.webkit.org/changeset/205105>
Comment 4 Frédéric Wang (:fredw) 2016-08-28 10:42:13 PDT
(In reply to comment #2)
> > Source/WebCore/rendering/mathml/RenderMathMLScripts.h:65
> > +    void getScriptMetricsAndLayoutIfNeeded(RenderBox* base, RenderBox* script, ScriptMetrics&);
> 
> Ditto. Return value instead of get style. Also, can these arguments be
> references instead of pointers?

That one is a bit complicated so I'll handle it in follow-up patches. Basically getScriptMetricsAndLayoutIfNeeded does three things: it retrieves MATH parameters, it calls layoutIfNeed on all the scripts and finally it determines the minimal shift/size of each script & take the maximum of the values (they are two separate calls for the pre and post script lists). I think this should be reorganized a bit (splitting the three parts and make the maximum of pre/post more explicit).