989560 - remove most uses of CheckEndProperty/ExpectEndProperty from the CSS parser

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Since bug 125246 introduced mTempData and fully-fixed the problem of writing to a declaration on a failed parse of a property, we haven't needed to use CheckEndProperty()/ExpectEndProperty() in property parsing functions.  Using it also tends to be bad practice:  it leads to parsing code that's less reusable than it should be because it's looking for what it doesn't accept rather than (as it should) looking for what it does accept.

I have patches to remove *most* remaining uses of CheckEndProperty()/ExpectEndProperty() from the CSS parser, although not all of them.

In most cases this tends to reduce code size as well.

(Simon also has another one in bug 988780.)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Make it an error to have an empty item within an animation or transition shorthand

This is needed to make the test in the next patch (which tests that we
reject entirely-empty properties) pass.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Don't accept an empty value for font-variant-alternates

This is needed to prevent a test failure with the next patch (which
removes the ExpectEndProperty check from the CSS_PROPERTY_PARSE_VALUE
case in CSSParserImpl::ParseProperty(nsCSSProperty) since its callers
handle checking for appropriate endings), since the way this function
returns success for empty values leads var() functions alone inside
font-variant-alternates to be rejected, since
CSSParserImpl::ParseProperty(nsCSSProperty) won't try reparsing with
variables.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4 - Remove most uses of CheckEndProperty()/ExpectEndProperty()

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Add one more ExpectEndProperty() call that is needed for variables

This adds a check that is currently present in most but not all
codepaths leading to this point, but which patch 4 will remove from many
of those codepaths.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #2)
> Created attachment 8398844 [details] [diff] [review]
> patch 2 - Don't accept an empty value for font-variant-alternates
> 
> This is needed to prevent a test failure with the next patch (which
> removes the ExpectEndProperty check from the CSS_PROPERTY_PARSE_VALUE
> case in CSSParserImpl::ParseProperty(nsCSSProperty) since its callers
> handle checking for appropriate endings), since the way this function
> returns success for empty values leads var() functions alone inside
> font-variant-alternates to be rejected, since
> CSSParserImpl::ParseProperty(nsCSSProperty) won't try reparsing with
> variables.

I've adjusted this message to read:

Had patch 3 not been present, this would be needed to prevent a test
failure with patch 4 (which removes the ExpectEndProperty check from the
CSS_PROPERTY_PARSE_VALUE case in
CSSParserImpl::ParseProperty(nsCSSProperty) since its callers handle
checking for appropriate endings), since the way this function returns
success for empty values leads var() functions alone inside
font-variant-alternates to be rejected, since
CSSParserImpl::ParseProperty(nsCSSProperty) won't try reparsing with
variables.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8398845 [details] [diff] [review]
patch 4 - Remove most uses of CheckEndProperty()/ExpectEndProperty()

Review of attachment 8398845 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +6906,5 @@
>    nsCSSValue values[numProps];
>  
>    int32_t found = ParseChoice(values, kFlexFlowSubprops, numProps);
>  
>    // Bail if we didn't successfully parse anything, or if there's trailing junk.

Remove the second half of this comment now that we don't call ExpectEndProperty.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8398856 [details] [diff] [review]
patch 3 - Add one more ExpectEndProperty() call that is needed for variables

Review of attachment 8398856 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +8493,5 @@
> +    // 4px 5px var(a)').
> +    //
> +    // It would be nice to find a better solution here
> +    // (and for the SkipUntilOneOf below), though, that doesn't depend
> +    // on using what we don't accept for doing parsing correctly.

Yeah, not sure of a better way to handle this without peeking to see what the next token is.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9742b43dc955
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8a2f866795
https://hg.mozilla.org/integration/mozilla-inbound/rev/59e4c490f4b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/05cf275996a2

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9742b43dc955
https://hg.mozilla.org/mozilla-central/rev/7a8a2f866795
https://hg.mozilla.org/mozilla-central/rev/59e4c490f4b2
https://hg.mozilla.org/mozilla-central/rev/05cf275996a2