Skip to content

Conversation

@cdumez
Copy link
Contributor

@cdumez cdumez commented Apr 13, 2023

2e76ca3

REGRESSION(262844@main): css3/filters/reference-filter-change-repaint.html is randomly failing
https://bugs.webkit.org/show_bug.cgi?id=255383

Reviewed by Said Abou-Hallawa.

262844@main slightly changed the ordering of attribute processing in SVGFETurbulenceElement
and it was caused this repaint test to become flaky. We need to make sure that
SVGFETurbulenceElement::attributeChanged() calls this first:
```
m_seed->setBaseValInternal(newValue.toFloat());
```
when the seed attribute changes, before calling the base class's attributeChanged.

This patch restores the ordering to address the issue.

* Source/WebCore/svg/SVGFETurbulenceElement.cpp:
(WebCore::SVGFETurbulenceElement::attributeChanged):

Canonical link: https://commits.webkit.org/262985@main

0da5a72

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@cdumez cdumez self-assigned this Apr 13, 2023
@cdumez cdumez added the CSS Cascading Style Sheets implementation label Apr 13, 2023
@cdumez cdumez marked this pull request as ready for review April 13, 2023 17:33
@cdumez
Copy link
Contributor Author

cdumez commented Apr 14, 2023

Patch is ready for review. It fixes a very recent regression.

Copy link
Contributor

@shallawa shallawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change seems fine. But how can we make sure 262844@main did not change the order of processing the attributes of something else? Or is this change going to affect layout tests only?

@cdumez cdumez added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 14, 2023
@cdumez
Copy link
Contributor Author

cdumez commented Apr 14, 2023

The change seems fine. But how can we make sure 262844@main did not change the order of processing the attributes of something else? Or is this change going to affect layout tests only?

I tried to be careful when I made 262844@main but clearly I got the ordering slightly wrong in some cases. I think this is mostly an issue for SVG because of svgAttributeChanged(). I'll take another look at the SVG changes to double check ordering. I am also curious if we really need a separate svgAttributeChanged() for SVG elements.

….html is randomly failing

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

Reviewed by Said Abou-Hallawa.

262844@main slightly changed the ordering of attribute processing in SVGFETurbulenceElement
and it was caused this repaint test to become flaky. We need to make sure that
SVGFETurbulenceElement::attributeChanged() calls this first:
```
m_seed->setBaseValInternal(newValue.toFloat());
```
when the seed attribute changes, before calling the base class's attributeChanged.

This patch restores the ordering to address the issue.

* Source/WebCore/svg/SVGFETurbulenceElement.cpp:
(WebCore::SVGFETurbulenceElement::attributeChanged):

Canonical link: https://commits.webkit.org/262985@main
@webkit-commit-queue webkit-commit-queue force-pushed the 255383_SVGFETurbulence_flakiness branch from 0da5a72 to 2e76ca3 Compare April 14, 2023 19:47
@webkit-commit-queue
Copy link
Collaborator

Committed 262985@main (2e76ca3): https://commits.webkit.org/262985@main

Reviewed commits have been landed. Closing PR #12699 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 2e76ca3 into WebKit:main Apr 14, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Apr 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CSS Cascading Style Sheets implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants