Make sure <url>s are being serialized according to spec

Make sure <url>s are being serialized according to spec

Chrome doesn't serialize string as is stated in the spec here:
https://drafts.csswg.org/cssom/#serialize-a-url. Each string passed to
a URL is supposed to be in quotes. As opposed to what we do currently.
Gecko on Firefox does this right.

BUG=534674
Committed: https://crrev.com/3475df106fbbf3d5a686ff166be869077b8a16b3
Cr-Commit-Position: refs/heads/master@{#351033}

[nainar, 2015-09-24 04:21:38]

nainar@chromium.org changed reviewers:
+ timloh@chromium.org

[nainar, 2015-09-24 04:21:38]

Tim,

PTAL?

Thanks!

[Timothy Loh, 2015-09-24 04:45:32]

On 2015/09/24 04:21:38, nainar wrote:
> Tim,
> 
> PTAL?
> 
> Thanks!

OK (I didn't look at the test changes yet), but we should also fix up
CSSPrimitiveValue::customCSSText in the URI case as well as
CSSSVGDocumentValue::customCSSText. Also lets make a serializeURL function
alongside serializeString and whatnot.

[Timothy Loh, 2015-09-24 04:47:17]

On 2015/09/24 04:45:32, Timothy Loh wrote:
> On 2015/09/24 04:21:38, nainar wrote:
> > Tim,
> > 
> > PTAL?
> > 
> > Thanks!
> 
> OK (I didn't look at the test changes yet), but we should also fix up
> CSSPrimitiveValue::customCSSText in the URI case as well as
> CSSSVGDocumentValue::customCSSText. Also lets make a serializeURL function
> alongside serializeString and whatnot.

Also vaguely relatedly, CSSPrimitiveValue should serialize custom idents
properly (so they shouldn't quote), but maybe we want font-family serialize as a
string if there are spaces and a custom-ident otherwise.

[nainar, 2015-09-24 07:41:15]

Tim,

Made the changes you asked for in your first comment, PTAL?

[Timothy Loh, 2015-09-24 07:53:21]

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css3/filters/filter-property-expected.txt
(right):

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css3/filters/filter-property-expected.txt:8: FAIL
declaration.getPropertyValue('-webkit-filter') should be url('#a') url('#b').
Was url("#a") url("#b").
These tests should be updated so this we pass...

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/cursor-parsing.html (right):

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/cursor-parsing.html:51:
testCursorRule('url(file:///foo.png), crosshair', 'url("file:///foo.png"),
crosshair');
just add quotes in the first argument instead of adding another argument

[nainar, 2015-09-25 04:10:19]

Tim, 

PTAL? I am currently looking into the test failing on Mac, Windows. 

Thanks!

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css3/filters/filter-property-expected.txt
(right):

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css3/filters/filter-property-expected.txt:8: FAIL
declaration.getPropertyValue('-webkit-filter') should be url('#a') url('#b').
Was url("#a") url("#b").
Done.

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/css/cursor-parsing.html (right):

https://codereview.chromium.org/1363233003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/cursor-parsing.html:51:
testCursorRule('url(file:///foo.png), crosshair', 'url("file:///foo.png"),
crosshair');
Done.

[nainar, 2015-09-25 04:27:33]

Patch 5 has a fix for extensions-panel.html.

[Timothy Loh, 2015-09-25 05:25:23]

Code change is fine. Layout test changes need some tweaks to make the tests more
readable.

- Don't escape single quotes in double-quoted strings, don't escape double
quotes in single-quoted strings, don't escape quotes in regexes
- Change shouldBe to shouldBeEqualToString so you don't need as many quotes

https://codereview.chromium.org/1363233003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/css/svg-attribute-parser-mode-expected.txt
(right):

https://codereview.chromium.org/1363233003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/svg-attribute-parser-mode-expected.txt:18:
FAIL document.defaultView.getComputedStyle(rect, null).fill should be
url(#reference). Was url("#reference").
fail?

https://codereview.chromium.org/1363233003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/custom/marker-getPropertyValue-expected.txt
(right):

https://codereview.chromium.org/1363233003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/marker-getPropertyValue-expected.txt:1:
url("#test") FAILED (actual: 'url("#test")' expected: 'url(#test)')
fail?

[nainar, 2015-09-25 09:29:02]

Made the changes you suggested, PTAL?

https://codereview.chromium.org/1363233003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/css/svg-attribute-parser-mode-expected.txt
(right):

https://codereview.chromium.org/1363233003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/css/svg-attribute-parser-mode-expected.txt:18:
FAIL document.defaultView.getComputedStyle(rect, null).fill should be
url(#reference). Was url("#reference").
Done.

[Timothy Loh, 2015-09-25 14:22:05]

should be the last round of comments

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js:214:
if (/url\("([^\)]*)"\)/g.test(matches[i]))
Are you sure it doesn't work if we just update the regex instead of adding this
check?

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/responsive/resources/responsive-test.js
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/responsive/resources/responsive-test.js:153:
if (/url\(([^\)]*)\)/g.test(expectation.is) &&
!/url\("([^\)]*)"\)/g.test(expectation.is)) {
just update fill-responsive.html instead?

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/getComputedStyle-relativeURL.html
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/getComputedStyle-relativeURL.html:10:
var expected = "-webkit-cross-fade(url(\"" +
location.href.replace('getComputedStyle-relativeURL.html', 'images1.jpg') +
"\"), url(\"" + location.href.replace('getComputedStyle-relativeURL.html',
'images2.jpg') + "\"), 0.5)";
single quotes for this file too

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/script-tests/image-set-parsing.js
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/script-tests/image-set-parsing.js:14:
result = div.style[property].replace(/url\("[^#]*#/g, "url(\"#");
single quotes for strings with double quotes in this file too...

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/styles-2/inject-stylesheet.html
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/styles-2/inject-stylesheet.html:14:
var styleSheet = "#main { color: red; -webkit-border-image:
url(data:image/png;base64," +
decodeURIComponent("iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAQAAAC1+jfqAAAAiElEQVR42r2RsQrDMAxEBRdl8SDcX8lQPGg1GBI6lvz/h7QyRRXV0qUULwfvwZ1tenw5PxToRPWMC52eA9+WDnlh3HFQ/xBQl86NFYJqeGflkiogrOvVlIFhqURFVho3x1moGAa3deMs+LS30CAhBN5nNxeT5hbJ1zwmji2k+aF6NENIPf/hs54f0sZFUVAMigAAAABJRU5ErkJggg==")
+ ") }  #iframeBody { background: red }";
what does decodeURIComponent do here?

[nainar, 2015-09-28 01:19:18]

Tim, 

PTAL?

Thanks!

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js:214:
if (/url\("([^\)]*)"\)/g.test(matches[i]))
Done.

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/responsive/resources/responsive-test.js
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/responsive/resources/responsive-test.js:153:
if (/url\(([^\)]*)\)/g.test(expectation.is) &&
!/url\("([^\)]*)"\)/g.test(expectation.is)) {
Done.

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/getComputedStyle-relativeURL.html
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/getComputedStyle-relativeURL.html:10:
var expected = "-webkit-cross-fade(url(\"" +
location.href.replace('getComputedStyle-relativeURL.html', 'images1.jpg') +
"\"), url(\"" + location.href.replace('getComputedStyle-relativeURL.html',
'images2.jpg') + "\"), 0.5)";
Done.

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/css/script-tests/image-set-parsing.js
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/css/script-tests/image-set-parsing.js:14:
result = div.style[property].replace(/url\("[^#]*#/g, "url(\"#");
Done.

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector/elements/styles-2/inject-stylesheet.html
(right):

https://codereview.chromium.org/1363233003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector/elements/styles-2/inject-stylesheet.html:14:
var styleSheet = "#main { color: red; -webkit-border-image:
url(data:image/png;base64," +
decodeURIComponent("iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAQAAAC1+jfqAAAAiElEQVR42r2RsQrDMAxEBRdl8SDcX8lQPGg1GBI6lvz/h7QyRRXV0qUULwfvwZ1tenw5PxToRPWMC52eA9+WDnlh3HFQ/xBQl86NFYJqeGflkiogrOvVlIFhqURFVho3x1moGAa3deMs+LS30CAhBN5nNxeT5hbJ1zwmji2k+aF6NENIPf/hs54f0sZFUVAMigAAAABJRU5ErkJggg==")
+ ") }  #iframeBody { background: red }";
Done.

[nainar, 2015-09-28 04:18:15]

Tim,

Removed decodeURIComponent, PTAL?

Thanks!

[Timothy Loh, 2015-09-28 06:18:13]

lgtm

https://codereview.chromium.org/1363233003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1363233003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js:210:
var matches = value.match(/url\([^\)]*\)/g);
should update this too

https://codereview.chromium.org/1363233003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js:213:
var url = /url\(("|')([^\)]*)('|")\)/g.exec(matches[i])[2];
does this work with just "? Otherwise it'd be better to at least use ['"]

[nainar, 2015-09-28 07:22:54]

Have fixed the interop tests please take a look?

https://codereview.chromium.org/1363233003/diff/180001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/1363233003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js:210:
var matches = value.match(/url\([^\)]*\)/g);
Done.

https://codereview.chromium.org/1363233003/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/animations/interpolation/resources/interpolation-test.js:213:
var url = /url\(("|')([^\)]*)('|")\)/g.exec(matches[i])[2];
Done.

[nainar, 2015-09-28 07:57:13]

The CQ bit was checked by nainar@chromium.org

[nainar, 2015-09-28 07:57:13]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1363233003/#ps200001
(title: "Fix interpolation tests")

[commit-bot: I haz the power, 2015-09-28 07:58:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1363233003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1363233003/200001

[commit-bot: I haz the power, 2015-09-28 08:15:27]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-09-28 08:16:09]

Patchset 11 (id:??) landed as
https://crrev.com/3475df106fbbf3d5a686ff166be869077b8a16b3
Cr-Commit-Position: refs/heads/master@{#351033}