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

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted 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 328775: Patch

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




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

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

Thanks, I have some minor comments below but otherwise LGTM. It looks like
wincairo failed, you should probably consider rebasing your patch against the
latest upstream revision.

> Source/WebCore/rendering/mathml/RenderMathMLOperator.cpp:181
> +    

nit: please remove the trailing whitespace.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:63
> +    

nit: please remove the trailing whitespace.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:75
> +    ASSERT(needsLayout());

Can you please add the following comment:

// We apply horizontal stretchy rules from the MathML spec (sections 3.2.5.8.3
and 3.2.5.8.4), which 
// can be roughly summarized as "stretching operators to the maximum widths of
all children" and
// minor variations of that algorithm do not affect the result. However, the
spec is a bit ambiguous
// for embellished operators (section 3.2.5.7.3) and different approaches can
lead to significant
// stretch size differences. We made the following decisions:
// - The unstretched size is the embellished operator width with the <mo> at
the core unstretched.
// - In general, the target size is just the maximum widths of non-stretchy
children because the
//   embellishments could make widths significantly larger.
// - In the edge case when all operators of stretchy, we follow the
specification and take the
//   maximum of all unstretched sizes.
// - The <mo> at the core is stretched to cover the target size, even if the
embellished operator
//   might become much wider.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:76
> +    

nit: trailing whitespace.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:81
> +    

nit: trailing whitespace

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

It is a bit sad that we have to do this relayout when we actually only measure
the unstretched size for the edge case isAllStretchyOperators == true. Are you
able to move that fixLayoutAfterStretch call into the isAllStretchyOperators
conditional?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:94
> +    

nit: trailing whitespace

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:99
> +    

nit: trailing whitespace