Bug 224664

Created attachment 426207 [details] Patch

Comment on attachment 426207 [details] Patch Hi, I think this patch is ready for review. Thanks:)

Created attachment 426222 [details] Patch

Comment on attachment 426222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426222&action=review > LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3 > -FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 4 +/- 0.001 but got 4.166666666666667 > +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.2547169811320755 +/- 0.001 but got 1.25 Given this patch is supposed to fix a bug, we’d normally want a test that was failing that now passes. Instead we have one failure being replaced by another? Could you explain, please? Especially wondering if we could construct a test that could show the value of the change, failing before and passing now.

Comment on attachment 426222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426222&action=review >> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3 >> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.2547169811320755 +/- 0.001 but got 1.25 > > Given this patch is supposed to fix a bug, we’d normally want a test that was failing that now passes. Instead we have one failure being replaced by another? Could you explain, please? > > Especially wondering if we could construct a test that could show the value of the change, failing before and passing now. I believe the test is not well designed, instead of listing all subtests any fail means the test is stopped. So there is progress but due to the test design it is not clear. I think Antti agreed as well that the test is not ideal.

Comment on attachment 426222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426222&action=review Hi Darin and Rob, Thanks for the review, and sorry for not to explain the patch clearly! >>> LayoutTests/imported/w3c/web-platform-tests/html/rendering/replaced-elements/attributes-for-embedded-content-and-images/img-aspect-ratio-expected.txt:3 >>> +FAIL Image width and height attributes are used to infer aspect-ratio assert_approx_equals: expected 1.2547169811320755 +/- 0.001 but got 1.25 >> >> Given this patch is supposed to fix a bug, we’d normally want a test that was failing that now passes. Instead we have one failure being replaced by another? Could you explain, please? >> >> Especially wondering if we could construct a test that could show the value of the change, failing before and passing now. > > I believe the test is not well designed, instead of listing all subtests any fail means the test is stopped. So there is progress but due to the test design it is not clear. I think Antti agreed as well that the test is not ideal. Like Rob said img-aspect-ratio.html is not well-designed. The test case with width 0.8 and height 0.2 in img-aspect-ratio.html can reveal the different values. Before this patch the aspect ratio is 4.166666666666667, so it failed. After this patch, the aspect ratio is 4. In the code, 0.8 and 0.2 is converted to LayoutUnit, stored as m_intrinsicSize, then use it to calculate logical width and height. Because LayoutUnit is not accurate to present the float values, so the aspect-ratio in this test is not accurate. IIUC, the width and height attribute value is not the actual intrinsic size, so it shouldn't impact the intrinsic size. And if we need intrinsic size to decide the logical width, we should wait for the image downloaded. PS: I will add an explanation to the ChangeLog.

Created attachment 426335 [details] Patch

Comment on attachment 426335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426335&action=review Looks good, but we can do better. > Source/WebCore/rendering/RenderReplaced.cpp:506 > + float attributeWidth = parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::widthAttr)).valueOr(0); > + float attributeHeight = parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::heightAttr)).valueOr(0); The parseValidHTMLFloatingPointNumber function returns an Optional<double>. But this code truncates that double to a float, then divides as float, then converts back to double. I suggest using auto here, or double and not involving single-precision float at all.

Comment on attachment 426335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426335&action=review >> Source/WebCore/rendering/RenderReplaced.cpp:506 >> + float attributeHeight = parseValidHTMLFloatingPointNumber(node->getAttribute(HTMLNames::heightAttr)).valueOr(0); > > The parseValidHTMLFloatingPointNumber function returns an Optional<double>. But this code truncates that double to a float, then divides as float, then converts back to double. I suggest using auto here, or double and not involving single-precision float at all. Done! Thanks:)

Created attachment 426373 [details] Patch

Committed r276228 (236710@main): <https://commits.webkit.org/236710@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426373 [details].

<rdar://problem/77110069>