WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
54816
style.borderSpacing always returns empty string
https://bugs.webkit.org/show_bug.cgi?id=54816
Summary
style.borderSpacing always returns empty string
Daniel Bates
Reported
2011-02-19 19:49:36 PST
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".
Attachments
Example
(480 bytes, text/html)
2011-02-19 19:49 PST
,
Daniel Bates
no flags
Details
Patch and layout test
(9.13 KB, patch)
2011-03-01 21:52 PST
,
Daniel Bates
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Daniel Bates
Comment 1
2011-03-01 21:52:08 PST
Created
attachment 84366
[details]
Patch and layout test
WebKit Review Bot
Comment 2
2011-03-01 21:54:39 PST
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.
Daniel Bates
Comment 3
2011-03-01 21:57:29 PST
(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.
Darin Adler
Comment 4
2011-03-02 10:18:26 PST
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.
Daniel Bates
Comment 5
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.
Daniel Bates
Comment 6
2011-03-06 16:23:04 PST
Committed
r80439
: <
http://trac.webkit.org/changeset/80439
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug