[webkit-reviews] review denied: [Bug 86739] background-size specified by a single value in background shorthand fails to parse : [Attachment 142539] ProposedPatch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Joe Thomas <joethomas at motorola.com>'s
request for review:
Bug 86739: background-size specified by a single value in background shorthand
fails to parse
https://bugs.webkit.org/show_bug.cgi?id=86739

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=142539&action=review


> Source/WebCore/css/CSSParser.cpp:3385
> +		       // If we're not parsing a shorthand then we are invalid.


I would remove this comment.  It's just restating what the code is doing.

> Source/WebCore/css/CSSParser.cpp:3394
>      } else if (!parsedValue2 && propId == CSSPropertyWebkitBackgroundSize) {

>	   // For backwards compatibility we set the second value to the first
if it is omitted.

It looks like the comment here is trying to handle this case.  Should we be
hitting this code instead?

>
LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.htm
l:-7
> -<div id="test-pixels" style="background: red url(dummy://test.png) repeat
top left / 100px 200px border-box padding-box"></div>

This test case appears to have changed to border-box border-box.  Was that
intentional?

>
LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.htm
l:-8
> -<div id="test-auto" style="background: red url(dummy://test.png) repeat 50%
/ auto auto border-box content-box"></div>

This too.  FWIW, it was hard to tell which test cases you added from the diff. 
If you wanted to change the test, you could have done that in a separate
refactor change before this bug fix.