Bug 27577 - [CSS3 Backgrounds and Borders] Add background-size to the background shorthand
: [CSS3 Backgrounds and Borders] Add background-size to the background shorthand
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: InRadar
:
: 27569
  Show dependency treegraph
 
Reported: 2009-07-22 15:48 PST by
Modified: 2012-09-27 10:42 PST (History)


Attachments
ProposedPatch (14.97 KB, patch)
2012-05-01 12:45 PST, Joe Thomas
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (5.67 MB, application/zip)
2012-05-01 13:25 PST, WebKit Review Bot
no flags Details
Patch-Updated (11.42 KB, patch)
2012-05-01 13:43 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-Updated (11.28 KB, patch)
2012-05-01 14:28 PST, Joe Thomas
alexis: review-
Review Patch | Details | Formatted Diff | Diff
Patch-updated (18.14 KB, patch)
2012-05-03 14:54 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-Updated (68.35 KB, patch)
2012-05-03 17:20 PST, Joe Thomas
alexis: review-
alexis: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch-with-More-testCases (70.86 KB, patch)
2012-05-04 07:57 PST, Joe Thomas
alexis: review-
alexis: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
patch (69.92 KB, patch)
2012-05-07 14:13 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-Updated (69.72 KB, patch)
2012-05-08 13:55 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch (69.76 KB, patch)
2012-05-08 17:00 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-22 15:48:26 PST
background-size is supposed to be part of the background shorthand.  It appears after background-position followed by a '/'.
------- Comment #1 From 2011-09-30 13:55:46 PST -------
Note bug 8464.
------- Comment #2 From 2011-09-30 13:57:02 PST -------
And is this the bug tracking the fact that "background" doesn't reset the background-size?  This is causing people to write CSS like this:

  .foo {
    background-size: contain;
    height: 200px;
    background: url(something) no-repeat center center;
  }

