[webkit-reviews] review denied: [Bug 52162] Implement CSS3 Images cross-fade() image function : [Attachment 112832] parse -webkit-cross-fade

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 52162: Implement CSS3 Images cross-fade() image function
https://bugs.webkit.org/show_bug.cgi?id=52162

Attachment 112832: parse -webkit-cross-fade
https://bugs.webkit.org/attachment.cgi?id=112832&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112832&action=review


Also, patch doesn't apply.

>
LayoutTests/fast/css/getComputedStyle/computed-style-cross-fade-expected.txt:16

> +PASS testCrossfade("background-image: -webkit-cross-fade(20%,
url(http://example.com/a.png), url(http://example.com/b.png))",
"background-image") is "none"
> +PASS successfullyParsed is true

You should test
-webkit-cross-fade(url(..),)
-webkit-cross-fade(,)
-webkit-cross-fade(,url(...)) as well.

> Source/WebCore/css/CSSCrossfadeValue.h:50
> +    void setFromImage(const PassRefPtr<CSSImageValue> fromImage) {
m_fromImage = fromImage; }
> +    void setToImage(const PassRefPtr<CSSImageValue> toImage) { m_toImage =
toImage; }
> +    void setPercentage(const PassRefPtr<CSSPrimitiveValue> percentage) {
m_percentage = percentage; }

No need for the consts.

Perhaps you should just pass the two CSSImageValues into the create() method
and ctor?

> Source/WebCore/css/CSSParser.cpp:2791
> +bool CSSParser::parseFillImage(CSSParserValueList * valueList,
RefPtr<CSSValue>& value)

No space before the * for any of these.

> Source/WebCore/css/CSSParser.cpp:6195
> +    RefPtr<CSSCrossfadeValue> result = CSSCrossfadeValue::create();

Might as well delay this until you actually need it.

> Source/WebCore/css/CSSParser.cpp:6229
> +    if (!a || a->unit != CSSPrimitiveValue::CSS_PERCENTAGE)
> +	   return false;

Should we allow fractions here ("0.4")?