RESOLVED FIXED156910
RenderMathMLOperator: add getBaseGlyph and avoid passing m_textContent as a parameter 
https://bugs.webkit.org/show_bug.cgi?id=156910
Summary RenderMathMLOperator: add getBaseGlyph and avoid passing m_textContent as a p...
Frédéric Wang Nélar
Reported 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.
Attachments
Patch (8.96 KB, patch)
2016-04-22 06:26 PDT, Frédéric Wang Nélar 		no flags
DetailsFormatted DiffDiff
Patch (11.10 KB, patch)
2016-04-27 05:55 PDT, Frédéric Wang Nélar 		alex: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Frédéric Wang Nélar
Comment 1 2016-04-22 06:26:41 PDT
Created attachment 277058 [details] Patch
Manuel Rego Casasnovas
Comment 2 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().
Frédéric Wang Nélar
Comment 3 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.
Frédéric Wang Nélar
Comment 4 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.
Frédéric Wang Nélar
Comment 5 2016-04-27 05:55:18 PDT
Created attachment 277469 [details] Patch Minor change: we avoid passing m_textContent to findStretchyData.
Alejandro G. Castro
Comment 6 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?
Frédéric Wang Nélar
Comment 7 2016-04-28 05:09:15 PDT
Committed r200185: <http://trac.webkit.org/changeset/200185>
WebKit Commit Bot
Comment 8 2016-04-28 05:14:21 PDT
Re-opened since this is blocked by bug 157131
Frédéric Wang Nélar
Comment 9 2016-04-28 05:20:27 PDT
Committed r200187: <http://trac.webkit.org/changeset/200187>
Note You need to log in before you can comment on or make changes to this bug.