[Webkit-unassigned] [Bug 179682] Incorrect bounds inside <mover>/<munder> when a stretchy operator is present

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=179682

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

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

@Minsheng Liu: OK, I think this looks much better now! However you did not address/reply to some of my previous comments and especially the fact that the stretch operator remains locked in fixLayoutAfterStretch. We are close I guess I can r+ in the next iteration once all the comments are addressed.

Regarding the GTK / Windows, you don't have to worry about it for now, we can land the patch as it and let the port maintainers update the expectations (or ask them to do it). You can also retrieve them from https://build.webkit.org/console after the patch lands and submit a follow-up patch.

> Source/WebCore/ChangeLog:20
> +        mathml/opentype/opentype-stretchy-horizonta.html

nit: horizontal

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

nit: argument name should be "stretchWidthLocked"

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

I'm still not sure why this is not stretchyOperator->setStretchWidthLocked(false) ?

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

Does it work if you rewrite this

stretchyOperator->resetStretchSize();
fixLayoutAfterStretch(child, stretchyOperator);

?

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:101
> +    }

Ah OK, this was the trick! That makes the code much more readable and understandable indeed. I think it would be great to add a C++ comment at the top of the function to explain the algorithm and rationale (comparing with the spec & Gecko maybe) since as you see we took some time to figure it out.

> Source/WebCore/rendering/mathml/RenderMathMLUnderOver.cpp:103
> +    for (std::size_t i = 0; i < embellishedOperators.size(); i ++) {

nit 1: do you really need the namespace prefix for size_t?
nit 2: there should be no space between i and ++.

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

I really think it would be nice to have more tests for the base case. But I guess it's ok to do that in a separate patch.

> LayoutTests/mathml/opentype/munderover-stretch-width.html:52
> +        runCase(8, 'simpe stretchy under should equal over', 'red', 'op');

s/simpe/simple/

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20171205/29d98206/attachment-0001.html>