Bug 129713

Created attachment 227939 [details] Patch

ping reviewers :)

Comment on attachment 227939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227939&action=review > Source/WebCore/css/CSSParser.cpp:4698 > +static inline bool isValidCustomIdent(CSSParserValue& value) Seems like the argument should be const& rather than just &. > Source/WebCore/css/CSSParser.cpp:4749 > + if (m_valueList->next()) { > + if (m_valueList->current() && !isForwardSlashOperator(m_valueList->current())) { > + if (!parseIntegerOrCustomIdentFromGridPosition(numericValue, gridLineName)) > + return nullptr; > + } > + } There is no reason to check current() for null just after calling next(), since next() returns the same thing that current() will return. This should be written more like this: if (auto* nextValue = m_valueList->next()) { if (!isForwardSlashOperator(nextValue) && !parseIntegerOrCustomIdentFromGridPosition(numericValue, gridLineName)) return nullptr; } But also, I think the logic here is getting really twisted. Repeating the !isForwardSlashOperator code is not good. I’d like to look at rearranging the function so this doesn’t have to be repeated. > Source/WebCore/css/StyleResolver.cpp:1987 > if (isSpanPosition) > - position.setSpanPosition(gridLineNumber, gridLineName); > + position.setSpanPosition(gridLineNumber ? gridLineNumber : 1, gridLineName); > else > position.setExplicitPosition(gridLineNumber, gridLineName); The difference between the two sides of the if here seem really strange. Do we have sufficient test coverage? Is there a value in adding a comment explaining why we have a minimum of 1 in one case, but not the other? Might also consider writing it as std::max(gridLineNumber, 1) instead of as a ? : expression.

Comment on attachment 227939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227939&action=review Thanks for the usual great review! >> Source/WebCore/css/CSSParser.cpp:4749 >> + } > > There is no reason to check current() for null just after calling next(), since next() returns the same thing that current() will return. This should be written more like this: > > if (auto* nextValue = m_valueList->next()) { > if (!isForwardSlashOperator(nextValue) && !parseIntegerOrCustomIdentFromGridPosition(numericValue, gridLineName)) > return nullptr; > } > > But also, I think the logic here is getting really twisted. Repeating the !isForwardSlashOperator code is not good. I’d like to look at rearranging the function so this doesn’t have to be repeated. Good point w.r.t. the next() thing. Also I'll consider the rearrangement you mentioned. Just wanted to keep the change minimal, but it's true that the resulting logic might be a bit twisted. >> Source/WebCore/css/StyleResolver.cpp:1987 >> position.setExplicitPosition(gridLineNumber, gridLineName); > > The difference between the two sides of the if here seem really strange. Do we have sufficient test coverage? Is there a value in adding a comment explaining why we have a minimum of 1 in one case, but not the other? > > Might also consider writing it as std::max(gridLineNumber, 1) instead of as a ? : expression. In the if part we set a SpanPosition while in the else we set an ExplicitPosition. It's true that the arguments are the same but the type of position, and the resolution, is totally different. Regarding the 1 as minimum, I agree it deserves a comment. For span positions the specs mandates that it should take an integer > 0, but that restriction does not apply for explicit positions. I'll consider switching to a std::max() expression. Finally, we indeed have test coverage for all the potential permutations allowed by the specs.

Committed r166712: <http://trac.webkit.org/changeset/166712>