Created attachment 83087 [details] Example Consider the following HTML snippet: <table id="table" style="border-spacing: 4px 5px"> <tr><td>Cell 1</td></tr> </table> Then document.getElementById("table").borderSpacing should be equal to the string "4px 5px". But document.getElementById("table").borderSpacing == "" (as of r79140). Notice that both Mac Firefox 3.6.13 and IE 8 return the string "4px 5px".
Created attachment 84366 [details] Patch and layout test
Attachment 84366 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSMutableStyleDeclaration.cpp:124: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #2) > [...] > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:124: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] We should fix the style for the whole switch block. I suggest we do this in a separate bug so as to demarcate the actual changes for this bug from the style correction for the switch block.
Comment on attachment 84366 [details] Patch and layout test 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.
(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.
Committed r80439: <http://trac.webkit.org/changeset/80439>