Bug 180923 – Add comments and improve code styles for RenderMathMLUnderOver::stretchHorizontalOperatorsAndLayoutChildren() and related functions

• Component: MathML
• Status: RESOLVED FIXED

[Minsheng Liu]

See more at the end of https://bugs.webkit.org/show_bug.cgi?id=179682

[Minsheng Liu]

Created attachment 329648 [details]
Patch

[Darin Adler]

Comment on attachment 329648 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:53
> +static RenderMathMLOperator* toHorizontalStretchyOperator(RenderBox& box)

I don’t think this function needs "to" in its name.

[Minsheng Liu]

Created attachment 329734 [details]
Patch

[Minsheng Liu]

Created attachment 329737 [details]
Patch

[Minsheng Liu]

I have removed "to" in the function's name as suggested by Darin.

Also, @zalan pointed out in 179682 that unembellishedOperator() should be const. I forgot to deal with that in my last patch but handled it in the new one. The only tricky part is that I had to use a const_cast because for RenderMathMLOperator, unembellishedOperator() needs to return *this, which is a pointer to a const object. I think this usage aligns with how we deal with other getters such as firstChild(), but I am not an expert on C++ so I need some professional advice on this.

[Minsheng Liu]

(In reply to Minsheng Liu from comment #5)
> I have removed "to" in the function's name as suggested by Darin.
> 
> Also, @zalan pointed out in 179682 that unembellishedOperator() should be
> const. I forgot to deal with that in my last patch but handled it in the new
> one. The only tricky part is that I had to use a const_cast because for
> RenderMathMLOperator, unembellishedOperator() needs to return *this, which
> is a pointer to a const object. I think this usage aligns with how we deal
> with other getters such as firstChild(), but I am not an expert on C++ so I
> need some professional advice on this.

By aligning, I mean that instead of

RenderBox* firstChildBox();
const RenderBox* firstChildBox() const;

WebKit uses

RenderBox* firstChildBox() const;

I am unsure why the first version is not adopted. If a child box is returned, that box can be changed, so the getter itself can potentially mutates the object. Therefore, unless the returned object is marked as const, the getter is not really const.

In any case, since we are settled down with the second one, I think the use of a const_cast as I did in the patch should be acceptable.

[Frédéric Wang (:fredw)]

Comment on attachment 329737 [details]
Patch

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

For some reason I did not receive the bug spam for this, sorry for the delay.

> Source/WebCore/ChangeLog:3
> +        [Followup to 179682] Add comment and code style improvement to MUnderOver's layout functions

I think you should use bug number 180923 and explain below that it is a follow-up.
It's either munderover (tag name) or RenderMathMLUnderOver (class name).

> Source/WebCore/ChangeLog:14
> +        It makes unembellishedOperator() a const function.

If this one is trickier, it should probably be a separate bug. Let's not delay the missing bits. Actually there is probably already a bug report for that...

> Source/WebCore/ChangeLog:16
> +        It also provide comments for stretchHorizontalOperatorsAndLayoutChildren() and elinminates

eliminate

> Source/WebCore/ChangeLog:19
> +        There is no behavior change so no new tests are necessary.

See my comment below about fixLayoutAfterStretch.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:-87
> -            fixLayoutAfterStretch(child, stretchyOperator);

If you remove this and don't do it again in the isAllStretchyOperators conditional below, then it's actually a behavior change.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:113
>          for (auto* embellishedOperator : embellishedOperators)

I think you'll need to do a for (size_t i = 0; i < embellishedOperators.size(); i++) in order to call fixLayoutAfterStretch and ensure that embellishedOperator->logicalWidth() is correctly set.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) [back 03/01/2018] from comment #7)
> I think you should use bug number 180923 and explain below that it is a
> follow-up.

Sorry, I was confused with Gecko's style. Anyway, I think it's better to explain this is a follow-up in the desc than in the title.

[Minsheng Liu]

You are correct. It strikes me that we do not have a test case for this. I will add one.

(In reply to Frédéric Wang (:fredw) [back 03/01/2018] from comment #7)
> Comment on attachment 329737 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329737&action=review
> 
> For some reason I did not receive the bug spam for this, sorry for the delay.
> 
> > Source/WebCore/ChangeLog:3
> > +        [Followup to 179682] Add comment and code style improvement to MUnderOver's layout functions
> 
> I think you should use bug number 180923 and explain below that it is a
> follow-up.
> It's either munderover (tag name) or RenderMathMLUnderOver (class name).
> 
> > Source/WebCore/ChangeLog:14
> > +        It makes unembellishedOperator() a const function.
> 
> If this one is trickier, it should probably be a separate bug. Let's not
> delay the missing bits. Actually there is probably already a bug report for
> that...
> 
> > Source/WebCore/ChangeLog:16
> > +        It also provide comments for stretchHorizontalOperatorsAndLayoutChildren() and elinminates
> 
> eliminate
> 
> > Source/WebCore/ChangeLog:19
> > +        There is no behavior change so no new tests are necessary.
> 
> See my comment below about fixLayoutAfterStretch.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:-87
> > -            fixLayoutAfterStretch(child, stretchyOperator);
> 
> If you remove this and don't do it again in the isAllStretchyOperators
> conditional below, then it's actually a behavior change.
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:113
> >          for (auto* embellishedOperator : embellishedOperators)
> 
> I think you'll need to do a for (size_t i = 0; i <
> embellishedOperators.size(); i++) in order to call fixLayoutAfterStretch and
> ensure that embellishedOperator->logicalWidth() is correctly set.

[Frédéric Wang (:fredw)]

Comment on attachment 329737 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:93
> +    // stretched to cover the base rather than the text comment, the latter of which could be very wide.

I don't think it's appropriate to insert a (vague) example in the general description of the algorithm. If you really want examples, they should be in separate paragraph describing some use cases in my opinion. But I believe it's better to have use cases as non-regression tests so that people don't change the behavior so easily in the future ;-) In any case, the "even if" part of the 4th point is rather to handle this situation of attachment 328081 [details]:

<munder>
  <mtext>SHORT TEXT</mtext>
  <msub>
    <mo>...</mo>
    <mtext>VERY LONG TEXT</mtext>
  </msub>
</munder>

where the unstretched size of the <msub> is *already* large enough to cover the width of the base. Stretching the <mo> at the core to cover the base width makes the <msub> even wider. So one interpretation of the spec might be that we actually don't need to stretch the <mo> at all. The 4th item says we don't care and stretch it anyway.

[Minsheng Liu]

Created attachment 330156 [details]
Patch

[Frédéric Wang (:fredw)]

Comment on attachment 330156 [details]
Patch

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

@Minsheng: Thanks for the test. I'm repeating my previous feedback since it seems you missed them in this patch again.

> Source/WebCore/ChangeLog:3
> +        [Followup to 179682] Add comment and code style improvement to MUnderOver's layout functions

See my previous comment about "MUnderOver" and explaining in the desc that this integrates missing changes from bug 179682. You can edit the bug title on Bugzilla BTW.

> Source/WebCore/ChangeLog:16
> +        It also provide comments for stretchHorizontalOperatorsAndLayoutChildren() and elinminates

See my previous comment about the spelling of "eliminates".

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:93
> +    // stretched to cover the base rather than the text comment, the latter of which could be very wide.

See my previous comment about this example.

> LayoutTests/mathml/opentype/munderover-stretch-width-expected.txt:26
> +This test passes if you see both the red thing the blue thing has the same width as the black bar.

that the red and blue things have

> LayoutTests/mathml/opentype/munderover-stretch-width.html:145
> +    <p>This test passes if you see both the red thing the blue thing has the same width as the black bar.</p>

Ditto.

[Minsheng Liu]

Quite sorry that I missed lots of things lately. Just focused on the behavior-changing part and forgot everything else.

(In reply to Frédéric Wang (:fredw) [back 03/01/2018] from comment #12)
> Comment on attachment 330156 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330156&action=review
> 
> @Minsheng: Thanks for the test. I'm repeating my previous feedback since it
> seems you missed them in this patch again.
> 
> > Source/WebCore/ChangeLog:3
> > +        [Followup to 179682] Add comment and code style improvement to MUnderOver's layout functions
> 
> See my previous comment about "MUnderOver" and explaining in the desc that
> this integrates missing changes from bug 179682. You can edit the bug title
> on Bugzilla BTW.
> 
> > Source/WebCore/ChangeLog:16
> > +        It also provide comments for stretchHorizontalOperatorsAndLayoutChildren() and elinminates
> 
> See my previous comment about the spelling of "eliminates".
> 
> > Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:93
> > +    // stretched to cover the base rather than the text comment, the latter of which could be very wide.
> 
> See my previous comment about this example.
> 
> > LayoutTests/mathml/opentype/munderover-stretch-width-expected.txt:26
> > +This test passes if you see both the red thing the blue thing has the same width as the black bar.
> 
> that the red and blue things have
> 
> > LayoutTests/mathml/opentype/munderover-stretch-width.html:145
> > +    <p>This test passes if you see both the red thing the blue thing has the same width as the black bar.</p>
> 
> Ditto.

[Minsheng Liu]

Created attachment 330161 [details]
Patch

[Frédéric Wang (:fredw)]

Thanks, I'm still unsure about the unembellishedOperator change, but let's see that later.

[WebKit Commit Bot]

Comment on attachment 330161 [details]
Patch

Clearing flags on attachment: 330161

Committed r226287: <https://trac.webkit.org/changeset/226287>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36202139>