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

Issue 1331233003: Move parseFontFaceDescriptor to CSSPropertyParser.cpp (Closed)

Created:
5 years, 3 months ago by rwlbuis
Modified:
5 years, 3 months ago
CC:
blink-reviews, kenneth.christiansen, Yoav Weiss, blink-reviews-style_chromium.org, blink-reviews-css, 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

Move parseFontFaceDescriptor to CSSPropertyParser.cpp Move parseFontFaceDescriptor and related functions to CSSPropertyParser.cpp. This also adds helper functions to consume FunctionToken and URLs. BUG=499780 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202630

Patch Set 1 #

Patch Set 2 : V2 #

Patch Set 3 : Add consumeFontVariant #

Patch Set 4 : V3 #

Patch Set 5 : V4 #

Patch Set 6 : V5 #

Patch Set 7 : Patch for review #

Total comments: 14

Patch Set 8 : Address review comments #

Patch Set 9 : Cleanup consumeFontWeight and consumeUrl #

Total comments: 23

Patch Set 10 : First round of review fixes #

Patch Set 11 : Fix test fails and address more review comments #

Patch Set 12 : Fix more issues #

Patch Set 13 : More cleanup #

Patch Set 14 : Rebase #

Patch Set 15 : Simplify font family parsing #

Total comments: 4

Patch Set 16 : Address review comments #

Total comments: 1

Patch Set 17 : Add consumeWS for contents #

Unified diffs Side-by-side diffs Delta from patch set Stats (+316 lines, -273 lines) Patch
M LayoutTests/fast/css/font-weight-1.html View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M Source/core/css/parser/CSSPropertyParser.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +5 lines, -8 lines 0 comments Download
M Source/core/css/parser/CSSPropertyParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 6 chunks +306 lines, -1 line 0 comments Download
M Source/core/css/parser/LegacyCSSPropertyParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 11 chunks +4 lines, -263 lines 0 comments Download

Messages

Total messages: 20 (2 generated)
rwlbuis
PTAL. BTW parseFontFaceDescriptor should be cleaned up later to always test for !range.atEnd at the ...
5 years, 3 months ago (2015-09-15 00:50:15 UTC) #2
Timothy Loh
On 2015/09/15 00:50:15, rwlbuis wrote: > PTAL. > > BTW parseFontFaceDescriptor should be cleaned up ...
5 years, 3 months ago (2015-09-15 01:22:32 UTC) #3
Timothy Loh
https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser/CSSPropertyParser.cpp#newcode98 Source/core/css/parser/CSSPropertyParser.cpp:98: static bool tryConsumeUrl(CSSParserTokenRange& valueList, String& urlValue) "consumeUrl" is fine ...
5 years, 3 months ago (2015-09-15 02:01:05 UTC) #4
rwlbuis
On 2015/09/15 01:22:32, Timothy Loh wrote: > On 2015/09/15 00:50:15, rwlbuis wrote: > > PTAL. ...
5 years, 3 months ago (2015-09-15 02:23:12 UTC) #5
Timothy Loh
https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser/CSSPropertyParser.cpp#newcode107 Source/core/css/parser/CSSPropertyParser.cpp:107: if (next.type() == BadStringToken) On 2015/09/15 02:01:05, Timothy Loh ...
5 years, 3 months ago (2015-09-15 03:05:04 UTC) #6
rwlbuis
@timloh all your issues should be addressed by patch set 9, PTAL. You are right ...
5 years, 3 months ago (2015-09-15 22:33:42 UTC) #7
rwlbuis
@timloh all your issues should be addressed by patch set 9, PTAL. You are right ...
5 years, 3 months ago (2015-09-15 22:33:43 UTC) #8
Timothy Loh
Lots of small comments, mostly I still think we should make all the code better ...
5 years, 3 months ago (2015-09-16 03:22:29 UTC) #9
rwlbuis
On 2015/09/16 03:22:29, Timothy Loh wrote: > Lots of small comments, mostly I still think ...
5 years, 3 months ago (2015-09-17 00:50:58 UTC) #10
Timothy Loh
> https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser/CSSPropertyParser.cpp#newcode268 > > Source/core/css/parser/CSSPropertyParser.cpp:268: static > > PassRefPtrWillBeRawPtr<CSSValue> > consumeFontFeatureSettings(CSSParserTokenRange& > > range) > ...
5 years, 3 months ago (2015-09-17 01:21:39 UTC) #11
rwlbuis
On 2015/09/17 01:21:39, Timothy Loh wrote: > > So I think the main remaining issue ...
5 years, 3 months ago (2015-09-18 03:12:46 UTC) #12
Timothy Loh
Hopefully the last round of comments :) https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser/CSSPropertyParser.cpp#newcode128 Source/core/css/parser/CSSPropertyParser.cpp:128: static inline ...
5 years, 3 months ago (2015-09-18 03:49:18 UTC) #13
rwlbuis
On 2015/09/18 03:49:18, Timothy Loh wrote: > Hopefully the last round of comments :) Hope ...
5 years, 3 months ago (2015-09-18 21:37:21 UTC) #14
Timothy Loh
https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser/CSSPropertyParser.cpp#newcode124 Source/core/css/parser/CSSPropertyParser.cpp:124: range.consumeWhitespace(); Uhh.. we should also consumeWhitespace from contents, right?
5 years, 3 months ago (2015-09-19 03:24:59 UTC) #15
rwlbuis
On 2015/09/19 03:24:59, Timothy Loh wrote: > https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser/CSSPropertyParser.cpp > File Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser/CSSPropertyParser.cpp#newcode124 ...
5 years, 3 months ago (2015-09-21 23:22:43 UTC) #16
Timothy Loh
lgtm
5 years, 3 months ago (2015-09-22 03:55:43 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1331233003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1331233003/320001
5 years, 3 months ago (2015-09-22 10:31:49 UTC) #19
commit-bot: I haz the power
5 years, 3 months ago (2015-09-22 12:29:14 UTC) #20
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202630

Powered by Google App Engine
This is Rietveld 408576698