Bug 110815 – [CSS Shaders] Parse the geometry descriptor

• Component: CSS
• Status: RESOLVED FIXED

[Michelangelo De Simone]

Support to parse such descriptor is missing and shall be added.

[Michelangelo De Simone]

Created attachment 195374 [details]
Patch not for formal review

[Michelangelo De Simone]

Created attachment 195717 [details]
Patch not for formal review

[Dirk Schulze]

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

[Dirk Schulze]

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?

[Max Vujovic]

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.

[Max Vujovic]

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.

[Michelangelo De Simone]

Created attachment 196201 [details]
Patch

[Dirk Schulze]

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.

[Michelangelo De Simone]

(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?

[Max Vujovic]

(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?

[Michelangelo De Simone]

(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?

[Max Vujovic]

(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.

[Michelangelo De Simone]

Created attachment 197507 [details]
Patch

[Michelangelo De Simone]

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.

[Max Vujovic]

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;

[Michelangelo De Simone]

Created attachment 197639 [details]
The Perfect One

[Max Vujovic]

Comment on attachment 197639 [details]
The Perfect One

Indeed, it is perfect. Ping krit for his opinion.

[Dirk Schulze]

Comment on attachment 197639 [details]
The Perfect One

r=me Great patch!

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 197639 [details]:

media/event-queue-crash.html bug 114177 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 197639 [details]
The Perfect One

Clearing flags on attachment: 197639

Committed r148220: <http://trac.webkit.org/changeset/148220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dean Jackson]

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