getComputedStyle should return shorthands properties with the minimum number of sides possible.

[Alexis Menard (darktears)]

getComputedStyle should return shorthands property with the minimum number of sides possible.

[Alexis Menard (darktears)]

Created attachment 120873 [details]
Patch

[Tony Chang]

Comment on attachment 120873 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120873&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2419
> +PassRefPtr<CSSValueList> CSSComputedStyleDeclaration::getCSSProperty4ValuesForShorthandProperties(const int* properties) const
> +{
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> +    // Assume the properties are in the usual order top, right, bottom, left.

I would try to make the name more clear that this function is for properties that apply to each direction/side.  For example, getCSSPropertyValuesForSidesShorthand or something.

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt:24
> +PASS computedStyle.getPropertyCSSValue('border-width').cssText is '10px 0px 0px'

In a follow up patch, maybe we should change 0px to 0 since the unit identifier is optional on zero length units. http://www.w3.org/TR/css3-values/#length-value

[Alexis Menard (darktears)]

(In reply to comment #2)
> (From update of attachment 120873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120873&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2419
> > +PassRefPtr<CSSValueList> CSSComputedStyleDeclaration::getCSSProperty4ValuesForShorthandProperties(const int* properties) const
> > +{
> > +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> > +    // Assume the properties are in the usual order top, right, bottom, left.
> 
> I would try to make the name more clear that this function is for properties that apply to each direction/side.  For example, getCSSPropertyValuesForSidesShorthand or something.

Yes I ran out of imagination on that one :(. I like your suggestion.

> 
> > LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-width-expected.txt:24
> > +PASS computedStyle.getPropertyCSSValue('border-width').cssText is '10px 0px 0px'
> 
> In a follow up patch, maybe we should change 0px to 0 since the unit identifier is optional on zero length units. http://www.w3.org/TR/css3-values/#length-value

I'll look at that one.

[Alexis Menard (darktears)]

Created attachment 120957 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 120957 [details]
Patch for landing

Clearing flags on attachment: 120957

Committed r103953: <http://trac.webkit.org/changeset/103953>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.