Bug 94633

Summary: CSSStyleDeclaration.cssText should not contain extraneous whitespace in final delimiter
Product: WebKit Reporter: Glenn Adams <glenn>
Component: CSSAssignee: 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 Flags
Patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-04
none
Patch
none
Archive of layout-test-results from gce-cr-linux-07
none
Patch
none
Patch none

Description Glenn Adams 2012-08-21 13:35:34 PDT
According to newly drafted text in CSSOM [1] for serializing CSS style declarations block (i.e., CSSStyleDeclaration), no whitespace should follow the final SEMICOLON in a non-empty value returned by the cssText getter. At present, other common browsers (FF, IE, Opera) either follow this behavior or have adopted it for an upcoming revision (Opera) [2]. However, WK is returning an extra SPACE after the SEMICOLON at the end of non-empty cssText. An incoming CSSWG test for this behavior can be found at [3].

[1] http://dvcs.w3.org/hg/csswg/raw-file/tip/cssom/Overview.html#serialize-a-css-declaration-block
[2] http://lists.w3.org/Archives/Public/www-style/2012Jul/0675.html
[3] http://hg.csswg.org/test/raw-file/tip/contributors/gadams/incoming/cssom/cssstyledeclaration-csstext-final-delimiter.xht
Comment 1 Glenn Adams 2012-08-21 14:13:19 PDT
Created attachment 159768 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-21 15:11:38 PDT
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
Comment 3 WebKit Review Bot 2012-08-21 15:11:44 PDT
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 4 Build Bot 2012-08-21 15:33:49 PDT
Comment on attachment 159768 [details]
Patch

Attachment 159768 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13564021
Comment 5 Glenn Adams 2012-08-21 15:41:36 PDT
Comment on attachment 159768 [details]
Patch

remove review request pending fix for cr-linux and win fails
Comment 6 Glenn Adams 2012-08-21 16:31:46 PDT
Created attachment 159796 [details]
Patch
Comment 7 WebKit Review Bot 2012-08-21 18:19:27 PDT
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
Comment 8 WebKit Review Bot 2012-08-21 18:19:32 PDT
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
Comment 9 Glenn Adams 2012-08-22 19:36:36 PDT
Created attachment 160070 [details]
Patch
Comment 10 Simon Fraser (smfr) 2012-08-23 20:57:33 PDT
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 11 Benjamin Poulain 2012-08-23 23:28:42 PDT
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 12 Benjamin Poulain 2012-08-24 00:20:32 PDT
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 13 Glenn Adams 2012-08-24 00:33:26 PDT
Comment on attachment 160070 [details]
Patch

remove cq? pending minor fixes per comments
Comment 14 Glenn Adams 2012-08-24 02:52:27 PDT
Created attachment 160368 [details]
Patch
Comment 15 Glenn Adams 2012-08-24 05:55:16 PDT
Comment on attachment 160368 [details]
Patch

update to use StringBuilder per comments; fix indents;
Comment 16 Benjamin Poulain 2012-08-24 16:44:07 PDT
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 17 WebKit Review Bot 2012-08-24 16:58:23 PDT
Comment on attachment 160368 [details]
Patch

Clearing flags on attachment: 160368

Committed r126656: <http://trac.webkit.org/changeset/126656>
Comment 18 WebKit Review Bot 2012-08-24 16:58:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Glenn Adams 2012-09-02 21:57:33 PDT
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
Comment 20 Alexey Proskuryakov 2012-11-16 11:05:53 PST
This has caused bug 102435.