Bug 76559

It would be great if you can use red and green boxes. If you can see red somewhere you know that the test fails. Right now I can see two different results between Safari and WebKit trunk and I am still not sure what I should see.

Created attachment 123310 [details] Reproducing Case w/ Red+Green I'm also seeing slightly different repaint patterns. However, each should show some red with this one.

Created attachment 123368 [details] Patch

Comment on attachment 123368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123368&action=review r- because of the test. Thanks for tackling this problem! > Source/WebCore/rendering/svg/RenderSVGImage.h:44 > + virtual void setNeedsBoundariesUpdate() { m_needsBoundariesUpdate = true; } Who is calling this? It's not clear from the patch, that this is the actual bug fix :-) > ChangeLog:8 > + * ManualTests/svg-repaint-image-clip-path.svg: Added. A manual test should really be avoided here, it's too important to not automatize it. Can you look at creating a dynamic testcase? I'd imagine doing this via a <script>, and a setTimeout(..., 0) - that should be enough to trigger it, no?

Created attachment 123488 [details] Patch

It turns out that Kellys fix was not 100% correct - I hit a smiler bug that can also be fixed, when changing this patch a little, so I uploaded a new version based on Kellys, including the layout test that I asked for (which uses layoutTestController.display() to check repainting).

Attachment 123488 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 Last 3072 characters of output: utTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86, debug, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86, debug, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86, release, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86, release, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86_64, debug, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86_64, debug, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] ERROR: FAILURES FOR <lucid, x86_64, release, gpu> in /mnt/git/webkit-style-queue/LayoutTests/platform/chromium/test_expectations.txt ERROR: Line:3927 Path does not exist. svg/filters/feImage-preserveAspectRatio.svg LayoutTests/platform/chromium/test_expectations.txt:3927: Path does not exist. svg/filters/feImage-preserveAspectRatio.svg [test/expectations] [2] Total errors found: 32 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 123488 [details] Patch Attachment 123488 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11179067 New failing tests: svg/repaint/image-with-clip-path.svg svg/custom/relative-sized-image.xhtml

Created attachment 123499 [details] Patch

Comment on attachment 123499 [details] Patch Some comments: View in context: https://bugs.webkit.org/attachment.cgi?id=123499&action=review > Source/WebCore/ChangeLog:14 > + The logic is also inconsistent, compared to the other renderes. renderers > Source/WebCore/rendering/svg/RenderSVGImage.cpp:78 > + if (oldBoundaries != m_objectBoundingBox) { > + m_imageResource->setContainerSizeForRenderer(enclosingIntRect(m_objectBoundingBox).size()); > + m_needsBoundariesUpdate = true; > + return true; > + } > + > + return false; WebKit prefers early return, I would swap these two branches. > Source/WebCore/rendering/svg/RenderSVGImage.cpp:86 > + updateImageViewport(); This function is defined as returning with bool but its return value is unused. Why? > LayoutTests/svg/repaint/image-with-clip-path-expected.txt:1 > +Bug 76559: After 200ms, A single green 50x50 rectangle should appear with center at 200, 50. All red should disappear when the green square appears. What is the reason of the gray background?

(In reply to comment #10) > renderers Fixed. > WebKit prefers early return, I would swap these two branches. Fixed. > > Source/WebCore/rendering/svg/RenderSVGImage.cpp:86 > > + updateImageViewport(); > > This function is defined as returning with bool but its return value is unused. Why? The bool is onlly used from SVGImageElement, when it determines if the bounds changed, and if so, it'll call setNeedsLayout(true) on the renderer. It serves two purposes. > > LayoutTests/svg/repaint/image-with-clip-path-expected.txt:1 > > +Bug 76559: After 200ms, A single green 50x50 rectangle should appear with center at 200, 50. All red should disappear when the green square appears. > > What is the reason of the gray background? layoutTestController.display() to test the repainting - I'll change the text color to make it more readable.

Created attachment 123533 [details] Patch v2

> layoutTestController.display() to test the repainting - I'll change the text color to make it more readable. I don't see any text anymore...

> I don't see any text anymore... Ok, so it helps to keep the png platform independent.

Comment on attachment 123533 [details] Patch v2 r=me nice patch

Committed r105613: <http://trac.webkit.org/changeset/105613>

(In reply to comment #16) > Committed r105613: <http://trac.webkit.org/changeset/105613> The new test is flaky under guard malloc, filed bug 77103 for this.