[webkit-reviews] review granted: [Bug 160547] Overflow of formulas is hidden for display mathematics : [Attachment 332493] patch

Sun Jan 28 13:35:46 PST 2018

Frédéric Wang (:fredw) <fred.wang at free.fr> has granted Minsheng Liu
<lambda at liu.ms>'s request for review:
Bug 160547: Overflow of formulas is hidden for display mathematics
https://bugs.webkit.org/show_bug.cgi?id=160547

Attachment 332493: patch

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

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

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

LGTM, but please be sure to address the review comments. Thanks!

> LayoutTests/mathml/presentation/math-inline-display-width.html:25
> +	   }, "dnline math, 100px content, 200px container");

s/dnline/display/

> LayoutTests/mathml/presentation/math-inline-display-width.html:43
> +	   overflow: auto;

maybe you want "overflow: hidden" so that you don't have any potential problem
with scroll bars.

> LayoutTests/mathml/presentation/math-inline-display-width.html:45
> +	   line-height: 0;

why this line-height property?

> LayoutTests/mathml/presentation/math-inline-display-width.html:49
> +  <body onload="setTimeout(run, 2000)">

why do you need this 2 seconds delay?

> Source/WebCore/rendering/mathml/RenderMathMLMath.cpp:48
> +    LayoutUnit centerBlockOffset = std::max<LayoutUnit>(0, logicalWidth() -
contentWidth) / 2;

As I said in previous review, I think this can just be centerBlockOffset =
(logicalWidth() - contentWidth) / 2; since contentWidth < logicalWidth().