Using a negative radius for the drop-shadow filters has slow repaint performance. Also added a bug on the spec to check what's the right behavior for negative radius lengths: https://www.w3.org/Bugs/Public/show_bug.cgi?id=20762 Example: -webkit-filter: drop-shadow(10px 10px -1px red)
<rdar://problem/13118861>
Alex, do you think you'll address this soon? It seems like an easy fix.
I will take a look at it.
FEDropShadow and FEGaussianBlur are also affected, meaning that SVGFEDropShadowElement and SVGFEGaussianBlurElement also reproduce the issue.
Alex's alter ego will take a look at this.:)
Created attachment 191040 [details] Patch
Attachment 191040 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/effect-blur-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-blur-negative-radius.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius.html', u'LayoutTests/css3/filters/filter-property-parsing-invalid-expected.txt', u'LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius-expected.html', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius.html', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/svg/filters/feDropShadow-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feDropShadow-negative-deviation.svg', u'LayoutTests/svg/filters/feDropShadow-zero-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-zero-deviation.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp', u'Source/WebCore/svg/SVGFEDropShadowElement.cpp', u'Source/WebCore/svg/SVGFEGaussianBlurElement.cpp']" exit_code: 1 LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191040&action=review > Source/WebCore/css/CSSParser.cpp:6276 > + } else if (validUnit(val, FLength | FNonNeg, CSSStrictMode)) { Do the specs say that negative radii are invalid, or clamped to zero?
(In reply to comment #8) > Do the specs say that negative radii are invalid, or clamped to zero? Specs claim "A negative value is an error...", so I guess it's not to be clamped.
Then that should cause the entire property to be ignored. Is that the behavior of your patch?
(In reply to comment #10) > Then that should cause the entire property to be ignored. Is that the behavior of your patch? Yep. Actually something like blur(-1px) was already covered by existing parsing tests.
Comment on attachment 191040 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191040&action=review The patch looks good to me in general. Just some change requests. Please check why the bots seems to have problems. > Source/WebCore/ChangeLog:28 > + (WebCore::FEGaussianBlur::calculateUnscaledKernelSize): Added "non-negative" assertion. > + * svg/SVGFEDropShadowElement.cpp: > + (WebCore::SVGFEDropShadowElement::build): If a negative standard deviation is encountered, don't Can you write some more lines what you did? IIRC there even was a problem with not returning early here? >> Source/WebCore/css/CSSParser.cpp:6276 >> + } else if (validUnit(val, FLength | FNonNeg, CSSStrictMode)) { > > Do the specs say that negative radii are invalid, or clamped to zero? Your comment should really say that. The spec follows CSS background and borders. And CSS3BG says that negative values for the radius is not allowed. > LayoutTests/css3/filters/effect-blur-negative-radius.html:10 > + -webkit-filter: blur(-1px); I wonder if we do not have negative tests for the -webkit-filter prefix? Maybe it is not necessary to add a ref test just for a parser check. > LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js:96 > +testInvalidFilterRule("Negative radius", "drop-shadow(10px 10px -1px red)"); Here you go. This makes the other test unnecessary. > LayoutTests/fast/box-shadow/box-shadow-negative-radius.html:35 > + The following divs have box-shadow associated with them; every shadow is declared > + using a mix of invalid negative values, which are clearly wrong. Ditto. Don't we have box-shadow parser tests? Since it should fail on parsing, this test seems to be no necessary. Thank you for taking the time to write this test :).
Created attachment 191548 [details] Patch
(In reply to comment #12) I've uploaded a revised patch according to your comments. > The patch looks good to me in general. Just some change requests. Please check why the bots seems to have problems. The check-webkit-style seems to be complaining about svn auto-props for PNGs, which AFAICT is a local configuration thingy not really related to the patch. Anyhow, I've just filed bug 111474. I'll eventually land this manually. > Can you write some more lines what you did? IIRC there even was a problem with not returning early here? Done. > Your comment should really say that. The spec follows CSS background and borders. And CSS3BG says that negative values for the radius is not allowed. Done. > > LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js:96 > > +testInvalidFilterRule("Negative radius", "drop-shadow(10px 10px -1px red)"); > Here you go. This makes the other test unnecessary. I've removed the blur-negative test, which was redundant. > > LayoutTests/fast/box-shadow/box-shadow-negative-radius.html:35 > > + The following divs have box-shadow associated with them; every shadow is declared > > + using a mix of invalid negative values, which are clearly wrong. > Ditto. Don't we have box-shadow parser tests? Since it should fail on parsing, this test seems to be no necessary. Thank you for taking the time to write this test :). I didn't find any existing parsing test for box-shadow so I thought to leave this test as it is; let me know if there's something I didn't check out. Thanks!
Attachment 191548 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius.html', u'LayoutTests/css3/filters/filter-property-parsing-invalid-expected.txt', u'LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius-expected.html', u'LayoutTests/fast/box-shadow/box-shadow-negative-radius.html', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/svg/filters/feDropShadow-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feDropShadow-negative-deviation.svg', u'LayoutTests/svg/filters/feDropShadow-zero-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-zero-deviation.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp', u'Source/WebCore/svg/SVGFEDropShadowElement.cpp', u'Source/WebCore/svg/SVGFEGaussianBlurElement.cpp']" exit_code: 1 LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 191548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191548&action=review Please look onto the style issue. Looks like you need to set the mime type manually? There is just one little change request. Other than that the patch looks great! > LayoutTests/ChangeLog:16 > + * fast/box-shadow/box-shadow-negative-radius-expected.html: Added. > + * fast/box-shadow/box-shadow-negative-radius.html: Added. box shadow seems really to miss parsing tests. That is really really bad. Would it be possible that you create a box-shadow-parsing-invalid.html similar to other tests where you test just your part. Create a new bug report to add parser tests for box-shadow and add a FIXME into this new created file. I really appreciate if you do that. Thanks.
Created attachment 191602 [details] Patch
Created attachment 192034 [details] Patch
Created attachment 192039 [details] Patch
(In reply to comment #16) > Please look onto the style issue. Looks like you need to set the mime type manually? There is just one little change request. Other than that the patch looks great! As far as I know webkit-patch land should take care of setting the mime type on PNGs automatically (see bug 75825). I've a git only repo here so, in case this is not the case, I'll handle this manually. > > LayoutTests/ChangeLog:16 > > + * fast/box-shadow/box-shadow-negative-radius-expected.html: Added. > > + * fast/box-shadow/box-shadow-negative-radius.html: Added. > box shadow seems really to miss parsing tests. That is really really bad. Would it be possible that you create a box-shadow-parsing-invalid.html similar to other tests where you test just your part. Create a new bug report to add parser tests for box-shadow and add a FIXME into this new created file. I really appreciate if you do that. Thanks. As requested, I've added a new test for box-shadow invalid parsing test and filed a new bug for it. Thanks.
Attachment 192039 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius-expected.html', u'LayoutTests/css3/filters/effect-drop-shadow-negative-radius.html', u'LayoutTests/css3/filters/filter-property-parsing-invalid-expected.txt', u'LayoutTests/css3/filters/script-tests/filter-property-parsing-invalid.js', u'LayoutTests/fast/box-shadow/box-shadow-parsing-invalid-expected.txt', u'LayoutTests/fast/box-shadow/box-shadow-parsing-invalid.html', u'LayoutTests/fast/box-shadow/script-tests/box-shadow-parsing-invalid.js', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/chromium-mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feDropShadow-zero-deviation-expected.txt', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png', u'LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.txt', u'LayoutTests/svg/filters/feDropShadow-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feDropShadow-negative-deviation.svg', u'LayoutTests/svg/filters/feDropShadow-zero-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation-expected.svg', u'LayoutTests/svg/filters/feGaussianBlur-negative-deviation.svg', u'LayoutTests/svg/filters/feGaussianBlur-zero-deviation.svg', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp', u'Source/WebCore/svg/SVGFEDropShadowElement.cpp', u'Source/WebCore/svg/SVGFEGaussianBlurElement.cpp']" exit_code: 1 LayoutTests/platform/mac/svg/filters/feGaussianBlur-zero-deviation-expected.png:0: Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png"). [image/png] [5] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 192039 [details] Patch LGTM. r=me. Let's see if the landing bot can handle it.
Comment on attachment 192039 [details] Patch Rejecting attachment 192039 [details] from commit-queue. New failing tests: fast/repaint/shadow-multiple-strict-vertical.html fast/box-shadow/inset-shadow-large-offset.html fast/box-shadow/spread-multiple-inset.html fast/repaint/box-shadow-inset-repaint.html http/tests/cache/subresource-failover-to-network.html fast/borders/border-radius-with-box-shadow-01.html fast/borders/border-shadow-large-radius.html compositing/filters/sw-shadow-overlaps-hw-shadow.html fast/box-shadow/spread.html fast/repaint/shadow-multiple-strict-horizontal.html fast/box-shadow/spread-multiple-normal.html fast/dom/title-directionality-removeChild.html fast/regions/element-region-overset-state-vertical-rl.html fast/loader/text-document-wrapping.html fast/repaint/box-shadow-h.html fast/repaint/shadow-multiple-horizontal.html fast/css/shadow-multiple.html compositing/filters/sw-layer-overlaps-hw-shadow.html fast/dom/title-directionality.html css3/filters/filter-repaint-shadow-rotated.html fast/box-shadow/inset.html fast/repaint/box-shadow-v.html fast/box-shadow/box-shadow-clipped-slices.html fast/repaint/shadow-multiple-vertical.html fast/box-shadow/inset-box-shadows.html fast/box-shadow/no-blur-multiple-offsets.html fast/loader/javascript-url-in-object.html css3/filters/filter-repaint-shadow-clipped.html compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html Full output: http://webkit-commit-queue.appspot.com/results/16991477
Comment on attachment 192039 [details] Patch Attachment 192039 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17025482 New failing tests: platform/mac/editing/deleting/deletionUI-single-instance.html compositing/filters/sw-layer-overlaps-hw-shadow.html svg/css/shadow-changes.svg svg/filters/shadow-on-rect-with-filter.svg http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html fast/repaint/box-shadow-h.html compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html fast/repaint/box-shadow-v.html fast/regions/element-region-overset-state-vertical-rl.html svg/css/text-shadow-multiple.xhtml svg/css/shadow-with-negative-offset.svg fast/loader/javascript-url-in-object.html transitions/cubic-bezier-overflow-shadow.html media/video-played-collapse.html svg/css/shadow-and-opacity.svg compositing/filters/sw-shadow-overlaps-hw-shadow.html http/tests/plugins/third-party-cookie-accept-policy.html
Looks like there are "few" tests that need to be rebased.:-/
(In reply to comment #25) > Looks like there are "few" tests that need to be rebased.:-/ Do these reproduce for you locally?
Comment on attachment 192039 [details] Patch Attachment 192039 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17000570 New failing tests: fast/repaint/shadow-multiple-strict-vertical.html fast/box-shadow/inset-shadow-large-offset.html fast/box-shadow/spread-multiple-inset.html fast/repaint/box-shadow-inset-repaint.html fast/hidpi/video-controls-in-hidpi.html http/tests/cache/subresource-failover-to-network.html fast/borders/border-radius-with-box-shadow-01.html fast/borders/border-shadow-large-radius.html compositing/filters/sw-shadow-overlaps-hw-shadow.html fast/box-shadow/spread.html fast/repaint/shadow-multiple-strict-horizontal.html fast/preloader/document-write-noscript.html fast/regions/element-region-overset-state-vertical-rl.html fast/loader/text-document-wrapping.html fast/repaint/box-shadow-h.html fast/repaint/shadow-multiple-horizontal.html fast/css/shadow-multiple.html compositing/filters/sw-layer-overlaps-hw-shadow.html css3/filters/filter-repaint-shadow-rotated.html fast/box-shadow/inset.html fast/repaint/box-shadow-v.html fast/box-shadow/spread-multiple-normal.html fast/box-shadow/box-shadow-clipped-slices.html fast/repaint/shadow-multiple-vertical.html fast/box-shadow/inset-box-shadows.html fast/box-shadow/no-blur-multiple-offsets.html fast/loader/javascript-url-in-object.html css3/filters/filter-repaint-shadow-clipped.html compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html fast/repaint/text-shadow-horizontal.html
(In reply to comment #26) > Do these reproduce for you locally? Almost all of them. I'm going over each one manually.
Comment on attachment 192039 [details] Patch Attachment 192039 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17136630 New failing tests: compositing/filters/sw-layer-overlaps-hw-shadow.html svg/filters/shadow-on-rect-with-filter.svg svg/css/shadow-changes.svg fast/repaint/box-shadow-h.html compositing/filters/sw-nested-shadow-overlaps-hw-nested-shadow.html svg/css/shadow-and-opacity.svg fast/regions/element-region-overset-state-vertical-rl.html svg/css/shadow-with-negative-offset.svg transitions/multiple-shadow-transitions.html svg/css/text-shadow-multiple.xhtml transitions/svg-text-shadow-transition.html transitions/cubic-bezier-overflow-shadow.html fast/repaint/box-shadow-v.html compositing/filters/sw-shadow-overlaps-hw-shadow.html
Created attachment 194530 [details] Patch
Comment on attachment 194530 [details] Patch Attachment 194530 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17141553 New failing tests: svg/filters/feGaussianBlur-zero-deviation.svg svg/filters/feDropShadow-zero-deviation.svg
Created attachment 194539 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
(In reply to comment #31) > New failing tests: > svg/filters/feGaussianBlur-zero-deviation.svg > svg/filters/feDropShadow-zero-deviation.svg These two tests seem to be false positives.
Created attachment 194577 [details] Patch
Comment on attachment 194577 [details] Patch LGTM. r=me
Comment on attachment 194577 [details] Patch Clearing flags on attachment: 194577 Committed r146762: <http://trac.webkit.org/changeset/146762>
All reviewed patches have been landed. Closing bug.