Bug 180348 – Split layout of RenderMathMLRow into smaller steps

• Component: MathML
• Status: RESOLVED FIXED

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

Split layout of RenderMathMLRow into smaller steps

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

Created attachment 328340 [details]
Patch

[Javier Fernandez]

Comment on attachment 328340 [details]
Patch

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

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:154
> +        LayoutUnit childHorizontalOffset = !style().isLeftToRightDirection() ? width - horizontalOffset - childWidth : horizontalOffset;

Wouldn't be easier to read if we avoid the ! and just switch the terms ? Also, I guess LTR is the most common use case, isn't it ?

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:178
> +        LayoutUnit centerBlockOffset = 0;

Why we initialize this variable to assign a new value just in the line bellow ?

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:180
> +        if (!style().isLeftToRightDirection() && centerBlockOffset > 0)

Given how we calculate the  centerBlockOffset value, is it possible that it's below 0 ?

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:181
> +            centerBlockOffset = -centerBlockOffset;

I think I don't understand this line; isn't it equivalent to assign it to 0 ?

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

Comment on attachment 328340 [details]
Patch

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

>> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:154
>> +        LayoutUnit childHorizontalOffset = !style().isLeftToRightDirection() ? width - horizontalOffset - childWidth : horizontalOffset;
> 
> Wouldn't be easier to read if we avoid the ! and just switch the terms ? Also, I guess LTR is the most common use case, isn't it ?

Right, I expanded the old shouldFlipHorizontal variable but it makes more sense to exchange the term.

>> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:178
>> +        LayoutUnit centerBlockOffset = 0;
> 
> Why we initialize this variable to assign a new value just in the line bellow ?

Yeah, we don't need initialization. It's a line taken from the old code.

>> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:180
>> +        if (!style().isLeftToRightDirection() && centerBlockOffset > 0)
> 
> Given how we calculate the  centerBlockOffset value, is it possible that it's below 0 ?

It's again a line taken from the old code (which might also be in the flexbox one). Definitely, it will be >= 0 so that condition only skips the case where centerBlockOffset == 0. In that case, centerBlockOffset = -centerBlockOffset probably does not have any effect so it's not clear why the old code was doing it.

>> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:181
>> +            centerBlockOffset = -centerBlockOffset;
> 
> I think I don't understand this line; isn't it equivalent to assign it to 0 ?

No, it's just a = not a +=. It's just changing the sign of centerBlockOffset (again this is from the old code). And does not have any effect iff centerBlockOffset == 0.

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

Created attachment 329751 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 329751 [details]
Patch

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

Patch looks good and new methods are easier to understand.
Just a concern from my side, how many for loops do we have now and how many do we have in the past.
It's clear that we're looping the children several times (I guess that's unavoidable)
and I'm wondering about the performance implications of that.

> Source/WebCore/ChangeLog:46
> +        (WebCore::RenderMathMLRow::computePreferredLogicalWidths): Skip out-of-flow children in the
> +        preferred width calculation.

Mmm, this seems like a change of behavior regarding previous status.
Would be possible to write a test for this or not?
Note that sometimes this is not possible with a layout test and only with unit tests
(at least we've used them for that on Blink https://chromium-review.googlesource.com/702437).

> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:83
> +        if (!toVerticalStretchyOperator(child)) {

Nit: I'd change this for:
  if (toVerticalStretchyOperator(child))
      continue;

  // Then the rest of the code goes here.

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

Comment on attachment 329751 [details]
Patch

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

In general the asymptotic behavior of browsing the child list 5 times (in the past) or 6 times (as done now) is the same. However, we are really not adding new operations so the total number should be the same. Actually I would even say less since the 6th loop is only executed for <math display="block"> (instead of any mrow-like element in the past) and the setLogicalWidth/setLogicalHeight calls are now only done once in the class's layoutBlock (in the past setLogicalHeight was repeated in a loop and the set calls were redone in the derived classes). Not to mention that in practice most mrow-like elements have a small number of children so compilers might actually expand the loops when optimizing code...

>> Source/WebCore/ChangeLog:46
>> +        preferred width calculation.
> 
> Mmm, this seems like a change of behavior regarding previous status.
> Would be possible to write a test for this or not?
> Note that sometimes this is not possible with a layout test and only with unit tests
> (at least we've used them for that on Blink https://chromium-review.googlesource.com/702437).

Yeah. I was not sure whether it was too important to test this edge case of out-of-flow children (e.g. Gecko positions them normally) but I guess we should. Something like this should exhibit the difference:

    <table>
      <tr>
        <td style="background: blue">
          <math>
            <mspace style="position: absolute;" width="100px"></mspace>
            <mspace width="100px"></mspace>
          </math>
        </td>
      </tr>
    </table>

>> Source/WebCore/rendering/mathml/RenderMathMLRow.cpp:83
>> +        if (!toVerticalStretchyOperator(child)) {
> 
> Nit: I'd change this for:
>   if (toVerticalStretchyOperator(child))
>       continue;
> 
>   // Then the rest of the code goes here.

Right.

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

Created attachment 329911 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 329911 [details]
Patch

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

Thanks for the explanation.
r=me

> LayoutTests/mathml/mrow-preferred-width-with-out-of-flow-child.html:8
> +    <p>This test passes if the absolute positioned child is not taken in account in the preferred width computation.</p>

Nit: Usually you have a description of the test (something like you have wrote). And then the condition of how to visually check it passes, things like "you'll see a green square and not red" (I believe this can be improved :-)).
Nit 2: Maybe we could do this test in WPT too (or only there, dunno how the export process on WebKit is going).

[Manuel Rego Casasnovas]

Comment on attachment 329751 [details]
Patch

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

>>> Source/WebCore/ChangeLog:46
>>> +        preferred width calculation.
>> 
>> Mmm, this seems like a change of behavior regarding previous status.
>> Would be possible to write a test for this or not?
>> Note that sometimes this is not possible with a layout test and only with unit tests
>> (at least we've used them for that on Blink https://chromium-review.googlesource.com/702437).
> 
> Yeah. I was not sure whether it was too important to test this edge case of out-of-flow children (e.g. Gecko positions them normally) but I guess we should. Something like this should exhibit the difference:
> 
>     <table>
>       <tr>
>         <td style="background: blue">
>           <math>
>             <mspace style="position: absolute;" width="100px"></mspace>
>             <mspace width="100px"></mspace>
>           </math>
>         </td>
>       </tr>
>     </table>

Yes, that or wrapping it in a floated element should do the trick.

It's not very important, but as we're actually fixing a bug I prefer we have a test for it.

[Manuel Rego Casasnovas]

Comment on attachment 329751 [details]
Patch

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

>>> Source/WebCore/ChangeLog:46
>>> +        preferred width calculation.
>> 
>> Mmm, this seems like a change of behavior regarding previous status.
>> Would be possible to write a test for this or not?
>> Note that sometimes this is not possible with a layout test and only with unit tests
>> (at least we've used them for that on Blink https://chromium-review.googlesource.com/702437).
> 
> Yeah. I was not sure whether it was too important to test this edge case of out-of-flow children (e.g. Gecko positions them normally) but I guess we should. Something like this should exhibit the difference:
> 
>     <table>
>       <tr>
>         <td style="background: blue">
>           <math>
>             <mspace style="position: absolute;" width="100px"></mspace>
>             <mspace width="100px"></mspace>
>           </math>
>         </td>
>       </tr>
>     </table>

Yes, that or wrapping it in a floated element should do the trick.

It's not very important, but as we're actually fixing a bug I prefer we have a test for it.

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

(In reply to Manuel Rego Casasnovas from comment #8)
> Nit: Usually you have a description of the test (something like you have
> wrote). And then the condition of how to visually check it passes, things
> like "you'll see a green square and not red" (I believe this can be improved
> :-)).

OK, will try rewording before landing.

> Nit 2: Maybe we could do this test in WPT too (or only there, dunno how the
> export process on WebKit is going).
> It's not very important, but as we're actually fixing a bug I prefer we have
> a test for it.

This is fixing an inconsistency between the layout VS preferred width calculations, so you're right we should have a test.

However, as as said by chat, the MathML spec is not really clear about how to deal with out-of-flow positioned elements (e.g. what about a fraction with an absolutely positioned numerator? is it laid out as invalid markup because a child is "missing"? or as a fraction with empty numerator?) so I'd prefer not to put this in WPT for now. It's already a reftest so it should be straightforward to port in the future. Also, Gecko just positioned out-of-flow children normally so we have a compatibility issue... Let's wait whether the WG will clarify it in the future!

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

Created attachment 329915 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 329915 [details]
Patch for landing

Rejecting attachment 329915 [details] from commit-queue.

New failing tests:
mathml/mrow-preferred-width-with-out-of-flow-child.html
Full output: http://webkit-queues.webkit.org/results/5773630

[WebKit Commit Bot]

Created attachment 329918 [details]
Archive of layout-test-results from webkit-cq-01 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

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

Created attachment 329920 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 329920 [details]
Patch for landing

Clearing flags on attachment: 329920

Committed r226180: <https://trac.webkit.org/changeset/226180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/36156500>