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

[bugzilla-daemon at webkit.org]

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


Daniel Bates <dbates at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com




--- Comment #5 from Daniel Bates <dbates at webkit.org>  2011-03-06 16:15:47 PST ---
(In reply to comment #4)
> (From update of attachment 84366 [details])
> 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.

Will add test cases for this.

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

I didn't make this change since it would add an additional function call compared to the logic proposed in the patch. Notice CSSMutableStyleDeclaration::getPropertyValue() calls CSSMutableStyleDeclaration::getPropertyCSSValue() and then CSSValue::cssText(). And the patch proposes calling CSSMutableStyleDeclaration::getPropertyCSSValue() directly.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:270
> > +    if (!horizontalValue || !verticalValue)
> > +        return String();
> 
> Is this correct? The test case doesn't cover this.

This is correct and I will include test cases for this. Moreover, we can strengthen this to be:

    if (!horizontalValue)
        return String();
    ASSERT(verticalValue); // By <http://www.w3.org/TR/CSS21/tables.html#separated-borders>.

> 
> > 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);
> 

Will change before landing.

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

I suggest we look into this as part of a separate bug. I filed bug #55851 for this issue.

> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.h:157
> > +    String borderSpacingValue(const int* properties) const;
> 
> I think const int properties[2] would be better and clearer here.

Will change before landing.

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