MathML internals - code clean-up for RenderMathMLSubSup
Created attachment 127890 [details] Patch
Comment on attachment 127890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127890&action=review I think this is OK, but could be made a bit more clear. Are you willing to go another round of cleanup before moving foward? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:-149 > - if (top->isBoxModelObject()) { > - RenderBoxModelObject* topBox = toRenderBoxModelObject(top); > - topBox->updateBoxModelInfoFromStyle(); > - } We no longer need this update? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:151 > + if (m_kind == SubSup && m_scripts) { It would be better to early return so we didn't have to indent the whole rest of this function? > Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:153 > + LayoutUnit heightDiff = (m_scripts->offsetHeight() - base->offsetHeight()) / 2; heightDiff sounds like a strange name for this half-height diff?
Mostly we need to explain why deleting the update line is OK.
Comment on attachment 127890 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127890&action=review I appreciate your review & judgment and will do any changes you still want. In the next patch though this stretchToHeight() formatting code & the code in layout() will be combined and revised. >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:-149 >> - } > > We no longer need this update? There's an old message from David Hyatt on webkit-dev saying that updateBoxModelInfoFromStyle() is an internal method to styleWillChange()/styleDidChange() and should be considered private. The supWrapper->setNeedsLayout(true) on new line 141 will cause these margins to be used, right? >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:151 >> + if (m_kind == SubSup && m_scripts) { > > It would be better to early return so we didn't have to indent the whole rest of this function? I didn't want to rewrite too much. Also we could change someday to doing our own layout() of msub or msup also, instead of using CSS vertical-align sub/super. That way we could get the baselines of msub or msup to be the same as the baselines of subscripts & superscripts in <msubsup>s, maybe. >> Source/WebCore/rendering/mathml/RenderMathMLSubSup.cpp:153 >> + LayoutUnit heightDiff = (m_scripts->offsetHeight() - base->offsetHeight()) / 2; > > heightDiff sounds like a strange name for this half-height diff? This variable is going away in the next patch. I just left the name as is for now.
Comment on attachment 127890 [details] Patch I'm very happy to approve this as a first-iteration patch. Small patches are always the best way to work, and I certainly wouldn't want to discourage you from such! Thank you for your responses and I look forward to the next patch.
Comment on attachment 127890 [details] Patch Clearing flags on attachment: 127890 Committed r108302: <http://trac.webkit.org/changeset/108302>
All reviewed patches have been landed. Closing bug.