[webkit-reviews] review denied: [Bug 107848] [CSS Filters] Using negative drop-shadow radius values has slow performance : [Attachment 191040] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Michelangelo De Simone
<michelangelo at webkit.org>'s request for review:
Bug 107848: [CSS Filters] Using negative drop-shadow radius values has slow
performance
https://bugs.webkit.org/show_bug.cgi?id=107848

Attachment 191040: Patch
https://bugs.webkit.org/attachment.cgi?id=191040&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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 :).