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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 913313002: Use isComma more in CSSPropertyParser (Closed)

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

Description

Use isComma more in CSSPropertyParser In a lot of places there are tests for comma but isComma is not called. Also in some places the current CSSParserValue is null tested where it is not needed. BUG=404023 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190468

Patch Set 1 #

Patch Set 2 : Fix logic #

Patch Set 3 : Different approach #

Total comments: 1

Patch Set 4 : Handle stroke-dasharray trailing comma #

Unified diffs Side-by-side diffs Delta from patch set Stats (+14 lines, -10 lines) Patch
M LayoutTests/svg/custom/invalid-dasharray.svg View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 3 7 chunks +13 lines, -10 lines 0 comments Download

Messages

Total messages: 25 (16 generated)
rwlbuis
PTAL. Sorry for the amount of reviews, let me know if somebody else can do ...
5 years, 10 months ago (2015-02-17 21:17:33 UTC) #2
Timothy Loh
https://codereview.chromium.org/913313002/diff/40001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/913313002/diff/40001/Source/core/css/parser/CSSPropertyParser.cpp#newcode8176 Source/core/css/parser/CSSPropertyParser.cpp:8176: if (value && isComma(value)) We should probably fix this ...
5 years, 10 months ago (2015-02-17 23:37:02 UTC) #3
Timothy Loh
On 2015/02/17 21:17:33, rwlbuis wrote: > PTAL. Sorry for the amount of reviews, let > ...
5 years, 10 months ago (2015-02-17 23:38:40 UTC) #4
rwlbuis
On 2015/02/17 23:37:02, Timothy Loh wrote: > https://codereview.chromium.org/913313002/diff/40001/Source/core/css/parser/CSSPropertyParser.cpp > File Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/913313002/diff/40001/Source/core/css/parser/CSSPropertyParser.cpp#newcode8176 ...
5 years, 10 months ago (2015-02-18 19:18:18 UTC) #5
Timothy Loh
lgtm
5 years, 10 months ago (2015-02-19 00:08:06 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913313002/60001
5 years, 10 months ago (2015-02-19 03:25:48 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913313002/60001
5 years, 10 months ago (2015-02-19 06:00:34 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/913313002/60001
5 years, 10 months ago (2015-02-19 13:44:10 UTC) #24
commit-bot: I haz the power
5 years, 10 months ago (2015-02-19 13:52:48 UTC) #25
Message was sent while issue was closed. 
Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190468

Powered by Google App Engine
This is Rietveld 408576698