[webkit-reviews] review denied: [Bug 27577] [CSS3 Backgrounds and Borders] Add background-size to the background shorthand : [Attachment 140130] Patch-with-getComputedStyle-changes

[bugzilla-daemon at webkit.org]

Alexis Menard (darktears) <alexis.menard at openbossa.org> has denied Joe Thomas
<joethomas at motorola.com>'s request for review:
Bug 27577: [CSS3 Backgrounds and Borders] Add background-size to the background
shorthand
https://bugs.webkit.org/show_bug.cgi?id=27577

Attachment 140130: Patch-with-getComputedStyle-changes
https://bugs.webkit.org/attachment.cgi?id=140130&action=review

------- Additional Comments from Alexis Menard (darktears)
<alexis.menard at openbossa.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140130&action=review


It's missing also tests for broken cases like for example : red
url(dummy://test.png) / cover basically making sure you parse correctly or drop
correctly the rule.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2701
> +    size_t count = 0;

unsigned is better, and call it i.

> Source/WebCore/css/CSSParser.cpp:2674
>		   }

I don't understand when the CSSPropertyBackgroundSize is added with
addProperty. You should definitively follow the same pattern as the other
cases.

> Source/WebCore/css/StylePropertySet.cpp:365
> +		   if (!foundBackgroundPositionYCSSProperty &&
shorthand.properties()[j] == CSSPropertyBackgroundSize) 

What if the background-position is : 50px and not 50px 50px.
background-shorthand-with-backgroundSize-style.html doesn't cover that case.

>
LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style-exp
ected.txt:11
> +PASS testPercentage.style.background is 'red url(dummy://test.png) no-repeat
50px 50px / 50% 75%'

Try also different background-position values.

>
LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.htm
l:6
> +<div id="test-percentage" style="background: red url(dummy://test.png)
no-repeat 50px 50px / 50% 75%"></div>

Would you mind moving around the couple background-position / background-size
so we can test better your change in parseFillShorthand. You always add them in
the end, for example you could move it before the image. The order in the
shorthand does not matter (as soon as the the position comes with the size).