Chromium Code Reviews

Issue 1219153003: Unify handling of <color> for SVG/non-SVG properties (Closed)

Created:
5 years, 5 months ago by fs
Modified:
5 years, 5 months ago
Reviewers:
Timothy Loh
CC:
blink-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, rwlbuis
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Unify handling of <color> for SVG/non-SVG properties Stop resolving system colors and "basic" named colors at parse time, and instead resolve them when resolving style. Implementation-wise this means getting rid of CSSPropertyParser::parseSVGColor in favor of parseColor, and similarly get rid of convertSVGColor in favor of convertColor on the style resolver side. Changes behavior for "basic" color names and system colors (as well as some proprietary/internal keywords). New behavior matches Gecko. BUG=370830, 505410 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198199

Patch Set 1 #

Patch Set 2 : More expectation changes. #

Patch Set 3 : Rebase #

Unified diffs Side-by-side diffs Stats (+32 lines, -59 lines)
M LayoutTests/fast/css/parse-color-int-or-percent-crash.html View 1 chunk +1 line, -1 line 0 comments
M LayoutTests/inspector/animation/animation-timeline-expected.txt View 1 chunk +4 lines, -4 lines 0 comments
M LayoutTests/svg/dom/rgb-color-parser-expected.txt View 2 chunks +4 lines, -4 lines 0 comments
M Source/build/scripts/templates/StyleBuilderFunctions.cpp.tmpl View 1 chunk +17 lines, -19 lines 0 comments
M Source/core/css/CSSProperties.in View 3 chunks +3 lines, -3 lines 0 comments
M Source/core/css/parser/CSSPropertyParser.h View 1 chunk +0 lines, -1 line 0 comments
M Source/core/css/parser/CSSPropertyParser.cpp View 4 chunks +3 lines, -17 lines 0 comments
M Source/core/css/resolver/StyleBuilderConverter.h View 1 chunk +0 lines, -1 line 0 comments
M Source/core/css/resolver/StyleBuilderConverter.cpp View 1 chunk +0 lines, -9 lines 0 comments

Messages

Total messages: 6 (2 generated)
fs
5 years, 5 months ago (2015-07-01 11:56:55 UTC) #2
Timothy Loh
On 2015/07/01 11:56:55, fs wrote: lgtm
5 years, 5 months ago (2015-07-02 01:44:54 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1219153003/40001
5 years, 5 months ago (2015-07-02 08:39:45 UTC) #5
commit-bot: I haz the power
5 years, 5 months ago (2015-07-02 08:43:28 UTC) #6
Message was sent while issue was closed. 
Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198199

Powered by Google App Engine