[webkit-reviews] review denied: [Bug 179682] Incorrect bounds inside <mover>/<munder> when a stretchy operator is present : [Attachment 328323] Patch

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has denied Minsheng Liu
<lambda at liu.ms>'s request for review:
Bug 179682: Incorrect bounds inside <mover>/<munder> when a stretchy operator
is present
https://bugs.webkit.org/show_bug.cgi?id=179682

Attachment 328323: Patch

https://bugs.webkit.org/attachment.cgi?id=328323&action=review




--- Comment #48 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 328323
  --> https://bugs.webkit.org/attachment.cgi?id=328323
Patch

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

We are getting closer, but I'm doing r- at least because the patch disables an
important pixel test for horizontal stretching and  causes a regression with
the base case (no embellishments). It seems some things are wrong/unclear in
the relayout, let's see if we can improve that a bit too in the next iteration.

> Source/WebCore/ChangeLog:24
> +	   what it tests is covered in the new test case.

mathml/opentype/opentype-stretchy-horizontal.html is a pixel test to check the
rendering of stretchy operators with a base size, variant size and glyph
assembly. The rendering is *not* tested by the new test so you should not
disable it. Operator spacing is irrelevant for this
mathml/opentype/opentype-stretchy-horizontal.html, though so please open a
preliminary bug that sets lspace="0px" rspace="0px" on the <mo> operators of
this mathml/opentype/opentype-stretchy-horizontal.html and updates the PNG/txt
expectations, so that we can really see what this patch is changing (hopefully
it should not change anything).

> Source/WebCore/rendering/mathml/RenderMathMLOperator.h:49
> +    void setStretchWidthLocked(bool stretchWidthLock) {
m_isStretchWidthLocked = stretchWidthLock; }

nit: The argument name should be "stretchWidthLocked"

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:73
> +    stretchyOperator->setStretchWidthLocked(true);

This leaves the stretchyOperator locked, I guess you mean
setStretchWidthLocked(false) instead?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:102
>	       child->layout();

I wonder whether that should be stretchyOperator->setNeedsLayout();
child->layoutIfNeeded();

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:113
> +	       stretchyOperators[i]->stretchTo(stretchWidthForNonBase());

This is wrong as it does not work the base case (no embellishments, first row
of attachment 328081). I think what you want is two LayoutUnit variables
stretchWidthForScripts and stretchWidthForBase both are set to the maximum of
the width of non-stretchy children in the first loop as this is what is clear
from the spec. Then you can adjust stretchWidthForScripts and
stretchWidthForBase to include other widths of stretchy children in the
calculation.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.h:52
> +    LayoutUnit stretchWidthForNonBase() const { return
firstChildBox()->logicalWidth(); };

I'm not sure you really need these helper functions. The variables and extra
comments should be enough.

> LayoutTests/TestExpectations:568
> +mathml/opentype/opentype-stretchy-horizontal.html [ Skip ]

This should not be disabled.

> LayoutTests/mathml/opentype/munderover-stretch-width.html:46
> +	   runCase(2, 'simple stretchy under', 'red', 'op');

The base case (no embellishments) is the most important one and is clear from
the spec, so I propose you move it into a separate test and performs exhaustive
testing of the combinations:

- munder/mover/munderover
- one, two or three stretchy operators.
- base/under/over as the widest non-stretchy operator (is there is one) OR
comparing with the max width of unstretched operators (if all operators are
stretchy). You can put the operator in a <mo stretchy="false"> or an <mtext> to
get the unstretched size.

Currently, your test is only checking munder/mover, one stretchy operator as
the script and one non-stretchy as the base so you can not see that your code
is wrong.