Bug 199843

WPT failed tests: http://web-platform-tests.live/svg/shapes/ellipse-02.svg http://web-platform-tests.live/svg/shapes/ellipse-03.svg http://web-platform-tests.live/svg/shapes/ellipse-05.svg http://web-platform-tests.live/svg/shapes/ellipse-06.svg http://web-platform-tests.live/svg/shapes/ellipse-07.svg http://web-platform-tests.live/svg/shapes/ellipse-08.svg

Created attachment 379046 [details] Patch

Comment on attachment 379046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379046&action=review Nice tests, thanks Said. > LayoutTests/ChangeLog:3 > + [SVG2]: Add auto behavior for rx and ry to the SVG <ellipse> and<rect> elements and<rect> -> and <rect> > LayoutTests/ChangeLog:8 > + Skip the tests with dynmaic chnages till webkit.org/b/201918 is fixed. typo: dynamic changes. Maybe add one sentence regarding the new tests?

Created attachment 379050 [details] Patch

Comment on attachment 379046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379046&action=review >> LayoutTests/ChangeLog:3 >> + [SVG2]: Add auto behavior for rx and ry to the SVG <ellipse> and<rect> elements > > and<rect> -> and <rect> Fixed. I just realized the space key in my keyboard is malfunctioning. It got stuck many times while typing this comment. >> LayoutTests/ChangeLog:8 >> + Skip the tests with dynmaic chnages till webkit.org/b/201918 is fixed. > > typo: dynamic changes. > Maybe add one sentence regarding the new tests? The typo is fixed and comments were added regarding the new tests.

Comment on attachment 379050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379050&action=review Sorry, just found another typo :/ For me the code changes look just fine. > Source/WebCore/ChangeLog:8 > + The sepcs is: https://www.w3.org/TR/SVG2/geometry.html#RxProperty. sepcs, spec :-) > Source/WebCore/ChangeLog:11 > + parse LengthOrAuto for these properties. Handle the case if one of them s/these /these /

Comment on attachment 379050 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379050&action=review > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:92 > + Length rx = style().svgStyle().rx().isAuto() ? style().svgStyle().ry() : style().svgStyle().rx(); Maybe avoid reading style().svgStyle().rx() twice. Length rx = style().svgStyle().rx(); if (rx.isAuto() rx = style().svgStyle().ry();

Created attachment 379147 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 379147 [details]: imported/w3c/web-platform-tests/websockets/bufferedAmount-unchanged-by-sync-xhr.any.worker.html bug 202003 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.

Comment on attachment 379147 [details] Patch Clearing flags on attachment: 379147 Committed r250103: <https://trac.webkit.org/changeset/250103>

All reviewed patches have been landed. Closing bug.

<rdar://problem/55532838>

Comment on attachment 379147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379147&action=review > Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:98 > + Length rx = style().svgStyle().rx(); > + if (rx.isAuto()) > + rx = style().svgStyle().ry(); > + > + Length ry = style().svgStyle().ry(); > + if (ry.isAuto()) > + ry = style().svgStyle().rx(); How about this alternative version? Length rx = style().svgStyle().rx(); Length ry = style().svgStyle().ry(); if (rx.isAuto()) rx = ry; else if (ry.isAuto()) ry = rx;

Comment on attachment 379147 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379147&action=review >> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:98 >> + ry = style().svgStyle().rx(); > > How about this alternative version? > > Length rx = style().svgStyle().rx(); > Length ry = style().svgStyle().ry(); > if (rx.isAuto()) > rx = ry; > else if (ry.isAuto()) > ry = rx; I agree. -- In the worst case scenario, which happens when rx and ry are both 'auto', my solution will calculate rx/ry four times. It will also call the Length::operator==() four times. -- Your solution will calculate rx/ry two times and will call Length::operator==() three times. -- How about this? Length rx = style().svgStyle().rx(); Length ry = style().svgStyle().ry(); m_radii = FloatSize( lengthContext.valueForLength(rx.isAuto() ? ry : rx, SVGLengthMode::Width), lengthContext.valueForLength(ry.isAuto() ? rx : ry, SVGLengthMode::Height)); This will calculate rx/ry two times and will call Length::operator==() two times.

Reopening to attach new patch.

Created attachment 379243 [details] Patch

Comment on attachment 379243 [details] Patch Clearing flags on attachment: 379243 Committed r250147: <https://trac.webkit.org/changeset/250147>

All reviewed patches have been landed. Closing bug.