Bug 110815

Summary: [CSS Shaders] Parse the geometry descriptor
Product: WebKit Reporter: Michelangelo De Simone <michelangelo>
Component: CSSAssignee: Michelangelo De Simone <michelangelo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, esprehn+autocc, macpherson, menard, mvujovic, ojan.autocc, syoichi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#geometry
Bug Depends on:    
Bug Blocks: 113695    
Attachments:
Description Flags
Patch not for formal review
none
Patch not for formal review
none
Patch
none
Patch
none
The Perfect One none

Description Michelangelo De Simone 2013-02-25 15:44:13 PST
Support to parse such descriptor is missing and shall be added.
Comment 1 Michelangelo De Simone 2013-03-27 12:11:24 PDT
Created attachment 195374 [details]
Patch not for formal review
Comment 2 Michelangelo De Simone 2013-03-29 03:32:06 PDT
Created attachment 195717 [details]
Patch not for formal review
Comment 3 Dirk Schulze 2013-03-29 03:41:35 PDT
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 4 Dirk Schulze 2013-03-29 03:43:27 PDT
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 5 Max Vujovic 2013-03-31 16:08:58 PDT
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 6 Max Vujovic 2013-03-31 16:17:57 PDT
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.
Comment 7 Michelangelo De Simone 2013-04-02 11:52:03 PDT
Created attachment 196201 [details]
Patch
Comment 8 Dirk Schulze 2013-04-02 15:14:10 PDT
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.
Comment 9 Michelangelo De Simone 2013-04-03 02:17:37 PDT
(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?
Comment 10 Max Vujovic 2013-04-04 15:22:24 PDT
(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?
Comment 11 Michelangelo De Simone 2013-04-08 09:41:07 PDT
(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?
Comment 12 Max Vujovic 2013-04-08 10:07:26 PDT
(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.
Comment 13 Michelangelo De Simone 2013-04-10 22:33:38 PDT
Created attachment 197507 [details]
Patch
Comment 14 Michelangelo De Simone 2013-04-11 09:29:09 PDT
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 15 Max Vujovic 2013-04-11 09:47:37 PDT
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;
Comment 16 Michelangelo De Simone 2013-04-11 10:45:09 PDT
Created attachment 197639 [details]
The Perfect One
Comment 17 Max Vujovic 2013-04-11 11:11:54 PDT
Comment on attachment 197639 [details]
The Perfect One

Indeed, it is perfect. Ping krit for his opinion.
Comment 18 Dirk Schulze 2013-04-11 11:14:36 PDT
Comment on attachment 197639 [details]
The Perfect One

r=me Great patch!
Comment 19 WebKit Commit Bot 2013-04-11 11:51:14 PDT
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 20 WebKit Commit Bot 2013-04-11 11:54:03 PDT
Comment on attachment 197639 [details]
The Perfect One

Clearing flags on attachment: 197639

Committed r148220: <http://trac.webkit.org/changeset/148220>
Comment 21 WebKit Commit Bot 2013-04-11 11:54:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Dean Jackson 2013-05-17 19:11:25 PDT
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