Details
- Reviewers
emilio zrhoffman - Commits
- rMOZILLACENTRAL85fff1757073: Bug 1841265 - Add InspectorUtils.getCSSRegisteredProperties. r=emilio,zrhoffman.
- Bugzilla Bug ID
- 1841265
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Build Status
Buildable 580825 Build 678755: arc lint + arc unit
Event Timeline
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 8631 | if let Some(multiplier) = component.multiplier() { | |
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 8617 | I guess? I wasn't aware it was available for CSS @property defined ones I'll add ToCss | |
1 defect closed compared to the previous diff 765089.
If you see a problem in this automated review, please report it here.
| servo/components/style/properties_and_values/syntax/mod.rs | ||
|---|---|---|
| 200 | If you implement ToCss for DataType, this whole match can become self.name().to_css(dest)?; | |
| 202 | An unchecked .ok() throws away the write_str result, but we should abort (using the ? operator would work) if write_str returns an Err. | |
| 205–212 | Rather than checking if self.multiplier() is Some or None, it can be unconditionally written if ToCss is implemented for Multiplier. | |
| servo/ports/geckolib/glue.rs | ||
| 8623–8625 | This can be just component.to_css_string() but no big deal. | |
Thanks for the review @zrhoffman :)
I tried to address all your comments, the test is still fine and I don't see any compiling error, so hopefully this is good
Thanks! Just 1 small nit, and then it looks good :)
| servo/components/style/properties_and_values/syntax/mod.rs | ||
|---|---|---|
| 227 | nit: Rather than writing out the impl, ComponentName can just derive ToCss. | |
| servo/components/style/properties_and_values/syntax/mod.rs | ||
|---|---|---|
| 227 | ah thanks, I was wondering how this could be done :) | |
| dom/chrome-webidl/InspectorUtils.webidl | ||
|---|---|---|
| 84 | This only gets the script ones tho, is that what you want? Or do you also want the @property-registered ones? | |
| servo/ports/geckolib/glue.rs | ||
| 8602 | It'd be more efficient to make these nsCString, and the WebIDL bit UTF8String... | |
| 8624 | Let's just implement ToCss for the syntax. That way you don't allocate all these temporary strings. | |
| 8627 | You can use nsCString here and avoid a copy below. | |
| 8637 | This would become just initial_value,. | |
| dom/chrome-webidl/InspectorUtils.webidl | ||
|---|---|---|
| 84 | Yes, I'd like those too, I was thinking it wasn't implemented yet. Is this behind another pref? | |
| dom/chrome-webidl/InspectorUtils.webidl | ||
|---|---|---|
| 83 | There's no point in adding [Pref] for chrome-only stuff imo. If the pref is disabled we'd just return the empty array which seems fine? | |
| 84 | No. Those live in the relevant CascadeData object: https://searchfox.org/mozilla-central/rev/48b6992e03fa66f77ac9688ba61c95d31a451bc1/servo/components/style/stylist.rs#2360 | |
| servo/components/style/properties_and_values/syntax/mod.rs | ||
|---|---|---|
| 78–85 | I'm not sure about returning Ok(()) here. Ideally I'd return the results from the to_css, write_str, but I wasn't able to get something working with the for loop | |
| servo/ports/geckolib/cbindgen.toml | ||
| 299 | Not sure about this change, but it made the compiler happy and the test runs fine, so I guess it's good | |
| servo/ports/geckolib/glue.rs | ||
| 8603–8604 | Added this so the UI can differentiate between CSS.registerProperty and @property | |
| 8656 | I'm probably iterating over too much things? I might add a boolean to the function to filter out/include user agent stylesheet ? | |
| 8658–8663 | I tried using iter() but the value was a SmallVec and so I had to do more work to extract the property_registration. Maybe I'm doing something wrong here? | |
| servo/components/style/properties_and_values/syntax/mod.rs | ||
|---|---|---|
| 78–85 | This is totally fine. If something has errored you return it above via the ? operator. Just make this Ok(()) (not return Ok(());) since it's the last expression in the block and rust returns it for you. | |
| 149 | nit: .write_char( and use single quotes below for slightly more performant code. | |
| servo/ports/geckolib/glue.rs | ||
| 8615 | Same here. | |
| 8620 | let mut initial_value = nsCString::new(); property_registration.initial_value.to_css(&mut initial_value).unwrap(); Saves a copy. Alternatively add a to_css_nscstring that does it for you. | |
| 8651 | nit: /* from_js = */ true is the more common style. | |
| 8656 | Just for (cascade_data, _origin) in stylist.iter_origins() | |
| 8658 | nit: custom_property_map | |
| 8658–8663 | Just use the last value, which is the valid registration. | |
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 8620 | let mut initial_value = nsCString::new(); property_registration.initial_value.to_css(&mut initial_value).unwrap(); does not compile as to_css takes a CssWriter, not a nsCString. let mut initial_value = nsCString::new(); property_registration .initial_value .to_css(&mut CssWriter::new(&mut initial_value)) .unwrap(); ? I went with this, let me know if that's not what you meant | |
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 8651 | I didn't think about those indeed. I may have a follow up for them | |