1375234 - Repetitive code in style::properties::PropertyDeclaration::parse_into

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[(Away)]

There's a bunch of arms like this, I'm guessing one for every CSS property? 

                LonghandId::FlexWrap => {
                            if rule_type == CssRuleType::Page {
                                return Err(PropertyDeclarationParseError::NotAllowedInPageRule)
                            }
        
        if structs::root::mozilla::SERVO_PREF_ENABLED_flex_wrap {
            let id = structs::
    nsCSSPropertyID::eCSSProperty_flex_wrap;
            let enabled = unsafe { bindings::Gecko_PropertyId_IsPrefEnabled(id) };
            if !enabled {
                return Err(PropertyDeclarationParseError::ExperimentalProperty)
            }
        }
                        match longhands::flex_wrap::parse_declared(context, input) {
                            Ok(value) => {
                                declarations.push(value);
                                Ok(())
                            },
                            Err(_) => Err(PropertyDeclarationParseError::InvalidValue),
                        }
                }


Can the NotAllowedInPageRule be factored out and done upfront? Maybe like
  if rule_type == CssRuleType::Page {
    if <this property doesn't allow it>
     return error
  }
  match (id) { ... }

(I assume that Page stuff is rare, so it doesn't matter if we have to do extra work in the rule_type == CssRuleType::Page case.)

And could the Ok/Err bit also be factored out? Maybe could the 'main' match expression just return the value of parse_declared, and then do the InvalidValue checking once at the end?

Not being able to follow the disassembly exactly, I don't know how smart the compiler is already being and how much it would win us to intervene.

Comment 1

[Josh Matthews [:jdm]]

For the record, I am in the process of modifying the InvalidValue enum variant to contain a property name, so we're still going to need _some_ special code per property even if we move InvalidValue out of the enum match.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

The whole bunch of pref / internal check code is based on an nsCSSPropertyID-based table, which should be much compact in codesize than Servo's approach. We can probably use that.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

FWIW, this is where Gecko does the check: https://dxr.mozilla.org/mozilla-central/rev/0b5603017c25e943ba3ab97cb46d88adf1e6a3e4/layout/style/nsCSSProps.h#657-676

Comment 4

[(Away)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> The whole bunch of pref / internal check code is based on an
> nsCSSPropertyID-based table, which should be much compact in codesize than
> Servo's approach. We can probably use that.

Just to clarify: Do you intend to do this work yourself?

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

No, I don't currently have plan to work on this.

Comment 6

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals