988780 - Avoid CheckEndProperty() in ParseGridTemplateAfterString() (layout/style/nsCSSParser.cpp)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Simon Sapin (:SimonSapin)]

Per IRC discussion, refactor ParseGridTemplateAfterString() to not use CheckEndProperty().

Depends on prior refactoring done in bug 983175 but not landed yet as of this writing.

> @dbaron: SimonSapin, I noticed some of the grid parsing code used CheckEndProperty()/ExpectEndProperty()
> @dbaron: SimonSapin, that's almost never the right thing to use -- I'm mostly trying to get rid of those functions
> @dbaron: SimonSapin, In the first case I looked at the call could just be removed
> @dbaron: SimonSapin, since the code that calls the property-parsing code checks for garbage after what's parsed, so the individual property parsing functions don't need to
> SimonSapin: dbaron: yes, in some cases I used it for longhands and had to refactor to remove it to reuse the functions un shorthands
> SimonSapin: dbaron: what remaining case are you looking at?
> @dbaron: oh, I guess ParseFlexFlow wasn't something you added
> @dbaron: but ParseGridTemplateAfterString is
> @dbaron: though that might be harder to fix
> @dbaron: ParseFlexFlow was the one that looked really easy to fix
> @dbaron: SimonSapin, ^
> SimonSapin: yes, ParseFlexFlow is for Flexbox (I think)
> SimonSapin: ParseGridTemplateAfterString is for the 'grid' shorthand, so I’m fairly confident that it’ll not be used in a yet "shorter-hand"
> SimonSapin: I get the idea of why CheckEndProperty is fragile, but how is it harmful in practice? Other than to reuse code for shorthands
> @dbaron: SimonSapin, also reusing code in other contexts
> @dbaron: SimonSapin, e.g., declarations vs. @supports
> SimonSapin: the fix for that was to have CheckEndProperty check for ')', wasn’t it?
> SimonSapin: this is what I mean by fragile: it needs to be updated when we have a new such context
> SimonSapin: dbaron: Is it this fragility (potential bugs) that you want to get rid of, rather than current bugs?
> @dbaron: SimonSapin, that, and often smaller code size
> @dbaron: SimonSapin, and also just generally teaching people the wrong way to write parsing functions
> SimonSapin: ok
> SimonSapin: I think I see how to remove it in ParseGridTemplateAfterString
> SimonSapin: dbaron: Do you want me to do that? In a bug just for this, or for CheckEndProperty() in general?
> @dbaron: SimonSapin, if you want to -- I was just going through and removing some of the easy ones myself, but I didn't do that one
> @dbaron: SimonSapin, it's not really that important, though, but I want to stop the pattern from spreading.

Comment 1

[Simon Sapin (:SimonSapin)]

Attachment: Patch

Comment 2

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

Comment on attachment 8397715 [details] [diff] [review]
Patch

r=dbaron

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/728e346fe9c9

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/728e346fe9c9