Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251663002:
[Typed-OM] Get CSSTokenStreamValue from StyleMap (Closed)
|
Created:
4 years, 4 months ago by anthonyhkf Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews-style_chromium.org, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Typed-OM] Get CSSTokenStreamValue from StyleMap
Spec: https://drafts.css-houdini.org/css-typed-om/#tokenstreamvalue-normalization
BUG=545318
Committed: https://crrev.com/ef023fe7c2b002e589d53f088977d567019343dc
Cr-Commit-Position: refs/heads/master@{#415006}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Move functions to CSSTokenStreamValue #Patch Set 3 : Add layouttest #
Total comments: 19
Patch Set 4 : Refactor and add test #
Total comments: 18
Patch Set 5 : Simplify functions #Patch Set 6 : Simplify functions, add test #
Total comments: 13
Patch Set 7 : Rebase and change parsing #
Total comments: 1
Patch Set 8 : Simplify function chain #
Total comments: 15
Patch Set 9 : Add more tests and change some functions #Patch Set 10 : Change test to expect CSSTokenStreamValue #
Total comments: 1
Patch Set 11 : Remove unused header #
Messages
Total messages: 46 (29 generated)
anthonyhkf@google.com changed reviewers: + meade@chromium.org
https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp (right): https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:21: void parseToken(CSSParserTokenRange range, Vector<Persistent<HeapVector<StringOrCSSVariableReferenceValue>>>& stack, bool isDirectlyInsideVar, String& stringLeft) I think this function can be simplified greatly if you use StringBuilder and the proper CSSParserToken methods on everything. Try to avoid string comparisons and allocations as much as you can or it'll be slow. Also, since this is so big, this function should be a static method on CSSTokenStreamValue. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:23: String variable; You probably want to use a StringBuilder for this. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:26: bool isVar = equalIgnoringASCIICase(range.peek().value(), "var"); You can do range.peek().functionId() == CSSValueVar here. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:39: stack.last()->append(StringOrCSSVariableReferenceValue::fromCSSVariableReferenceValue(CSSStyleVariableReferenceValue::create(stringLeft, (*lastVect.get()).size() ? CSSTokenStreamValue::create(*lastVect.get()) : nullptr))); This statement is way complex. It would be more readable if you split it up. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:96: Vector<Persistent<HeapVector<StringOrCSSVariableReferenceValue>>> stack; What? There must be a simpler way to do this other than a vector of vectors. You're only trying to create one final vector, right?
https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp (right): https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:23: String variable; On 2016/08/17 00:55:43, Eddy wrote: > You probably want to use a StringBuilder for this. I'm not sure what to use for this one. Should I use StringBuilder or StringView? This 'variable' will only be used if the function is inside a var(). The one that we get from range.peek().value() is of type StringView. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:26: bool isVar = equalIgnoringASCIICase(range.peek().value(), "var"); On 2016/08/17 00:55:43, Eddy wrote: > You can do range.peek().functionId() == CSSValueVar here. Done. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:39: stack.last()->append(StringOrCSSVariableReferenceValue::fromCSSVariableReferenceValue(CSSStyleVariableReferenceValue::create(stringLeft, (*lastVect.get()).size() ? CSSTokenStreamValue::create(*lastVect.get()) : nullptr))); On 2016/08/17 00:55:43, Eddy wrote: > This statement is way complex. It would be more readable if you split it up. Done. https://codereview.chromium.org/2251663002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/cssom/StyleValueFactory.cpp:96: Vector<Persistent<HeapVector<StringOrCSSVariableReferenceValue>>> stack; On 2016/08/17 00:55:43, Eddy wrote: > What? There must be a simpler way to do this other than a vector of vectors. > You're only trying to create one final vector, right? Done.
This is looking much better, but I think there's still lots of room to make it simpler and more readable. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:48: CSSTokenStreamValue* CSSTokenStreamValue::fromCSSValue(const CSSValue& cssValue) It'd be more correct to take a CSSVariableReferenceValue here, and do the cast before this function gets called. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:54: void parseTokenRangeToFragments(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder, bool isInsideVariable) Put this helper function up in the anonymous namespace above, since it's not a member of CSSTokenStreamValue. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:56: StringView variable; This variable needs a more descriptive name - I don't understand what it's doing. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:62: CSSTokenStreamValue* tokenStreamValue = CSSTokenStreamValue::fromParserTokenRange(range.consumeBlock(), builder, true); It would be clearer if you split out the creation of the tokenstream value into another helper function. The level of nesting here is confusing. Alternatively you could pull out the entire section that handles what happens if it is a var. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:68: parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false); Should you be both serializing the first token, and then parsing the block? It seems like the chars will appear twice? https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:80: range.consume(); You should be able simplify these type of calls by instead doing CSSParserToken& token = range.consume(); if (token.type() == CSSParserTokenType...) .. } else if (token.type() == CSSParserTokenType...) https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:97: fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString())); Can this be moved up into parseTokenFragments? It's kind of weird here. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:100: return nullptr; If this is a private function, you can DCHECK this condition instead to ensure this never happens. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:27: static CSSTokenStreamValue* fromParserTokenRange(CSSParserTokenRange, StringBuilder&, bool); Can this be a private method?
https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:48: CSSTokenStreamValue* CSSTokenStreamValue::fromCSSValue(const CSSValue& cssValue) On 2016/08/19 08:43:51, Eddy wrote: > It'd be more correct to take a CSSVariableReferenceValue here, and do the cast > before this function gets called. Done. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:54: void parseTokenRangeToFragments(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder, bool isInsideVariable) On 2016/08/19 08:43:51, Eddy wrote: > Put this helper function up in the anonymous namespace above, since it's not a > member of CSSTokenStreamValue. Done. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:56: StringView variable; On 2016/08/19 08:43:52, Eddy wrote: > This variable needs a more descriptive name - I don't understand what it's > doing. Done. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:62: CSSTokenStreamValue* tokenStreamValue = CSSTokenStreamValue::fromParserTokenRange(range.consumeBlock(), builder, true); On 2016/08/19 08:43:51, Eddy wrote: > It would be clearer if you split out the creation of the tokenstream value into > another helper function. The level of nesting here is confusing. > > Alternatively you could pull out the entire section that handles what happens if > it is a var. Done. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:68: parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false); On 2016/08/19 08:43:51, Eddy wrote: > Should you be both serializing the first token, and then parsing the block? It > seems like the chars will appear twice? I think the consumeBlock() will only return the parts inside the block. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parse... https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:80: range.consume(); On 2016/08/19 08:43:51, Eddy wrote: > You should be able simplify these type of calls by instead doing > > CSSParserToken& token = range.consume(); > if (token.type() == CSSParserTokenType...) > .. > } else if (token.type() == CSSParserTokenType...) Done. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:97: fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString())); On 2016/08/19 08:43:51, Eddy wrote: > Can this be moved up into parseTokenFragments? It's kind of weird here. Done. https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:100: return nullptr; On 2016/08/19 08:43:51, Eddy wrote: > If this is a private function, you can DCHECK this condition instead to ensure > this never happens. It is possible to happen. This one is for the cases like "var(--a)" (without the value of the variable), it should return the fallback as "undefined". https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:27: static CSSTokenStreamValue* fromParserTokenRange(CSSParserTokenRange, StringBuilder&, bool); On 2016/08/19 08:43:52, Eddy wrote: > Can this be a private method? Done.
timloh@chromium.org changed reviewers: + timloh@chromium.org
https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:53: void handleVariableBlock(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder) handle -> parse? I think this function should be structured as: input: CSSParserTokenRange output: CSSVariableReferenceValue This function should be responsible for everything specific to var() blocks, instead of splitting it up. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:72: while (!range.atEnd()) { I think it'd be better (overall much simpler) to structure this as something like: input: CSSParserTokenRange output: CSSTokenStreamValue do { const CSSParserToken* start = &range.peek(); while (!atEnd() && !var) range.consume(); if (start != &range.peek()) append range.makeSubRange(start, &range.peek()).serialize(); if var, call other function } while (!atEnd()) https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:84: if (!isInsideVariable) BTW this logic is wrong because you can write "var(--foo, a, b)" which makes the fallback " a, b" but this would output " a b" and the variable name "b".
https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:68: parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false); On 2016/08/22 03:19:21, anthonyhkf wrote: > On 2016/08/19 08:43:51, Eddy wrote: > > Should you be both serializing the first token, and then parsing the block? It > > seems like the chars will appear twice? > > I think the consumeBlock() will only return the parts inside the block. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/parse... In that case it might be clearer to do builder.append('('); parseTokenRangeToFragments(range.consumeBlock(), fragments, builder, false); builder.append(')'); Or whatever the equivalent thing you expect to come from range.peek().serialize(). https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:5: <div id="testElement1"></div> You can use the same element for each test, and just set a different value in width. That way you don't need to have testElement1, testElement2 etc, and it just looks tidier. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:10: function compareTokenStreamValueWithArray(tokenStreamValue, expectedTokenStreamValue) { This function is much too complicated to be in a test - it would require tests of its own! Lets keep it simple and just do the iteration manually in your test. You'll get a better error message if the test fails that way too (right now you'll just get a message that says "expected true but got false", which isn't helpful). so like this: test(function() { testElement.style.width = "calc(42px + var(--foo, 15em) + var(--bar, var(--far) + 15px))"; var result = [...testElement.styleMap.get('width')]; assert_equals(result.length, whatever); assert_equals(result[0], 'calc(42px + '); ...etc }, "Construction of CSSTokenStreamValue is normalized using example from the spec"); https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:53: void handleVariableBlock(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder) how about calling this void variableReferenceValue(const StringView& name, const HeapVector<StringOrCSSVariableReferenceValue>& fragments)? As Tim says, it should only do stuff relevant to var() blocks, and if you get the recursion right, you shouldn't need to pass in a builder or CSSParserTokenRange. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:57: fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString())); If you move this bit to the caller of this function, you can avoid having to clear the builder inside this function, allowing you to create the new one. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:68: void parseTokenRangeToFragments(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder, bool isInsideVariable) nit: Was this supposed to be parserTokenRangeToFragments? (parse -> parser) https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:72: while (!range.atEnd()) { On 2016/08/22 06:30:59, Timothy Loh wrote: > I think it'd be better (overall much simpler) to structure this as something > like: > > input: CSSParserTokenRange > output: CSSTokenStreamValue > > do { > const CSSParserToken* start = &range.peek(); > while (!atEnd() && !var) range.consume(); > if (start != &range.peek()) > append range.makeSubRange(start, &range.peek()).serialize(); > if var, call other function > } while (!atEnd()) I agree, but I think it works out slightly better to have the function return a HeapVector<StringOrCSSVariableReferenceValue>, so like this: HeapVector<StringOrCSSVariableReferenceValue> parserTokenRangeToFragments2(CSSParserTokenRange range) { HeapVector<...> fragments; StringBuilder builder; while (!range.atEnd()) { if (range.peek().functionId() == CSSValueVar) { // Do var stuff } else { range.consume().serialize(builder); } } // Append builder stuff to fragments return fragments; } If you get this right, you don't need to pass any HeapVectors, StringBuilders, or booleans into this function :) https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:94: if (!builder.isEmpty()) { You do this line regardless of isInsideVariable, so why not move it out of the if statements? if (!builder.isEmpty()) { fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString())); } https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:112: CSSTokenStreamValue* CSSTokenStreamValue::fromCSSVariableReferenceValue(const CSSVariableReferenceValue& cssVariableReferenceValue) You can still call this fromCSSValue for consistency with the other Typed OM types though :)
https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:53: void handleVariableBlock(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder) On 2016/08/22 07:55:48, Eddy wrote: > how about calling this void variableReferenceValue(const StringView& name, const > HeapVector<StringOrCSSVariableReferenceValue>& fragments)? > > As Tim says, it should only do stuff relevant to var() blocks, and if you get > the recursion right, you shouldn't need to pass in a builder or > CSSParserTokenRange. Done. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:57: fragments.append(StringOrCSSVariableReferenceValue::fromString(builder.toString())); On 2016/08/22 07:55:48, Eddy wrote: > If you move this bit to the caller of this function, you can avoid having to > clear the builder inside this function, allowing you to create the new one. Done. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:68: void parseTokenRangeToFragments(CSSParserTokenRange range, HeapVector<StringOrCSSVariableReferenceValue>& fragments, StringBuilder& builder, bool isInsideVariable) On 2016/08/22 07:55:48, Eddy wrote: > nit: Was this supposed to be parserTokenRangeToFragments? (parse -> parser) Done. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:72: while (!range.atEnd()) { On 2016/08/22 07:55:47, Eddy wrote: > On 2016/08/22 06:30:59, Timothy Loh wrote: > > I think it'd be better (overall much simpler) to structure this as something > > like: > > > > input: CSSParserTokenRange > > output: CSSTokenStreamValue > > > > do { > > const CSSParserToken* start = &range.peek(); > > while (!atEnd() && !var) range.consume(); > > if (start != &range.peek()) > > append range.makeSubRange(start, &range.peek()).serialize(); > > if var, call other function > > } while (!atEnd()) > > I agree, but I think it works out slightly better to have the function return a > HeapVector<StringOrCSSVariableReferenceValue>, so like this: > > HeapVector<StringOrCSSVariableReferenceValue> > parserTokenRangeToFragments2(CSSParserTokenRange range) > { > HeapVector<...> fragments; > StringBuilder builder; > > while (!range.atEnd()) { > if (range.peek().functionId() == CSSValueVar) { > // Do var stuff > } else { > range.consume().serialize(builder); > } > } > > // Append builder stuff to fragments > return fragments; > } > > If you get this right, you don't need to pass any HeapVectors, StringBuilders, > or booleans into this function :) Done. https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:84: if (!isInsideVariable) On 2016/08/22 06:30:59, Timothy Loh wrote: > BTW this logic is wrong because you can write "var(--foo, a, b)" which makes the > fallback " a, b" but this would output " a b" and the variable name "b". Oh, sorry. Before I thought that it is only possible to have at most one value. This will return fallback as " a b" but the variable name is still "--foo". https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:112: CSSTokenStreamValue* CSSTokenStreamValue::fromCSSVariableReferenceValue(const CSSVariableReferenceValue& cssVariableReferenceValue) On 2016/08/22 07:55:47, Eddy wrote: > You can still call this fromCSSValue for consistency with the other Typed OM > types though :) Done.
Looks like you still need to sync this branch too. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:12: function compareTokenStreamValueWithArray(tokenStreamValue, expectedTokenStreamValue) { Don't forget to address my previous comments on this function. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:40: HeapVector<StringOrCSSVariableReferenceValue> parserTokenRangeToFragments(CSSParserTokenRange); You shouldn't need to have so complicated a function calling chain that this is necessary. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:42: CSSTokenStreamValue* fromParserTokenRangeToCSSTokenStreamValue(const CSSParserTokenRange& range) Still don't need to have "from" in the method name for conversion functions of the type classXToClassY. In any case, this function is unnecessary (and your code simplified) if you keep CSSParserToken handling strictly inside parserTokenRangeToFragments. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:45: // return undefined if there is no value following the variable's name s/undefined/null https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:53: while (range.peek().type() != CSSParserTokenType::IdentToken) You shouldn't need to do this. At most you'd need to consumeWhitespace(). Remember that the token stream is already validated by the time it gets here, and anyway if it wasn't, this would potentially return the wrong thing. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:58: CSSStyleVariableReferenceValue* variableReferenceValue(CSSParserTokenRange range) It would be clearer if this function took (StringView name, HeapVector fragments) instead of doing more parsing. That way you can simplify your recursion in parserTokenRangeToFragments. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:62: while (!range.atEnd() && range.consume().type() != CSSParserTokenType::CommaToken) { } If there's no comma, this will just skip a bunch of stuff... (e.g. var(--foo)) https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:82: } while (!range.atEnd()); This doesn't need to be a do/while loop, nor does it need to have nested loops. It's easier to see that while(!range.atEnd()) { //dostuff } will just get skipped and return an empty Vector, as opposed to do { while (!range.atEnd()) { ...} if (start != &range.peek()) { //do stuff } else if (!range.atEnd()) { // do other stuff } } while (!range.atEnd()) Which also does nothing if the range is empty. You can simplify to while(!range.atEnd()) { if (functionId == CSSValueVar) { // Serialize and clear your StringBuilder // Consume block // Extract variable name // Remove comma if necessary // Create CSSVariableReferenceValue, recursing tokenToFragments on consumed block } else { // Handle non-variable case } }
https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:5: <div id="testElement1"></div> On 2016/08/22 07:55:47, Eddy wrote: > You can use the same element for each test, and just set a different value in > width. That way you don't need to have testElement1, testElement2 etc, and it > just looks tidier. Done. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:12: function compareTokenStreamValueWithArray(tokenStreamValue, expectedTokenStreamValue) { On 2016/08/25 01:00:01, Eddy wrote: > Don't forget to address my previous comments on this function. Oh, sorry, I didn't see it. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:45: // return undefined if there is no value following the variable's name On 2016/08/25 01:00:01, Eddy wrote: > s/undefined/null Done. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:53: while (range.peek().type() != CSSParserTokenType::IdentToken) On 2016/08/25 01:00:01, Eddy wrote: > You shouldn't need to do this. At most you'd need to consumeWhitespace(). > Remember that the token stream is already validated by the time it gets here, > and anyway if it wasn't, this would potentially return the wrong thing. Done. https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:62: while (!range.atEnd() && range.consume().type() != CSSParserTokenType::CommaToken) { } On 2016/08/25 01:00:01, Eddy wrote: > If there's no comma, this will just skip a bunch of stuff... (e.g. var(--foo)) Hmm.. I think if there's no comma, it will go to range.atEnd(), and then it will stop (?) https://codereview.chromium.org/2251663002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:82: } while (!range.atEnd()); On 2016/08/25 01:00:01, Eddy wrote: > This doesn't need to be a do/while loop, nor does it need to have nested loops. > It's easier to see that > > while(!range.atEnd()) { > //dostuff > } > > will just get skipped and return an empty Vector, as opposed to > > do { > while (!range.atEnd()) { ...} > if (start != &range.peek()) { > //do stuff > } else if (!range.atEnd()) { > // do other stuff > } > } while (!range.atEnd()) > > Which also does nothing if the range is empty. > > > > You can simplify to > > while(!range.atEnd()) { > if (functionId == CSSValueVar) { > // Serialize and clear your StringBuilder > // Consume block > // Extract variable name > // Remove comma if necessary > // Create CSSVariableReferenceValue, recursing tokenToFragments on consumed > block > } else { > // Handle non-variable case > } > } Done. https://codereview.chromium.org/2251663002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:40: CSSTokenStreamValue* fragmentsToTokenStreamValue(const HeapVector<StringOrCSSVariableReferenceValue>& fragments) Should this be moved to the parserTokenRangeToFragments()? So it will become parserTokenRangeToTokenStreamValue() and return the CSSTokenStreamValue from the HeapVector (just move this function's body to the end of current parserTokenRangeToFragments()).
https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:13: assert_equals(result[0], "calc(42px + "); You seem to be missing a bracket? Don't forget to run your tests! :p https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:19: var result_3 = [...result[3].fallback]; How about a more descriptive variable name like "barFallback"? https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:38: testElement.style.width = " var( --a , 5px ) "; You should also test "calc( var( --far , 30px ) + 15px )", " var( --a ) " and "calc( var( --far ) + 15px )" if you're going to do this :) "Can handle extra spaces with fallback" "Can handle extra spaces without fallback" https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:48: return CSSStyleVariableReferenceValue::create(variableName.toString(), tokenStreamValue); It's not really worth creating a new function that just contains a single line. May as well just put this back inside the other function in this case. FWIW, I thought it was tidier when the recursing function only dealt with tokens, and didn't instantiate any objects, so the functions were StringOrCSSVariableReferenceValue variableReferenceValue(const StringView& name, HeapVector<StringOrCSSVariableReferenceValue> tokens) { // Instantiates CSSTokenStreamValue value if tokens not empty // Instantiates CSSVariableReferenceValue // Wraps CSSVariableReferenceValue in StringOr } HeapVector<StringOrCSSVariableReferenceValue> parserTokenRangeToFragments(CSSParserTokenRange range) { // while !atEnd // if var // find variable name // fragments.append(variableReferenceValue(variableName, parserTokenRangeToFragments(block)); // etc } ... For the fromCSSValue case, I think it's fine to return an empty CSSTokenStreamValue. https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:61: CSSParserTokenRange current = range.consumeBlock(); Current is not a good variable name, as it is too general. Variable names should describe what they are, in this case "block" could be a good name (because it came from consumeBlock, and "range" is already taken). https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:63: while (!current.atEnd() && current.consume().type() != CSSParserTokenType::CommaToken) { } This will do the wrong thing if there isn't a comma. Why not just do consumeWhitespace() if (peek().type() == Comma) consume() ? https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:72: // returns undefined/null if there is no value following the variable's name We aren't in a var(--foo, fallback) block, so this comment doesn't make sense. It's also not really useful to just state what the code is doing (which is returning null if no fragments were found)
https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:13: assert_equals(result[0], "calc(42px + "); On 2016/08/26 01:29:22, Eddy wrote: > You seem to be missing a bracket? Don't forget to run your tests! :p Hmm? I think the brackets match? https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:19: var result_3 = [...result[3].fallback]; On 2016/08/26 01:29:22, Eddy wrote: > How about a more descriptive variable name like "barFallback"? Done. https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:38: testElement.style.width = " var( --a , 5px ) "; On 2016/08/26 01:29:22, Eddy wrote: > You should also test "calc( var( --far , 30px ) + 15px )", " var( --a ) " > and "calc( var( --far ) + 15px )" if you're going to do this :) > > "Can handle extra spaces with fallback" > "Can handle extra spaces without fallback" Done. https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp (right): https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:48: return CSSStyleVariableReferenceValue::create(variableName.toString(), tokenStreamValue); On 2016/08/26 01:29:22, Eddy wrote: > It's not really worth creating a new function that just contains a single line. > May as well just put this back inside the other function in this case. > > FWIW, I thought it was tidier when the recursing function only dealt with > tokens, and didn't instantiate any objects, so the functions were > > StringOrCSSVariableReferenceValue variableReferenceValue(const StringView& name, > HeapVector<StringOrCSSVariableReferenceValue> tokens) > { > // Instantiates CSSTokenStreamValue value if tokens not empty > // Instantiates CSSVariableReferenceValue > // Wraps CSSVariableReferenceValue in StringOr > } > > HeapVector<StringOrCSSVariableReferenceValue> > parserTokenRangeToFragments(CSSParserTokenRange range) > { > // while !atEnd > // if var > // find variable name > // fragments.append(variableReferenceValue(variableName, > parserTokenRangeToFragments(block)); > // etc > } > > ... > > For the fromCSSValue case, I think it's fine to return an empty > CSSTokenStreamValue. Done. https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:61: CSSParserTokenRange current = range.consumeBlock(); On 2016/08/26 01:29:22, Eddy wrote: > Current is not a good variable name, as it is too general. Variable names should > describe what they are, in this case "block" could be a good name (because it > came from consumeBlock, and "range" is already taken). Done. https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:63: while (!current.atEnd() && current.consume().type() != CSSParserTokenType::CommaToken) { } On 2016/08/26 01:29:22, Eddy wrote: > This will do the wrong thing if there isn't a comma. Why not just do > > consumeWhitespace() > if (peek().type() == Comma) > consume() > > ? Done. https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.cpp:72: // returns undefined/null if there is no value following the variable's name On 2016/08/26 01:29:22, Eddy wrote: > We aren't in a var(--foo, fallback) block, so this comment doesn't make sense. > It's also not really useful to just state what the code is doing (which is > returning null if no fragments were found) Done.
The CQ bit was checked by meade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nice work! lgtm Tim, since you're already on here, want to do the OWNERs review? https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html (right): https://codereview.chromium.org/2251663002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/typedcssom/inlinestyle/tokenStreamValue.html:13: assert_equals(result[0], "calc(42px + "); On 2016/08/26 03:10:28, anthonyhkf wrote: > On 2016/08/26 01:29:22, Eddy wrote: > > You seem to be missing a bracket? Don't forget to run your tests! :p > > Hmm? I think the brackets match? So they do! The extra bracket is in the string. My bad..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2251663002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h (right): https://codereview.chromium.org/2251663002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/css/cssom/CSSTokenStreamValue.h:12: #include "core/css/parser/CSSParserTokenRange.h" this include shouldn't be needed
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyhkf@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by anthonyhkf@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2251663002/#ps200001 (title: "Remove unused header")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Typed-OM] Get CSSTokenStreamValue from StyleMap Spec: https://drafts.css-houdini.org/css-typed-om/#tokenstreamvalue-normalization BUG=545318 ========== to ========== [Typed-OM] Get CSSTokenStreamValue from StyleMap Spec: https://drafts.css-houdini.org/css-typed-om/#tokenstreamvalue-normalization BUG=545318 Committed: https://crrev.com/ef023fe7c2b002e589d53f088977d567019343dc Cr-Commit-Position: refs/heads/master@{#415006} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ef023fe7c2b002e589d53f088977d567019343dc Cr-Commit-Position: refs/heads/master@{#415006} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
