Details
- Reviewers
emilio - Group Reviewers
Restricted Project - Commits
- rFIREFOXAUTOLAND23e0d2dcbb7d: Bug 1986629 - Lookup element attribute custom properties for css attr().
- Bugzilla Bug ID
- 1986629
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
- Build Status
Buildable 882502 Build 983511: arc lint + arc unit
Event Timeline
| servo/components/style/custom_properties.rs | ||
|---|---|---|
| 1032 | nit: This style is fine, but you can use Option<impl TElement> if you fancy nowadays (and remove the generics) | |
| 2118–2119 | Fairly sure you need this chunk as well right? Maybe factor it out like: let substitution: Option<_> = match reference.substitution_kind { Var => { let registration = stylist.get_custom_property_registration(&reference.name); custom_properties.get(registration, &reference.name).map(|v| Substitution::from_value(v.to_variable_value())) }, Env => { let device = stylist.device(); device.environment().get(&reference.name, device, url_data).map(Substitution::from_value) }, Attr => { ... } }; if let Some(s) = substitution { // Skip references that are inside the outer variable (in fallback for example). while references .next_if(|next_ref| next_ref.end <= reference.end) .is_some() {} return Ok(s); } | |
| 2126 | using local_name you avoid the to_string here, and this can be: if let Some(attr) = element.and_then(|e| e.get_attr(&reference.name)) {
...
} | |
| 2127 | This does the right thing but in a very expensive way. Instead, change Substitution::new to take a Cow<'a, str> and pass a Cow::Owned(attr) here. | |
What's the testing story here? I guess you're just stashing the string directly in there which probably doesn't do what you want. But seems ok for now, as we need to implement the whole type() stuff.
| servo/components/style/properties/declaration_block.rs | ||
|---|---|---|
| 365 | Yeah, exactly the code-path I was concerned about hah | |
| 1017 | This is not Gecko-specific so might need some changes for servo, cc @Oriol Maybe we should have some sort of AttrProvider trait / struct or something that returned None for all attributes, and another that just wrapped a TElement. | |
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!
| servo/components/style/properties/declaration_block.rs | ||
|---|---|---|
| 1017 | Yes, please avoid using GeckoElement in non-Gecko-specific code. This AttrProvider idea seems reasonable. | |