Created attachment 328340 [details] Patch

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 ?

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.

Created attachment 329751 [details] Patch

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.

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.

Created attachment 329911 [details] Patch

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).

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.

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.

(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!

Created attachment 329915 [details] Patch for landing

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

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

Created attachment 329920 [details] Patch for landing

Comment on attachment 329920 [details] Patch for landing Clearing flags on attachment: 329920 Committed r226180: <https://trac.webkit.org/changeset/226180>

All reviewed patches have been landed. Closing bug.

<rdar://problem/36156500>