Closed
Bug 1375555
Opened 7 years ago
Closed 7 years ago
stylo: Users of StyleVariables() in nsComputedDOMStyle need to be fixed
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jdm, Assigned: ferjm)
References
Details
Attachments
(3 files, 1 obsolete file)
Bug 1336891 only fixed one caller, but there are two others in the file that will need fixing.
Comment 1•7 years ago
|
||
Right, sorry for overlooking it, but we also need to fix IndexedGetter and Length.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ferjmoreno
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-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 ::: 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 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-review-reply |
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 9•7 years ago
|
||
mozreview-review |
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 10•7 years ago
|
||
mozreview-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+
Comment 11•7 years ago
|
||
Also, please add an assertion in StyleVariables() that !mComputedStyle->IsServo()
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-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/#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 17•7 years ago
|
||
mozreview-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 18•7 years ago
|
||
mozreview-review |
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!`?
Comment 19•7 years ago
|
||
Awesome, thanks for fixing this! \o/
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8881277 -
Attachment is obsolete: true
Comment 31•7 years ago
|
||
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
Comment 32•7 years ago
|
||
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)
Comment 33•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/795b780b2c3e https://hg.mozilla.org/mozilla-central/rev/3910a0d799d0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(ferjmoreno)
You need to log in
before you can comment on or make changes to this bug.
Description
•