Bug 156910 - RenderMathMLOperator: add getBaseGlyph and avoid passing m_textContent as a parameter
Summary: RenderMathMLOperator: add getBaseGlyph and avoid passing m_textContent as a p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL:
Keywords:
Depends on: 157131
Blocks: 152244 156913 156921 156950 157071
  Show dependency treegraph
 
Reported: 2016-04-22 06:18 PDT by Frédéric Wang (:fredw)
Modified: 2016-04-28 05:20 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.96 KB, patch)
2016-04-22 06:26 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (11.10 KB, patch)
2016-04-27 05:55 PDT, Frédéric Wang (:fredw)
alex: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2016-04-22 06:18:59 PDT
Splitting another first step from bug 152244.

The RenderStyle parameter is not really needed yet until we move getBaseGlyph code, but I'm adding it for consistency with the MathOperator class.
Comment 1 Frédéric Wang (:fredw) 2016-04-22 06:26:41 PDT
Created attachment 277058 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2016-04-22 06:45:09 PDT
Comment on attachment 277058 [details]
Patch

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

LGTM. Nice refactoring.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:146
> +    StretchyData getDisplayStyleLargeOperator() const;

Maybe we can take advantage to remove the "get" prefix and rename this to displayStyleLargeOperator().
Comment 3 Frédéric Wang (:fredw) 2016-04-22 06:51:23 PDT
Comment on attachment 277058 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:146
>> +    StretchyData getDisplayStyleLargeOperator() const;
> 
> Maybe we can take advantage to remove the "get" prefix and rename this to displayStyleLargeOperator().

When this is moved to the MathOperator class it does not return a value but just finds such a large operator and set the MathOperator members immediately. And it actually get renamed findDisplayStyleLargeOperator. I plan to do this step in a next patch, so I don't think remove the get* prefix for now.
Comment 4 Frédéric Wang (:fredw) 2016-04-25 00:49:10 PDT
Comment on attachment 277058 [details]
Patch

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

>>> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:146
>>> +    StretchyData getDisplayStyleLargeOperator() const;
>> 
>> Maybe we can take advantage to remove the "get" prefix and rename this to displayStyleLargeOperator().
> 
> When this is moved to the MathOperator class it does not return a value but just finds such a large operator and set the MathOperator members immediately. And it actually get renamed findDisplayStyleLargeOperator. I plan to do this step in a next patch, so I don't think remove the get* prefix for now.

The rename to findDisplayStyleLargeOperator and removal of return value is done on the patch for bug 156913.
Comment 5 Frédéric Wang (:fredw) 2016-04-27 05:55:18 PDT
Created attachment 277469 [details]
Patch

Minor change: we avoid passing m_textContent to findStretchyData.
Comment 6 Alejandro G. Castro 2016-04-28 04:22:11 PDT
Comment on attachment 277469 [details]
Patch

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

LGTM

> Source/WebCore/ChangeLog:12
> +        (WebCore::RenderMathMLOperator::italicCorrection): We do not need to pass m_textContent to getDisplayStyleLargeOperator.
> +        (WebCore::RenderMathMLOperator::computePreferredLogicalWidths): We use getBaseGlyph and do not pass m_textContent to getDisplayStyleLargeOperator or findStretchyData.

Are these lines wrapped?
Comment 7 Frédéric Wang (:fredw) 2016-04-28 05:09:15 PDT
Committed r200185: <http://trac.webkit.org/changeset/200185>
Comment 8 WebKit Commit Bot 2016-04-28 05:14:21 PDT
Re-opened since this is blocked by bug 157131
Comment 9 Frédéric Wang (:fredw) 2016-04-28 05:20:27 PDT
Committed r200187: <http://trac.webkit.org/changeset/200187>