Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionMove 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 #
Messages
Total messages: 20 (2 generated)
rob.buis@samsung.com changed reviewers: + alancutter@chromium.org, timloh@chromium.org
PTAL. BTW parseFontFaceDescriptor should be cleaned up later to always test for !range.atEnd at the end of the method instead of in each case, but for that t work first parseFontWeight needs to work, which needs validUnit, which should be next :)
On 2015/09/15 00:50:15, rwlbuis wrote: > PTAL. > > BTW parseFontFaceDescriptor should be cleaned up later to always test for > !range.atEnd at the end of the method instead of in each case, but for that t > work first parseFontWeight needs to work, which needs validUnit, which should be > next :) I don't think parseFontWeight needs any sort of validation helpers. Let's not refer to m_valueList anywhere in the new code-path :/
https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:98: static bool tryConsumeUrl(CSSParserTokenRange& valueList, String& urlValue) "consumeUrl" is fine I think. Also maybe we can return a null String instead of a bool? Also needs argument renaming :-) I think we want consumeUrl to do nothing if we have a bad URL (e.g. "url('a' b)"). Probably consumeBlock from a temporary range and only update the passed in one if we succeed. https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:102: if (!token.valueEqualsIgnoringCase("url")) functionId? https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:107: if (next.type() == BadStringToken) || EOFToken I think? (i.e. "url()") https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:111: valueList.consumeWhitespace(); Should check whether the inner range is atEnd() https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:114: if (token.type() == UrlToken) { I think it'd be nicer if this check was before the function case (which could just be if (token.functionId() == ..) since functionId will check the type). https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:126: CSSParserTokenRange ret = valueList.consumeBlock(); ret -> result? contents? https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:197: while (!range.atEnd()) { do { } while (!range.atEnd()); return ligatureValues.release(); WDYT? peek (and the other functions) will always return something sane, i.e. EOFTokens. https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:198: if (range.peek().type() != IdentToken) No need to check this since id() will just return CSSValueInvalid https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:208: ligatureValues->append(consumeIdent(range)); Probably nicer outside of the switch statement so we don't have to write it four times https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:423: case CSSPropertyWebkitColumnSpan: let's leave the properties which aren't font-face descriptors for a separate patch https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:446: } while (consumeCommaIncludingWhitespace(range)); I like this comma-separated-list pattern, can we do it everywhere else? :-) https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:459: if (m_range.atEnd() || m_range.peek().type() != FunctionToken || m_range.peek().functionId() != CSSValueFormat) { No need to check atEnd() or type() here, functionId() should be enough. https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:467: if (args.atEnd() || (args.peek().type() != StringToken && args.peek().type() != IdentToken)) No need for atEnd() here.
On 2015/09/15 01:22:32, Timothy Loh wrote: > On 2015/09/15 00:50:15, rwlbuis wrote: > > PTAL. > > > > BTW parseFontFaceDescriptor should be cleaned up later to always test for > > !range.atEnd at the end of the method instead of in each case, but for that t > > work first parseFontWeight needs to work, which needs validUnit, which should > be > > next :) > > I don't think parseFontWeight needs any sort of validation helpers. Let's not > refer to m_valueList anywhere in the new code-path :/ Oh great! I think I was confused with line-height. Note that I don't think we can share parseFontFamily between old and new paths, I guess it needs to be duplicated for now (maybe parseFontWeight too). The problem in that the font shorthand parsing uses them and there we can't alternate between m_valueList and m_range.
https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/120001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:107: if (next.type() == BadStringToken) On 2015/09/15 02:01:05, Timothy Loh wrote: > || EOFToken I think? (i.e. "url()") Whoops, url() makes a UrlToken.
@timloh all your issues should be addressed by patch set 9, PTAL. You are right that we should stick to font descriptor properties only in this CL.
@timloh all your issues should be addressed by patch set 9, PTAL. You are right that we should stick to font descriptor properties only in this CL.
Lots of small comments, mostly I still think we should make all the code better instead of taking the existing code and trying to make it work with CSSParserTokens. BTW in future patches I don't think we should be moving so much logic at once. Preferably the diffs would all be <100 lines. I think moving lots at once actually will make things move slower because it'll take longer to do reviews and for you to pipeline changes. https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:122: CSSParserTokenRange contens = range.consumeBlock(); contens -> contents? :-) https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:183: static PassRefPtrWillBeRawPtr<CSSValue> consumeFontVariantLigatures(CSSParserTokenRange& range) Can we move the properties which are not @font-face descriptors in a separate patch? It'll be much easier to review these if the diffs are smaller :) https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:225: if (!ligatureValues->length()) The list will never be empty here https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:231: static bool consumeFontFeatureTag(CSSParserTokenRange& range, CSSValueList* settings) should probably just return a CSSFontFeatureValue and not take a CSSValueList https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:236: CSSParserToken token = range.peek(); const CSSParserToken&. Also maybe easier if this is just consumeIncludingWhitespace? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:253: if (!range.atEnd()) { don't need this check, peek() will just return EOFToken https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:255: tagValue = clampTo<int>(range.peek().numericValue()); range.consumeIncludingWhitespace().numericValue()? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:257: return false; does this ever get hit? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:260: tagValue = range.peek().id() == CSSValueOn; range.consumeIncludingWhitespace().id() https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:268: static PassRefPtrWillBeRawPtr<CSSValue> consumeFontFeatureSettings(CSSParserTokenRange& range) this one too, separate patch https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:276: if (range.atEnd()) I think just return the value outside the loop and not have this check. https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:313: // normal | small-caps | inherit Comment is wrong, let's just drop these (as well as elsewhere, most of them are wrong..) https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:314: PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::consumeFontVariant() static for consistency? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:316: RefPtrWillBeRawPtr<CSSValueList> values = nullptr; Can we rewrite this function so it isn't super complicated? Also would be nice to comment that "all" is no longer in the spec and having lists of normal/small-caps is also no longer valid. https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:404: bool CSSPropertyParser::consumeFontFaceSrcURI(CSSValueList* valueList) return a CSSValue instead of updating a CSSValueList directly https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:417: // FIXME: http://www.w3.org/TR/2011/WD-css3-fonts-20111004/ says that format() contains a comma-separated list of strings, Should update the URL (https://drafts.csswg.org/css-fonts). Also maybe a good idea to make it clearer that IdentTokens are not valid as per spec here. https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:422: uriValue->setFormat(args.consumeIncludingWhitespace().value()); need to check for any tokens after the string? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:427: bool CSSPropertyParser::consumeFontFaceSrcLocal(CSSValueList* valueList) return a CSSValue https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:433: const CSSParserToken& arg = args.consumeIncludingWhitespace(); This is just <family-name>, right? We should share the logic with font-family as much as possible (the font-family property will need to check the <generic-family> idents but is otherwise just a simple commas separated list). The logic for font-family below seems super convoluted to me. https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:479: return token.id() == CSSValueInitial || token.id() == CSSValueInherit || token.id() == CSSValueUnset || token.id() == CSSValueDefault; I know id() has a cache but can we pull the value into a local first? Also this probably deserves to be higher up in the file. https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:557: if (token.type() != NumberToken) || token.numericValueType() != IntegerValueType? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:566: bool CSSPropertyParser::parseFontFaceDescriptor(CSSPropertyID propId) I think we decided a while ago to split this into a function that returns a CSSValue and a function which checks everything is consumed and yields the property? https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:591: addProperty(propId, cssValuePool().createIdentifierValue(id), false); parsedValue = ?
On 2015/09/16 03:22:29, Timothy Loh wrote: > Lots of small comments, mostly I still think we should make all the code better > instead of taking the existing code and trying to make it work with > CSSParserTokens. Sounds good. > BTW in future patches I don't think we should be moving so much logic at once. > Preferably the diffs would all be <100 lines. I think moving lots at once > actually will make things move slower because it'll take longer to do reviews > and for you to pipeline changes. Ok. Downside is that it will take a lot of patches/time with +- 100 line patches :/ > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > File Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:122: CSSParserTokenRange contens = > range.consumeBlock(); > contens -> contents? :-) Fixed. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:183: static > PassRefPtrWillBeRawPtr<CSSValue> > consumeFontVariantLigatures(CSSParserTokenRange& range) > Can we move the properties which are not @font-face descriptors in a separate > patch? It'll be much easier to review these if the diffs are smaller :) Moved to https://codereview.chromium.org/1345063003/. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:225: if (!ligatureValues->length()) > The list will never be empty here Fixed in https://codereview.chromium.org/1345063003/. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:231: static bool > consumeFontFeatureTag(CSSParserTokenRange& range, CSSValueList* settings) > should probably just return a CSSFontFeatureValue and not take a CSSValueList Done. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:236: CSSParserToken token = > range.peek(); > const CSSParserToken&. Also maybe easier if this is just > consumeIncludingWhitespace? Done. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:253: if (!range.atEnd()) { > don't need this check, peek() will just return EOFToken Fixed. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:255: tagValue = > clampTo<int>(range.peek().numericValue()); > range.consumeIncludingWhitespace().numericValue()? Fixed. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:257: return false; > does this ever get hit? It is a proper 32-bit bug fix on WebKit side, I'd rather not touch it. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:260: tagValue = range.peek().id() > == CSSValueOn; > range.consumeIncludingWhitespace().id() Done. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:268: static > PassRefPtrWillBeRawPtr<CSSValue> consumeFontFeatureSettings(CSSParserTokenRange& > range) > this one too, separate patch In fact we called this in parseFontFaceDescriptor before my patch, is that wrong? > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:276: if (range.atEnd()) > I think just return the value outside the loop and not have this check. Fixed. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:313: // normal | small-caps | > inherit > Comment is wrong, let's just drop these (as well as elsewhere, most of them are > wrong..) All those kind of comments removed. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:314: > PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::consumeFontVariant() > static for consistency? We still call m_styleRule. Otherwise I do prefer static. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:316: > RefPtrWillBeRawPtr<CSSValueList> values = nullptr; > Can we rewrite this function so it isn't super complicated? Also would be nice > to comment that "all" is no longer in the spec and having lists of > normal/small-caps is also no longer valid. I tried to simplify it. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:404: bool > CSSPropertyParser::consumeFontFaceSrcURI(CSSValueList* valueList) > return a CSSValue instead of updating a CSSValueList directly Fixed. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:417: // FIXME: > http://www.w3.org/TR/2011/WD-css3-fonts-20111004/ says that format() contains a > comma-separated list of strings, > Should update the URL (https://drafts.csswg.org/css-fonts). Also maybe a good > idea to make it clearer that IdentTokens are not valid as per spec here. Done. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:422: > uriValue->setFormat(args.consumeIncludingWhitespace().value()); > need to check for any tokens after the string? Logic added. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:427: bool > CSSPropertyParser::consumeFontFaceSrcLocal(CSSValueList* valueList) > return a CSSValue Done. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:433: const CSSParserToken& arg = > args.consumeIncludingWhitespace(); > This is just <family-name>, right? We should share the logic with font-family as > much as possible (the font-family property will need to check the > <generic-family> idents but is otherwise just a simple commas separated list). > The logic for font-family below seems super convoluted to me. See below. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:479: return token.id() == > CSSValueInitial || token.id() == CSSValueInherit || token.id() == CSSValueUnset > || token.id() == CSSValueDefault; > I know id() has a cache but can we pull the value into a local first? Also this > probably deserves to be higher up in the file. Done. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:557: if (token.type() != > NumberToken) > || token.numericValueType() != IntegerValueType? Good catch! This is an existing bug! I changed a testcase to catch this. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:566: bool > CSSPropertyParser::parseFontFaceDescriptor(CSSPropertyID propId) > I think we decided a while ago to split this into a function that returns a > CSSValue and a function which checks everything is consumed and yields the > property? I am not sure that is a big improvement. In my latest patch I reuse parseSingleValue more so the whole method seems shorter. Let me know if you feel strongly about this and if possible a reference to the original discussion would be nice. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:591: addProperty(propId, > cssValuePool().createIdentifierValue(id), false); > parsedValue = ? Absolutely! So I think the main remaining issue is the consumeFontFamily/consumeFontFaceSrcLocal complexity. Are you in favor of removing FontFamilyBuilder and reusing more code between this two methods?
> https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > > Source/core/css/parser/CSSPropertyParser.cpp:268: static > > PassRefPtrWillBeRawPtr<CSSValue> > consumeFontFeatureSettings(CSSParserTokenRange& > > range) > > this one too, separate patch > > In fact we called this in parseFontFaceDescriptor before my patch, is that > wrong? Nevermind, looks fine. > https://codereview.chromium.org/1331233003/diff/160001/Source/core/css/parser... > > Source/core/css/parser/CSSPropertyParser.cpp:314: > > PassRefPtrWillBeRawPtr<CSSValue> CSSPropertyParser::consumeFontVariant() > > static for consistency? > > We still call m_styleRule. Otherwise I do prefer static. OK. > So I think the main remaining issue is the > consumeFontFamily/consumeFontFaceSrcLocal complexity. Are you in favor of > removing FontFamilyBuilder and reusing more code between this two methods? SGTM.
On 2015/09/17 01:21:39, Timothy Loh wrote: > > So I think the main remaining issue is the > > consumeFontFamily/consumeFontFaceSrcLocal complexity. Are you in favor of > > removing FontFamilyBuilder and reusing more code between this two methods? > > SGTM. Done! You are right, it can be done with much less complexity and no FontFamilyBuilder, see patch set #15. Sadly, it fails to compile due to a clang warning (it compiled on my machine using gcc). Nevertheless, maybe you have some time to look at my changes, PTAL.
Hopefully the last round of comments :) https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:128: static inline bool isComma(const CSSParserToken& value) not used now, not sure if we'll actually need it :) https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:422: CSSParserToken token = range.peek(); Copying CSSParserTokens is a bit dodgy, I've been meaning to make this compile error but I haven't worked out a good way to do it yet... How about const CSSParserToken& firstToken = range.peek(), and then builder.append(range.consumeIncludingWhitespace().value()) below? https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:441: if (args.atEnd()) No need to check this I guess. I think the only places where we'll have to check atEnd() for individual properties is for trailing junk in functions. https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:510: if (!list->length() || (m_ruleType == StyleRule::FontFace && list->length() > 1)) list is never empty here. Also how about just return nullptr instead?
On 2015/09/18 03:49:18, Timothy Loh wrote: > Hopefully the last round of comments :) Hope so too! :) > https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... > File Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:128: static inline bool > isComma(const CSSParserToken& value) > not used now, not sure if we'll actually need it :) Removed for now. I don't think it is needed really, seeing CommaToken is quite clear. > https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:422: CSSParserToken token = > range.peek(); > Copying CSSParserTokens is a bit dodgy, I've been meaning to make this compile > error but I haven't worked out a good way to do it yet... > > How about const CSSParserToken& firstToken = range.peek(), and then > builder.append(range.consumeIncludingWhitespace().value()) below? Done. > https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:441: if (args.atEnd()) > No need to check this I guess. I think the only places where we'll have to check > atEnd() for individual properties is for trailing junk in functions. Done. > https://codereview.chromium.org/1331233003/diff/280001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:510: if (!list->length() || > (m_ruleType == StyleRule::FontFace && list->length() > 1)) > list is never empty here. Also how about just return nullptr instead? Done. PTAL :)
https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser... File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser... Source/core/css/parser/CSSPropertyParser.cpp:124: range.consumeWhitespace(); Uhh.. we should also consumeWhitespace from contents, right?
On 2015/09/19 03:24:59, Timothy Loh wrote: > https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser... > File Source/core/css/parser/CSSPropertyParser.cpp (right): > > https://codereview.chromium.org/1331233003/diff/300001/Source/core/css/parser... > Source/core/css/parser/CSSPropertyParser.cpp:124: range.consumeWhitespace(); > Uhh.. we should also consumeWhitespace from contents, right? Well spotted, fixed! PTAL
lgtm
The CQ bit was checked by rob.buis@samsung.com
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
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=202630 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
