[Webkit-unassigned] [Bug 54816] style.borderSpacing always returns empty string

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54816


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84366|review?                     |review+
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2011-03-02 10:18:26 PST ---
(From update of attachment 84366)
View in context: https://bugs.webkit.org/attachment.cgi?id=84366&action=review

I’d also suggest having the test case cover computed style too, even though that code probably already handles this correctly.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:267
> +    RefPtr<CSSValue> horizontalValue = getPropertyCSSValue(properties[0]);
> +    RefPtr<CSSValue> verticalValue = getPropertyCSSValue(properties[1]);

It’s probably better to just call getPropertyValue instead of calling getPropertyCSSValue and then separately calling cssText.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:270
> +    if (!horizontalValue || !verticalValue)
> +        return String();

Is this correct? The test case doesn't cover this.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:274
> +    if (result != verticalValue->cssText())
> +        result += " " + verticalValue->cssText();

Appending to strings is inefficient. The code in this file is consistently less efficient than it can be. The efficient way to combine two strings into a new string with a character between them is to use the makeString function from the StringConcatenate.h header. So assuming that horizontalValue and verticalValue are strings, not CSSValue objects, the code would be:

    if (horizontalValue == verticalValue)
        return horizontalValue;
    return makeString(horizontalValue, ' ', verticalValue);

The rest of the file could also be fixed so it does less of the extremely-inefficient string concatenation. For building up a string a piece at a time, the best approach is to probably use either StringBuilder or Vector<UChar>.

> Source/WebCore/css/CSSMutableStyleDeclaration.h:157
> +    String borderSpacingValue(const int* properties) const;

I think const int properties[2] would be better and clearer here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.