Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(114)

Issue 1778743003: Make <custom-ident> not insert quotes (Closed)

Created:
4 years, 9 months ago by rwlbuis
Modified:
4 years, 9 months ago
Reviewers:
Timothy Loh
CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make <custom-ident> not insert quotes Before this patch, <custom-ident> serializing used quoteCSSStringIfNeeded to serialize, which can end up inserting quotes. However <custom-ident> are identifiers [1] and should serialize as such, meaning no quotes [2]. This patches fixes that by calling serializeIdentifier. Since the font family property relied on old CSSCustomIdentValue behavior, this patch adds a CSSFontFamilyValue class to keep said behavior, but with the change that previously we used single quotes, now double quotes [3]. Most of the expected test changes are because of this. In order to fix parsing-css-string-characters.html and string-quote-binary.html completely, the U+007F handling mentioned in [3] is implemented. Behavior matches Firefox. BUG=584999 [1] https://drafts.csswg.org/css-values-3/#custom-idents [2] https://drafts.csswg.org/cssom/#serialize-an-identifier [3] https://drafts.csswg.org/cssom/#serialize-a-string Committed: https://crrev.com/0c2c3981d4621fc6f5525e01f4de2a14cbc2d15e Cr-Commit-Position: refs/heads/master@{#381619}

Patch Set 1 #

Patch Set 2 : V2 #

Patch Set 3 : V3 #

Patch Set 4 : V4 #

Patch Set 5 : V5 #

Patch Set 6 : V6 #

Patch Set 7 : V7 #

Patch Set 8 : Add CSSFontFamilyValue #

Patch Set 9 : Also fix FontFace.cpp #

Total comments: 12

Patch Set 10 : Address review comments #

Patch Set 11 : Also fix serializeString #

Patch Set 12 : Fix test expectations #

Patch Set 13 : Fix ExecuteScriptApiTest.FrameWithHttp204 #

Patch Set 14 : Fix regexps #

Patch Set 15 : Fix win_chromium_rel_ng #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -146 lines) Patch
M chrome/test/data/extensions/api_test/executescript/http204/background.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/animations/animations-csstext.html View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/animations/svg-presentation-attribute-animation.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/editing/pasteboard/onpaste-text-html.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/font-family-trailing-bracket-gunk.html View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/font-family-trailing-bracket-gunk-expected.txt View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/computed-style-font-family-expected.txt View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/font-family-fallback-reset-expected.txt View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/getComputedStyle/script-tests/font-family-fallback-reset.js View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/parsing-css-string-characters.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +15 lines, -15 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/script-tests/string-quote-binary.js View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +10 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/css/string-quote-binary-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +10 lines, -10 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/ondrop-text-html.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/inspector-support/style-expected.txt View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/fast/css/css2-system-fonts-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -3 lines 0 comments Download
D third_party/WebKit/LayoutTests/typedcssom/keywordValue-expected.txt View 1 2 3 4 5 1 chunk +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSCustomIdentValue.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -1 line 0 comments Download
A third_party/WebKit/Source/core/css/CSSFontFamilyValue.h View 1 2 3 4 5 6 7 1 chunk +43 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/core/css/CSSFontFamilyValue.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +26 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSMarkup.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSMarkup.cpp View 1 2 3 4 5 6 7 8 9 10 3 chunks +7 lines, -74 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSValue.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSValue.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 6 chunks +14 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSValuePool.h View 1 2 3 4 5 6 7 3 chunks +3 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSValuePool.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/FontFace.cpp View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/editing/EditingStyle.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 44 (26 generated)
rwlbuis
PTAL. Note that Firefox actually has a slight difference in these two tests: fast/css/parsing-css-string-characters.html fast/css/string-quote-binary.html ...
4 years, 9 months ago (2016-03-14 21:41:42 UTC) #8
Timothy Loh
https://codereview.chromium.org/1778743003/diff/180001/third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html File third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html (right): https://codereview.chromium.org/1778743003/diff/180001/third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html#newcode25 third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html:25: return text.replace(/font-family: .+;; /, ''); I don't get this, ...
4 years, 9 months ago (2016-03-14 23:25:11 UTC) #9
rwlbuis
On 2016/03/14 21:41:42, rwlbuis wrote: > PTAL. > > Note that Firefox actually has a ...
4 years, 9 months ago (2016-03-15 18:29:44 UTC) #10
rwlbuis
PTAL. https://codereview.chromium.org/1778743003/diff/180001/third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html File third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html (right): https://codereview.chromium.org/1778743003/diff/180001/third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html#newcode25 third_party/WebKit/LayoutTests/editing/pasteboard/data-transfer-items.html:25: return text.replace(/font-family: .+;; /, ''); On 2016/03/14 23:25:11, ...
4 years, 9 months ago (2016-03-15 18:34:15 UTC) #12
Timothy Loh
On 2016/03/15 18:29:44, rwlbuis wrote: > On 2016/03/14 21:41:42, rwlbuis wrote: > > PTAL. > ...
4 years, 9 months ago (2016-03-15 23:26:45 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778743003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778743003/260001
4 years, 9 months ago (2016-03-15 23:42:41 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/131368)
4 years, 9 months ago (2016-03-16 00:40:59 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778743003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778743003/260001
4 years, 9 months ago (2016-03-16 00:51:55 UTC) #20
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/157404)
4 years, 9 months ago (2016-03-16 01:01:50 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778743003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778743003/260001
4 years, 9 months ago (2016-03-16 01:32:22 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/157424)
4 years, 9 months ago (2016-03-16 01:43:06 UTC) #26
rwlbuis
On 2016/03/16 01:43:06, commit-bot: I haz the power wrote: > Try jobs failed on following ...
4 years, 9 months ago (2016-03-16 01:52:34 UTC) #27
Timothy Loh
On 2016/03/16 01:52:34, rwlbuis wrote: > On 2016/03/16 01:43:06, commit-bot: I haz the power wrote: ...
4 years, 9 months ago (2016-03-16 12:37:46 UTC) #28
rwlbuis
On 2016/03/16 12:37:46, Timothy Loh wrote: > On 2016/03/16 01:52:34, rwlbuis wrote: > > On ...
4 years, 9 months ago (2016-03-16 12:48:02 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778743003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778743003/360001
4 years, 9 months ago (2016-03-16 20:05:49 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1778743003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1778743003/380001
4 years, 9 months ago (2016-03-16 21:29:33 UTC) #40
commit-bot: I haz the power
Committed patchset #15 (id:380001)
4 years, 9 months ago (2016-03-17 01:08:02 UTC) #42
commit-bot: I haz the power
4 years, 9 months ago (2016-03-17 01:10:23 UTC) #44
Message was sent while issue was closed.
Patchset 15 (id:??) landed as
https://crrev.com/0c2c3981d4621fc6f5525e01f4de2a14cbc2d15e
Cr-Commit-Position: refs/heads/master@{#381619}

Powered by Google App Engine
This is Rietveld 408576698