Bug 79063 – MathML internals - code clean-up for RenderMathMLSubSup

• Component: MathML
• Status: RESOLVED FIXED

[Dave Barton]

MathML internals - code clean-up for RenderMathMLSubSup

[Dave Barton]

Created attachment 127890 [details]
Patch

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

Mostly we need to explain why deleting the update line is OK.

[Dave Barton]

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.

[Eric Seidel (no email)]

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.

[WebKit Review Bot]

Comment on attachment 127890 [details]
Patch

Clearing flags on attachment: 127890

Committed r108302: <http://trac.webkit.org/changeset/108302>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.