Bug 110815

Created attachment 195374 [details] Patch not for formal review

Created attachment 195717 [details] Patch not for formal review

Comment on attachment 195374 [details] Patch not for formal review View in context: https://bugs.webkit.org/attachment.cgi?id=195374&action=review The parser implementation is not correct. r- > Source/WebCore/css/CSSParser.cpp:8814 > + // Third argument must be (detached | attached). This comment is wrong. The third argument can be and integer. According to the grammar above you can have: int int int int det int att int int det int int att att int att int int det int det int int > Source/WebCore/css/CSSParser.cpp:8820 > + if (gridParserValue) { > + if (gridParserValue->id == CSSValueAttached || gridParserValue->id == CSSValueDetached) > + geometryList->append(cssValuePool().createIdentifierValue(gridParserValue->id)); > + else { > + geometryList.release(); yes, this does not do what you expect according to the grammar. > LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-geometry-property-invalid-expected.txt:78 > +Attached value before numeric value > +geometry: grid(attached 1); This is valid. > LayoutTests/css3/filters/custom-with-at-rule-syntax/parsing-geometry-property-invalid-expected.txt:84 > +Detached value before numeric value > +geometry: grid(detached 1); valid

Comment on attachment 195717 [details] Patch not for formal review View in context: https://bugs.webkit.org/attachment.cgi?id=195717&action=review See last comments as well. r- for this one as well. But I guess you want to watch the bots. > Source/WebCore/css/CSSParser.cpp:8815 > + // Third argument must be (detached | attached). > + gridParserValue = gridParserValueList->next(); > + if (gridParserValue) { Isn't there a way to make the conditions less nested?

Comment on attachment 195717 [details] Patch not for formal review Looking good, Michelangelo. Thanks for the clarification on the syntax, Dirk. I'll take another look when you address Dirk's comments, but here's a couple of things I noticed: View in context: https://bugs.webkit.org/attachment.cgi?id=195717&action=review > Source/WebCore/css/CSSParser.cpp:8791 > + CSSParserValueList* gridParserValueList = value->function->args.get(); Can you add an invalid parsing test for "grid()"? I'm worried gridParserValueList can be null, and we dereference it on the next line. > Source/WebCore/css/CSSParser.cpp:8795 > + if (!validUnit(gridParserValue, FInteger | FNonNeg) || !gridParserValue->fValue) Could we write this a little shorter? Like this: if (!validUnit(gridParserValue, FPositiveInteger)) FPositiveInteger means greater than zero, so you don't need to do the extra zero check.

Comment on attachment 195717 [details] Patch not for formal review View in context: https://bugs.webkit.org/attachment.cgi?id=195717&action=review > Source/WebCore/css/CSSParser.cpp:2878 > + return parseGeometry(propId, value, important); Let's only accept the geometry property inside a @filter rule. You can change this line to: return m_inFilterRule ? parseGeometry(propId, value, important) : 0; This is similar to CSSPropertySrc, where we do different parsing if we're in a @filter rule vs. a @font-face rule.

Created attachment 196201 [details] Patch

Comment on attachment 196201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196201&action=review I review the patch together with Max. The patch looks great already. Just some small modifications. The tests are good. > Source/WebCore/css/CSSParser.cpp:2887 > + return m_inFilterRule ? parseGeometry(propId, value, important) : 0; s/0/false/ > Source/WebCore/css/CSSParser.cpp:8802 > + // grid() function should have 1..3 arguments. .. -> to > Source/WebCore/css/CSSParser.cpp:8803 > + unsigned size = value->function->args->size(); Can you add at least an assertion for args? > Source/WebCore/css/CSSParser.cpp:8812 > + bool allowFirstNumber = true; > + bool allowSecondNumber = false; > + bool allowAttachment = true; How about breaking up the parsing of the mesh dimensions in a helper function "parseMeshDimensions". That function will parse 1 or 2 numbers or return false.

