Summary: | CSSStyleDeclaration.cssText should not contain extraneous whitespace in final delimiter | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Glenn Adams <glenn> | ||||||||||||||
Component: | CSS | Assignee: | Glenn Adams <glenn> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, cmarcelo, dglazkov, eric, gyuyoung.kim, kling, koivisto, macpherson, menard, mifenton, rakuco, rniwa, seikwon.kim, webkit.review.bot | ||||||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Glenn Adams
2012-08-21 13:35:34 PDT
Created attachment 159768 [details]
Patch
Comment on attachment 159768 [details] Patch Attachment 159768 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13551462 New failing tests: editing/pasteboard/data-transfer-items.html fast/events/ondrop-text-html.html editing/pasteboard/onpaste-text-html.html Created attachment 159776 [details]
Archive of layout-test-results from gce-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 159768 [details] Patch Attachment 159768 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13564021 Comment on attachment 159768 [details]
Patch
remove review request pending fix for cr-linux and win fails
Created attachment 159796 [details]
Patch
Comment on attachment 159796 [details] Patch Attachment 159796 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13567069 New failing tests: fast/events/ondrop-text-html.html editing/pasteboard/onpaste-text-html.html Created attachment 159833 [details]
Archive of layout-test-results from gce-cr-linux-07
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 160070 [details]
Patch
Comment on attachment 160070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160070&action=review > Source/WebCore/css/CSSStyleSheet.cpp:305 > + String text = selector; > + text += " { "; > + text += style; > + if (!style.isEmpty()) > + text += " "; > + text += "}"; > + insertRule(text, index, ec); Although the existing code uses it a lot, using String += is very inefficient. For new code we try to use StringBuilder. Comment on attachment 160070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160070&action=review > Although the existing code uses it a lot, using String += is very inefficient. For new code we try to use StringBuilder. +1 to that! Please use preferably the string operators or StringBuilder. String::append() and String::operator+=() are very inefficient for adding several strings. > Source/WebCore/css/StylePropertySet.cpp:669 > + result.append(" "); This should be result.append(' '); > Source/WebCore/css/StylePropertySet.cpp:827 > + result.append(" "); ditto. > Source/WebCore/css/StylePropertySet.cpp:832 > + result.append(";"); ditto. > Source/WebCore/css/StylePropertySet.cpp:841 > + result.append(" "); ditto. > Source/WebCore/css/StylePropertySet.cpp:852 > + result.append(";"); ditto. > Source/WebCore/css/StylePropertySet.cpp:856 > + result.append(" "); ditto. > Source/WebCore/css/StylePropertySet.cpp:861 > + result.append(" "); ditto. > Source/WebCore/css/StylePropertySet.cpp:869 > + result.append(" "); ditto. > Source/WebCore/css/StylePropertySet.cpp:880 > + result.append(";"); ditto. > Source/WebCore/css/StylePropertySet.cpp:884 > + result.append(" "); ditto. > Source/WebCore/css/StylePropertySet.cpp:889 > + result.append(" "); ditto. Comment on attachment 160070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160070&action=review Some more nitpick :) > Source/WebCore/css/CSSFontFaceRule.cpp:57 > + result += " "; Indent is wrong here. > Source/WebCore/css/CSSPageRule.cpp:88 > + result += " "; Indent. > Source/WebCore/css/CSSProperty.cpp:56 > + return cssName() + ": " + m_value->cssText() + (isImportant() ? " !important" : "") + ";"; ";" -> ';' > Source/WebCore/css/CSSStyleRule.cpp:121 > + result += " "; Indent. > Source/WebCore/css/WebKitCSSKeyframeRule.cpp:86 > + result += " "; Indent. Comment on attachment 160070 [details]
Patch
remove cq? pending minor fixes per comments
Created attachment 160368 [details]
Patch
Comment on attachment 160368 [details]
Patch
update to use StringBuilder per comments; fix indents;
Comment on attachment 160368 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160368&action=review Simon had already reviewed this. The strings fix looks good to me, so let's r+ again. Thank you for updating the patch for my comments after Simon's review. > Source/WebCore/css/CSSProperty.cpp:65 > - return cssName() + ": " + m_value->cssText() + (isImportant() ? " !important" : "") + "; "; > + StringBuilder result; > + result.append(cssName()); > + result.appendLiteral(": "); > + result.append(m_value->cssText()); > + if (isImportant()) > + result.appendLiteral(" !important"); > + result.append(';'); > + return result.toString(); Using "operator+" was actually ok here. The StringOperators do a pretty good job. Just a note though, no problem with using StringBuilder here. Comment on attachment 160368 [details] Patch Clearing flags on attachment: 160368 Committed r126656: <http://trac.webkit.org/changeset/126656> All reviewed patches have been landed. Closing bug. retroactively adding WebExposed keyword, since this patch affects the values returned from CSSStyleDeclaration.cssText; authors who previously relied upon WK's insertion of an extraneous space will need to account for this space no longer appearing after a final (semicolon) delimiter This has caused bug 102435. |