that doesn't work in spec-compliant browsers but works in WebKit...
------- Comment #3 From 2011-09-30 16:18:31 PST -------
<rdar://problem/10218264>
------- Comment #4 From 2012-05-01 12:45:11 PST -------
Created an attachment (id=139659) [details]
ProposedPatch
------- Comment #5 From 2012-05-01 12:52:35 PST -------
*** Bug 8464 has been marked as a duplicate of this bug. ***
------- Comment #6 From 2012-05-01 13:17:33 PST -------
(From update of attachment 139659 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139659&action=review

> LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:36
> +The background property is a shorthand property for setting most background properties at the same place in the style sheet. 
> +The number of comma-separated items defines the number of background layers.
> +Given a valid declaraion, for each layer the shorthand first sets the corresponding layer of each of background-image,
> +background-position, background-size, background-repeat, background-origin, background-clip and background-attachment
> +to that propertys initial value, then assigns any explicit values specified for this layer in the declaration. 
> +Finally background-color is set to the specified color, if any, else set to its initial value.
> +If one <box> value is present then it sets both background-origin and background-clip to that value. 
> +If two values are present, then the first sets background-origin and the second background-clip. 

Could we have just used manually sized divs instead of this wall of text?
------- Comment #7 From 2012-05-01 13:25:38 PST -------
(From update of attachment 139659 [details])
Attachment 139659 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12588649

New failing tests:
fast/inspector-support/style.html
inspector/styles/lazy-computed-style.html
inspector/styles/styles-new-API.html
inspector/styles/styles-computed-trace.html
------- Comment #8 From 2012-05-01 13:25:45 PST -------
Created an attachment (id=139665) [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #9 From 2012-05-01 13:43:41 PST -------
Created an attachment (id=139667) [details]
Patch-Updated

Fixed Layout test failures.
Incorporated Eric's review comments
------- Comment #10 From 2012-05-01 13:44:50 PST -------
(In reply to comment #6)
> (From update of attachment 139659 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139659&action=review
> 
> > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:36
> 
> Could we have just used manually sized divs instead of this wall of text?


Thanks for the review. Patch looks much better now.
------- Comment #11 From 2012-05-01 13:52:43 PST -------
(From update of attachment 139667 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139667&action=review

Do you know who has recently been working on the shorthand stuff?  It seems they should be CC'd if they aren't already. I know we've had several patches in this area as of late.

> LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:12
> +#size-cover
> +{
> +background: url('resources/flower.jpg') center / cover no-repeat;
> +}

It doesn't matter, but by putting these selectors all on one line, you could cut this file size in half w/o hurting readability. :)

#size-cover { background: url('resources/flower.jpg') center / cover no-repeat; }

It's also not clear to me if you don't just want to inline these on the div elements themselves. :)
------- Comment #12 From 2012-05-01 14:28:22 PST -------
Created an attachment (id=139681) [details]
Patch-Updated
------- Comment #13 From 2012-05-01 14:32:23 PST -------
(In reply to comment #11)
> (From update of attachment 139667 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139667&action=review
> 
> Do you know who has recently been working on the shorthand stuff?  It seems they should be CC'd if they aren't already. I know we've had several patches in this area as of late.

CCed Antti Koivisto, Kling & rniwa, whom I could locate from recent patches.
> 
> > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand.html:12
> > +#size-cover
> > +{
> > +background: url('resources/flower.jpg') center / cover no-repeat;
> > +}
> 
> It doesn't matter, but by putting these selectors all on one line, you could cut this file size in half w/o hurting readability. :)
> 
> #size-cover { background: url('resources/flower.jpg') center / cover no-repeat; }
> 
> It's also not clear to me if you don't just want to inline these on the div elements themselves. :)

Done.
------- Comment #14 From 2012-05-02 04:14:45 PST -------
(From update of attachment 139681 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139681&action=review

In the same way you also should patch StylePropertySet::getPropertyValue, StylePropertyShorthand::backgroundShorthand() and CSSComputedStyleDeclaration and it would be nice to add a test case to cover these cases. You also want to test other values than initial (well maybe you did but it seemed there was a problem when you uploaded)

> LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand-expected.html:16
> +</html>

It doesn't seem to be the right file.
------- Comment #15 From 2012-05-02 04:50:33 PST -------
(In reply to comment #14)
> (From update of attachment 139681 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139681&action=review
> 
Thanks for the review.

> In the same way you also should patch StylePropertySet::getPropertyValue, StylePropertyShorthand::backgroundShorthand() and CSSComputedStyleDeclaration 
and it would be nice to add a test case to cover these cases. 

I will cover these cases.

You also want to test other values than initial (well maybe you did but it seemed there was a problem when you uploaded)
> 
Can you please let me know what is the problem that you are seeing? It looks fine to me.

> > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand-expected.html:16
> > +</html>
> 
> It doesn't seem to be the right file.

This is a new reftest added by me. Do you mean to say that the path is wrong?
------- Comment #16 From 2012-05-02 05:14:01 PST -------
(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 139681 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=139681&action=review
> > 
> Thanks for the review.
> 
> > In the same way you also should patch StylePropertySet::getPropertyValue, StylePropertyShorthand::backgroundShorthand() and CSSComputedStyleDeclaration 
> and it would be nice to add a test case to cover these cases. 
> 
> I will cover these cases.
> 
> You also want to test other values than initial (well maybe you did but it seemed there was a problem when you uploaded)
> > 
> Can you please let me know what is the problem that you are seeing? It looks fine to me.
> 
> > > LayoutTests/fast/backgrounds/size/backgroundSize-in-background-shorthand-expected.html:16
> > > +</html>
> > 
> > It doesn't seem to be the right file.
> 
> This is a new reftest added by me. Do you mean to say that the path is wrong?

Maybe you don't need a reftest. I simple JS based test like in fast/css/getComputedStyle/ is good enough. I feel this :

shouldBe("computedStyle.getPropertyValue('border-width')", "'10px 5px 4px 3px'");

is more easy to read and is more what you want to test.
------- Comment #17 From 2012-05-03 14:54:03 PST -------
Created an attachment (id=140100) [details]
Patch-updated

Added the background-size property to StylePropertySet::getPropertyValue & StylePropertyShorthand::backgroundShorthand()

But I haven't added background-size to background shorthand property in CSSComputedStyleDeclaration. Tried to add / seperator between background-position and background-size in CSSComputedStyleDeclaration::getCSSPropertyValuesForShorthandProperties with 

 if (shorthand.properties()[i] == CSSPropertyBackgroundSize)
    list->append(cssValuePool().createValue("/", CSSPrimitiveValue::CSS_STRING));

This adds the seperator into the shorthand with single quotes, like 

rgb(255, 0, 0) url(dummy://test.png) repeat scroll 50px 50px '/' 50% 25%

The single quote is getting added to the separator in CSSPrimitiveValue::customCssText() with "quoteCSSStringIfNeeded(m_value.string)". 

Can someone please suggest a way to avoid this? Is it a good idea to create a PlainTextCSSValue class derived from CSSValue and return the string alone without any modification?
------- Comment #18 From 2012-05-03 14:58:56 PST -------
(In reply to comment #17)
> Created an attachment (id=140100) [details] [details]
> Patch-updated
> 
> Added the background-size property to StylePropertySet::getPropertyValue & StylePropertyShorthand::backgroundShorthand()
> 
> But I haven't added background-size to background shorthand property in CSSComputedStyleDeclaration. Tried to add / seperator between background-position and background-size in CSSComputedStyleDeclaration::getCSSPropertyValuesForShorthandProperties with 
> 
>  if (shorthand.properties()[i] == CSSPropertyBackgroundSize)
>     list->append(cssValuePool().createValue("/", CSSPrimitiveValue::CSS_STRING));
> 
> This adds the seperator into the shorthand with single quotes, like 
> 
> rgb(255, 0, 0) url(dummy://test.png) repeat scroll 50px 50px '/' 50% 25%
> 
> The single quote is getting added to the separator in CSSPrimitiveValue::customCssText() with "quoteCSSStringIfNeeded(m_value.string)". 
> 
> Can someone please suggest a way to avoid this? Is it a good idea to create a PlainTextCSSValue class derived from CSSValue and return the string alone without any modification?

You want to look at 
static PassRefPtr<CSSValueList> getBorderRadiusShorthandValue(const RenderStyle* style, RenderView* renderView)

We add a slash there.
------- Comment #19 From 2012-05-03 17:20:56 PST -------
Created an attachment (id=140130) [details]
Patch-Updated
------- Comment #20 From 2012-05-03 17:22:47 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=140100) [details] [details] [details]
> > Patch-updated
> > 
> > 
> > Can someone please suggest a way to avoid this? Is it a good idea to create a PlainTextCSSValue class derived from CSSValue and return the string alone without any modification?
> 
> You want to look at 
> static PassRefPtr<CSSValueList> getBorderRadiusShorthandValue(const RenderStyle* style, RenderView* renderView)
> 
> We add a slash there.

Thanks very much :)
------- Comment #21 From 2012-05-04 05:41:37 PST -------
(From update of attachment 140130 [details])
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-expected.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.html: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).
------- Comment #22 From 2012-05-04 05:52:47 PST -------
(From update of attachment 140130 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140130&action=review

Also add a test with a broken background-image value : e.g. / cover cover, / 3px cover

>> 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.

Ok found it nevermind.
------- Comment #23 From 2012-05-04 07:57:12 PST -------
Created an attachment (id=140226) [details]
Patch-with-More-testCases
------- Comment #24 From 2012-05-04 08:04:13 PST -------
(In reply to comment #21)
> (From update of attachment 140130 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140130&action=review
> 
Thanks for the 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.
> 
It will drop the rule. Added testcase.

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2701
> > +    size_t count = 0;
> 
> unsigned is better, and call it i.
> 
Done

> > 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.
> 
Added testcase. backgroundPositionY will be 50%. Similar test case in LayoutTests/fast/css/getComputedStyle/getComputedStyle-background-position.html

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

Done. Added test cases for background-position with top left, center, percentage and pixels

> 
> > LayoutTests/fast/backgrounds/background-shorthand-with-backgroundSize-style.html: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).

Done.
------- Comment #25 From 2012-05-04 08:06:06 PST -------
(In reply to comment #22)
> (From update of attachment 140130 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140130&action=review
> 
> Also add a test with a broken background-image value : e.g. / cover cover, / 3px cover
> 
Done. It will drop the rule.
------- Comment #26 From 2012-05-07 11:07:30 PST -------
(From update of attachment 140226 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140226&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2708
> +    RefPtr<CSSValueList> listOnOrAfterBackgroundSize = CSSValueList::createSpaceSeparated();

The name is strange. what about backgroundSizeList.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2710
> +        RefPtr<CSSValue> value = getPropertyCSSValue(shorthand.properties()[i], DoNotUpdateLayout);

You don't need this for loop. Just call getPropertyCSSValue(CSSPropertyBackgroundSize, DoNotUpdateLayout); what else could be after the CSSPropertyBackgroundSize?
------- Comment #27 From 2012-05-07 11:10:32 PST -------
(From update of attachment 140226 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140226&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2696
> +PassRefPtr<CSSValueList> CSSComputedStyleDeclaration::getBackgroundShorthandValue(const StylePropertyShorthand& shorthand) const

Also you are making a dedicated function now we don't need to pass a list. Just call getPropertyCSSValue on the properties you need. The array existed because we were using getCSSPropertyValuesForShorthandProperties.
------- Comment #28 From 2012-05-07 14:13:55 PST -------
Created an attachment (id=140591) [details]
patch

Review comments from Alexis incorporated.
------- Comment #29 From 2012-05-07 14:20:31 PST -------
(In reply to comment #26)
> (From update of attachment 140226 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140226&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2708
> > +    RefPtr<CSSValueList> listOnOrAfterBackgroundSize = CSSValueList::createSpaceSeparated();
> 
> The name is strange. what about backgroundSizeList.
> 
I changed the names to better ones, please let me know if it is fine.
list -> backgroundShorthandList
listBeforeBackgroundSize -> list
listOnOrAfterBackgroundSize -> removed and added backgroundSizeValue

> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2710
> > +        RefPtr<CSSValue> value = getPropertyCSSValue(shorthand.properties()[i], DoNotUpdateLayout);
> 
> You don't need this for loop. Just call getPropertyCSSValue(CSSPropertyBackgroundSize, DoNotUpdateLayout); what else could be after the CSSPropertyBackgroundSize?

Currently background-clip and background-origin are not returned in getComputedStyle of background shorthand property. I am planning to add the support for them after this lands. Second 'For' loop was added so that I can add these missing properties towards the end of the array. 

I removed the second for loop from this patch. I will add background-clip and background-origin before background-position in the next patch.
------- Comment #30 From 2012-05-08 07:30:10 PST -------
(From update of attachment 140591 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140591&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2699
> +    const CSSPropertyID properties[5] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,

you can make that array static :D
------- Comment #31 From 2012-05-08 07:46:02 PST -------
(From update of attachment 140591 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140591&action=review

This change looks wrong, it's adding an extra layer of indirection to the computed CSSValue, turning this:
{ color, image, repeat, attachment, position }
Into this:
{ { color, image, repeat, attachment, position }, size }

Is this behavior change correct?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2709
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(properties); ++i) {
> +        RefPtr<CSSValue> value = getPropertyCSSValue(properties[i], DoNotUpdateLayout);
> +        list->append(value);
> +    }
> +    backgroundShorthandList->append(list);

This looks like a copy-paste of getCSSPropertyValuesForShorthandProperties(). Better to just call it.
------- Comment #32 From 2012-05-08 08:12:51 PST -------
(In reply to comment #31)
> (From update of attachment 140591 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140591&action=review
> 
> This change looks wrong, it's adding an extra layer of indirection to the computed CSSValue, turning this:
> { color, image, repeat, attachment, position }
> Into this:
> { { color, image, repeat, attachment, position }, size }
> 
> Is this behavior change correct?

The spec http://www.w3.org/TR/css3-background/ does not mention anything about the internal representation of computed CSS value. Do we have any other spec that I can refer to?

The same kind of indirection we are adding in case of border-radius shorthand value to add a '/' between horizontal and vertical radii in getBorderRadiusShorthandValue.

Please let me know if we need to avoid this. Also see comment #17.
------- Comment #33 From 2012-05-08 13:39:06 PST -------
(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 140591 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140591&action=review
> > 
> > This change looks wrong, it's adding an extra layer of indirection to the computed CSSValue, turning this:
> > { color, image, repeat, attachment, position }
> > Into this:
> > { { color, image, repeat, attachment, position }, size }
> > 
> > Is this behavior change correct?
> 
> The spec http://www.w3.org/TR/css3-background/ does not mention anything about the internal representation of computed CSS value. Do we have any other spec that I can refer to?
> 
> The same kind of indirection we are adding in case of border-radius shorthand value to add a '/' between horizontal and vertical radii in getBorderRadiusShorthandValue.
> 
> Please let me know if we need to avoid this. Also see comment #17.

It is not so important the internal representation of computed CSSValue of shorthand properties as getPropertyCSSValue of shorthand property is expected to return NULL. As per http://www.w3.org/TR/DOM-Level-2-Style/css.html, Shorthand property values can only be accessed and modified as strings using the getPropertyValue and setProperty methods.

Paraphrasing from http://www.w3.org/TR/DOM-Level-2-Style/css.html:

"getPropertyCSSValue: Used to retrieve the object representation of the value of a CSS property if it has been explicitly set within this declaration block. This method returns null if the property is a shorthand property. Shorthand property values can only be accessed and modified as strings, using the getPropertyValue and setProperty methods."
------- Comment #34 From 2012-05-08 13:55:58 PST -------
Created an attachment (id=140774) [details]
Patch-Updated

Review comments from Alexis and Kling incorporated.
------- Comment #35 From 2012-05-08 16:44:08 PST -------
(From update of attachment 140774 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140774&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2349
>          case CSSPropertyBackground: {
> -            const CSSPropertyID properties[5] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,
> -                                        CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment,
> -                                        CSSPropertyBackgroundPosition };
> -            return getCSSPropertyValuesForShorthandProperties(StylePropertyShorthand(properties, WTF_ARRAY_LENGTH(properties)));
> +
> +            return getBackgroundShorthandValue();
>          }

Should get rid of the braces and blank line here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2703
> +    RefPtr<CSSValueList> backgroundShorthandList = CSSValueList::createSlashSeparated();

I’d just call this local variable “list”. I don’t think the longer name helps readability.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2705
> +    // Now append CSSPropertyBackgroundSize value to the backgroundShorthandList.

This is not a helpful comment. It says the same thing as the next line.

> Source/WebCore/css/CSSParser.cpp:2313
> +        return parseFillShorthand(propId, properties, 8, important);

It would be better to not have the number 8 hard-coded here. We should use WTF_ARRAY_LENGTH.
------- Comment #36 From 2012-05-08 17:00:00 PST -------
Created an attachment (id=140821) [details]
Patch

Incorporated Darin's review comments.
------- Comment #37 From 2012-05-08 17:03:40 PST -------
(In reply to comment #35)
> (From update of attachment 140774 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140774&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2349
> >          case CSSPropertyBackground: {
> > -            const CSSPropertyID properties[5] = { CSSPropertyBackgroundColor, CSSPropertyBackgroundImage,
> > -                                        CSSPropertyBackgroundRepeat, CSSPropertyBackgroundAttachment,
> > -                                        CSSPropertyBackgroundPosition };
> > -            return getCSSPropertyValuesForShorthandProperties(StylePropertyShorthand(properties, WTF_ARRAY_LENGTH(properties)));
> > +
> > +            return getBackgroundShorthandValue();
> >          }
> 
> Should get rid of the braces and blank line here.
Done
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2703
> > +    RefPtr<CSSValueList> backgroundShorthandList = CSSValueList::createSlashSeparated();
> 
> I’d just call this local variable “list”. I don’t think the longer name helps readability.
> 
Changed.
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2705
> > +    // Now append CSSPropertyBackgroundSize value to the backgroundShorthandList.
> 
> This is not a helpful comment. It says the same thing as the next line.
> 
Removed
> > Source/WebCore/css/CSSParser.cpp:2313
> > +        return parseFillShorthand(propId, properties, 8, important);
> 
> It would be better to not have the number 8 hard-coded here. We should use WTF_ARRAY_LENGTH.
Done
------- Comment #38 From 2012-05-10 07:02:48 PST -------
(From update of attachment 140821 [details])
r=me. It is very unlikely that code would rely on the internal representation of the shorthand. In addition, the support of the shorthand property in getComputedStyle was added by myself 3-4 months ago (before NULL was returned) so the behavior change happening in this patch would mean that the breakage would be for people relying on this internal representation (which is not recommended by the spec) in the last 3-4 months. Only the last version of Chrome (18) ships the support of the background shorthand in general so it is fine. I agree with kling that the internal representation is a bit cumbersome, and it could be fixed in another patch by supporting creating CSSValueList with mixed separators (without increasing the number of items of the list) which is probably possible today. But what this patch fix and is very welcome is the support of the background-size in the shorthand which is a long standing bug (Opera and FF are supporting this feature).
------- Comment #39 From 2012-05-10 08:09:28 PST -------
(From update of attachment 140821 [details])
Clearing flags on attachment: 140821

Committed r116645: <http://trac.webkit.org/changeset/116645>
------- Comment #40 From 2012-05-10 08:09:49 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #41 From 2012-05-21 13:14:39 PST -------
This caused a regression, bug 86880.
------- Comment #42 From 2012-05-21 13:49:47 PST -------
As I commented on the other bug, we should make sure shorthand notations we generate can be parsed properly by old UAs like IE6. If not, we have to use a different function for editing.
------- Comment #43 From 2012-05-21 13:57:47 PST -------
(In reply to comment #42)
> As I commented on the other bug, we should make sure shorthand notations we generate can be parsed properly by old UAs like IE6. If not, we have to use a different function for editing.

It's not the problem on #86880, it simply rely on undefined behavior.
------- Comment #44 From 2012-09-27 10:42:09 PST -------
This is questioned again in bug 97761