(In reply to comment #8) Good comments, will apply them in the next revision. > > Source/WebCore/css/CSSParser.cpp:8812 > > + bool allowFirstNumber = true; > > + bool allowSecondNumber = false; > > + bool allowAttachment = true; > > How about breaking up the parsing of the mesh dimensions in a helper function "parseMeshDimensions". That function will parse 1 or 2 numbers or return false. I thought about this already but I disregarded the idea, mostly for readability and slightly increased complexity in dealing with cases such as "1 attached 1": in such a case I'd have to perform one more check before cycling on the parservaluelist, eg: if (size == 3 && middleElement == (attached | detached)) ...return false... What do you suggest is the best approach in this case?

(In reply to comment #9) > (In reply to comment #8) > > Good comments, will apply them in the next revision. > > > > Source/WebCore/css/CSSParser.cpp:8812 > > > + bool allowFirstNumber = true; > > > + bool allowSecondNumber = false; > > > + bool allowAttachment = true; > > > > How about breaking up the parsing of the mesh dimensions in a helper function "parseMeshDimensions". That function will parse 1 or 2 numbers or return false. > > I thought about this already but I disregarded the idea, mostly for readability and slightly increased complexity in dealing with cases such as "1 attached 1": in such a case I'd have to perform one more check before cycling on the parservaluelist, eg: if (size == 3 && middleElement == (attached | detached)) ...return false... > > What do you suggest is the best approach in this case? I don't think the case "1 attached 1" is necessarily an issue. A parseGridDimensions should simplify things because you only need to accept the following cases in the calling function: d d c c d Where: d: grid dimensions (either "1" or "1 1") c: mesh connectivity (either "attached" or "detached") Here's some pseudocode showing how a parseGridDimensions function could make the code easier to follow. You might not even have to check the size if you use this approach. Note the assumption is that parseGridDimensions succeeds and advances arg or fails and returns false. --- bool hasDimensions = false; bool hasConnectivity = false; while(arg) { if (hasDimensions && hasConnectivity) { // We shouldn't see any more arguments if we've already parsed the grid dimensions and connectivity. return false; } else if (value is attached or detached) { hasConnectivity = true; accept the value advance arg } else if (parseGridDimensions(...)) { hasDimensions = true; } else return false; } // Only dimensions are required. return hasDimensions; --- Do you think you could try that approach?

(In reply to comment #10) > Do you think you could try that approach? I can surely QA-approve somebody else's code.;) The question here is simple: are we sure we want yet another one-consumer-only ad hoc function in the parser which doesn't necessarily improve readability?

(In reply to comment #11) > (In reply to comment #10) > > > Do you think you could try that approach? > > I can surely QA-approve somebody else's code.;) > > The question here is simple: are we sure we want yet another one-consumer-only ad hoc function in the parser which doesn't necessarily improve readability? I think breaking up this large function into two smaller functions improves readability. Functions don't need to have multiple consumers in order to be justified. In your current patch, I'm not a fan of managing control flow so significantly with three booleans. I find it difficult to verify the correctness of the code by reading it. Factoring out a function would help me verify the correctness of the code one small piece at a time.

Created attachment 197507 [details] Patch

Looks like this will most likely conflict given the recent and unadvertised patch for mix() from bug 114414 coming from the same reviewer. I'm gonna wait for the review outcome before rebasing.

Comment on attachment 197507 [details] Patch Thanks very much for making the changes! I'm finding this version much easier to follow. Just one more nit to make it perfect... :) View in context: https://bugs.webkit.org/attachment.cgi?id=197507&action=review > Source/WebCore/css/CSSParser.cpp:8866 > + if (arg && validUnit(arg, FPositiveInteger)) I like the way you exited early at the beginning of the function. Could you rearrange the conditions at the end here to look similar? It would look like this: // If the next argument is not numeric, we are done parsing the grid dimensions. if (!arg || !validUnit(arg, FPositiveInteger)) return true; // Commit the second numeric value we found. gridValueList->append(createPrimitiveNumericValue(arg)); args->next(); return true;

Created attachment 197639 [details] The Perfect One

Comment on attachment 197639 [details] The Perfect One Indeed, it is perfect. Ping krit for his opinion.

Comment on attachment 197639 [details] The Perfect One r=me Great patch!

The commit-queue encountered the following flaky tests while processing attachment 197639 [details]: media/event-queue-crash.html bug 114177 (author: shinyak@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 197639 [details] The Perfect One Clearing flags on attachment: 197639 Committed r148220: <http://trac.webkit.org/changeset/148220>

All reviewed patches have been landed. Closing bug.

Alas, this "Perfect One" was not perfect. See the change you made to Source/WebCore/css/CSSParser.cpp. Now FontStretch is parsed as shader geometry. https://bugs.webkit.org/show_bug.cgi?id=116370