[webkit-reviews] review granted: [Bug 54816] style.borderSpacing always returns empty string : [Attachment 84366] Patch and layout test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 54816: style.borderSpacing always returns empty string
https://bugs.webkit.org/show_bug.cgi?id=54816

Attachment 84366: Patch and layout test
https://bugs.webkit.org/attachment.cgi?id=84366&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.