Open
Bug 1375234
Opened 7 years ago
Updated 2 years ago
Repetitive code in style::properties::PropertyDeclaration::parse_into
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
NEW
People
(Reporter: away, Unassigned)
References
Details
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
FWIW, this is where Gecko does the check: https://dxr.mozilla.org/mozilla-central/rev/0b5603017c25e943ba3ab97cb46d88adf1e6a3e4/layout/style/nsCSSProps.h#657-676
(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?
Flags: needinfo?(xidorn+moz)
Comment 5•7 years ago
|
||
No, I don't currently have plan to work on this.
Flags: needinfo?(xidorn+moz)
Updated•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
status-firefox57:
--- → wontfix
status-firefox58:
--- → affected
Updated•7 years ago
|
Comment 6•6 years ago
|
||
https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals
status-firefox59:
--- → ?
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•