Closed Bug 1375555 Opened 8 years ago Closed 8 years ago

stylo: Users of StyleVariables() in nsComputedDOMStyle need to be fixed

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: jdm, Assigned: ferjm)

References

Details

Attachments

(3 files, 1 obsolete file)

Bug 1375555 - Part 1: Get custom props count from Servo side for nsComputedDOMStyle::GetLength.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1375555 - Part 2: Implement indexed getter for custom property names.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1375555 - Part 3: Update test expectations.
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1336891 only fixed one caller, but there are two others in the file that will need fixing.
Right, sorry for overlooking it, but we also need to fix IndexedGetter and Length.
Assignee: nobody → ferjmoreno
Comment on attachment 8881274 [details] Bug 1375555 - Part 1: Get custom props count from Servo side for nsComputedDOMStyle::GetLength. https://reviewboard.mozilla.org/r/152488/#review157648 ::: servo/components/style/custom_properties.rs:105 (Diff revision 1) > +#[derive(Clone, Debug)] > +pub struct ComputedValuesMap { > + /// Custom property name index. > + pub index: Vec<Name>, > + /// Computed values indexed by custom property name. > + pub values: HashMap<Name, ComputedValue>, Can we use a `BTreeMap` instead, which guarantees order?
Comment on attachment 8881275 [details] Bug 1375555 - Part 2: Implement indexed getter for custom property names. https://reviewboard.mozilla.org/r/152490/#review157650
Attachment #8881275 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8881274 [details] Bug 1375555 - Part 1: Get custom props count from Servo side for nsComputedDOMStyle::GetLength. https://reviewboard.mozilla.org/r/152488/#review157648 > Can we use a `BTreeMap` instead, which guarantees order? Hmm... After reading the other patches this won't be great, because you can't index on it, so this is probably fine. I'd like to enforce the invariants a bit more thugh, could we make the fields private, and add `get()`, `len()`, `insert()`, etc... methods to `CustomPropertiesMap` which enforce and assert that each name only appears once in `index`, and only appears there iff it appears also in `values`?
Comment on attachment 8881276 [details] Bug 1375555 - Part 3: Update test expectations. https://reviewboard.mozilla.org/r/152492/#review157652 ::: servo/ports/geckolib/glue.rs:3122 (Diff revision 1) > None => 0, > } > } > + > +#[no_mangle] > +pub extern "C" fn Servo_GetCustomPropertyNameAt(computed_values: ServoComputedValuesBorrowed, nit: put each argument on its own line.
Attachment #8881276 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8881277 [details] Bug 1375555 - Part 4: Update test expectations. https://reviewboard.mozilla.org/r/152494/#review157656
Attachment #8881277 - Flags: review?(emilio+bugs) → review+
Also, please add an assertion in StyleVariables() that !mComputedStyle->IsServo()
Comment on attachment 8881274 [details] Bug 1375555 - Part 1: Get custom props count from Servo side for nsComputedDOMStyle::GetLength. https://reviewboard.mozilla.org/r/152488/#review157934 ::: servo/components/style/custom_properties.rs:120 (Diff revision 2) > + } > + > + /// Insert a computed value if it has not previously been inserted. > + pub fn insert(&mut self, name: &Name, value: ComputedValue) { > + debug_assert!(!self.index.contains(name)); > + if self.index.contains(name) { Let's remove this, there's no need to do a linear search here on release builds. ::: servo/components/style/custom_properties.rs:130 (Diff revision 2) > + self.values.insert(name.clone(), value); > + } > + > + /// Custom property computed value getter by name. > + pub fn get_computed_value(&self, name: &Name) -> Option<&ComputedValue> { > + if self.index.contains(name) { Let's do: ``` let value = self.values.get(name); debug_assert_eq!(value.is_some(), self.index.contains(name)); value ``` instead, so we don't need to go through all the property names in release builds.
Attachment #8881274 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8881276 [details] Bug 1375555 - Part 3: Update test expectations. https://reviewboard.mozilla.org/r/152492/#review157938 ::: servo/components/style/custom_properties.rs:139 (Diff revision 2) > None > } > } > > + /// Get the name of a custom property given its list index. > + pub fn get_name_at(&self, index: u32) -> Option<&Name> nit: brace to the previous line. ::: servo/components/style/custom_properties.rs:140 (Diff revision 2) > } > } > > + /// Get the name of a custom property given its list index. > + pub fn get_name_at(&self, index: u32) -> Option<&Name> > + { this method can be written in a single line with: `self.index.get(index)`. ::: servo/ports/geckolib/glue.rs:3165 (Diff revision 2) > + let custom_properties = match ComputedValues::as_arc(&computed_values).custom_properties() { > + Some(p) => p, > + None => return false, > + }; > + > + let name = unsafe { name.as_mut().unwrap() }; nit: I think this would be clearer if the `let name` is moved down, right before it's used. not a big deal anyway. ::: servo/ports/geckolib/glue.rs:3170 (Diff revision 2) > + let name = unsafe { name.as_mut().unwrap() }; > + let property_name = match custom_properties.get_name_at(index) { > + Some(n) => n, > + None => return false, > + }; > + name.assign_utf8(&property_name.to_string()); nit: `name.assign(&**property_name)` should work, I think.
Comment on attachment 8881275 [details] Bug 1375555 - Part 2: Implement indexed getter for custom property names. https://reviewboard.mozilla.org/r/152490/#review157936 ::: servo/components/style/custom_properties.rs:145 (Diff revision 2) > self.values.iter() > } > + > + /// Get the count of custom properties computed values. > + pub fn len(&self) -> usize { > + debug_assert!(self.values.len() == self.index.len()); nit: `debug_assert_eq!`?
Awesome, thanks for fixing this! \o/
Attachment #8881277 - Attachment is obsolete: true
Pushed by ferjmoreno@gmail.com: https://hg.mozilla.org/integration/autoland/rev/795b780b2c3e Part 1: Get custom props count from Servo side for nsComputedDOMStyle::GetLength. r=emilio https://hg.mozilla.org/integration/autoland/rev/3910a0d799d0 Part 2: Implement indexed getter for custom property names. r=emilio https://hg.mozilla.org/integration/autoland/rev/99dfc776b001 Part 3: Update test expectations. r=emilio
Backed out the test expectations update for failing layout/style/test/test_variables.html on stylo: https://hg.mozilla.org/integration/autoland/rev/30f1e5abaee95da5e1cd234ef9cbb54d3d6353a7 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=99dfc776b001dca817851839f42a21c6f55ca1a5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=110629168&repo=autoland [task 2017-06-29T03:19:09.404756Z] 03:19:09 INFO - TEST-PASS | layout/style/test/test_variables.html | undefined assertion name [task 2017-06-29T03:19:09.406834Z] 03:19:09 INFO - TEST-PASS | layout/style/test/test_variables.html | custom property name returned by item() on style declaration [task 2017-06-29T03:19:09.408684Z] 03:19:09 INFO - TEST-PASS | layout/style/test/test_variables.html | custom property name returned by indexed getter on style declaration [task 2017-06-29T03:19:09.409994Z] 03:19:09 INFO - Buffered messages finished [task 2017-06-29T03:19:09.411016Z] 03:19:09 INFO - TEST-UNEXPECTED-FAIL | layout/style/test/test_variables.html | custom property name returned by item() on computed style - got "--a", expected "--SomeVariableName" [task 2017-06-29T03:19:09.412278Z] 03:19:09 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 [task 2017-06-29T03:19:09.413424Z] 03:19:09 INFO - tests<@layout/style/test/test_variables.html:88:5 [task 2017-06-29T03:19:09.414543Z] 03:19:09 INFO - runTest/<@layout/style/test/test_variables.html:136:32 [task 2017-06-29T03:19:09.415381Z] 03:19:09 INFO - runTest@layout/style/test/test_variables.html:136:3 [task 2017-06-29T03:19:09.416439Z] 03:19:09 INFO - EventListener.handleEvent*prepareTest@layout/style/test/test_variables.html:129:3 [task 2017-06-29T03:19:09.418169Z] 03:19:09 INFO - @layout/style/test/test_variables.html:141:1 [task 2017-06-29T03:19:09.418933Z] 03:19:09 INFO - Not taking screenshot here: see the one that was previously logged
Flags: needinfo?(ferjmoreno)
https://hg.mozilla.org/mozilla-central/rev/795b780b2c3e https://hg.mozilla.org/mozilla-central/rev/3910a0d799d0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1377115
Blocks: 1377068
Flags: needinfo?(ferjmoreno)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: