Summary: | style.borderSpacing always returns empty string | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | Tables | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bdakin, darin, hyatt, tonikitoo, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Daniel Bates
2011-02-19 19:49:36 PST
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> |