Merge normal and important declarations in style rules #12943

SimonSapin · 2016-08-19T14:39:57Z

Have a single Vec instead of two. Fix #3426

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Make PropertyDeclarationBlock store a single vector of declarations #3426.

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-08-19T14:40:01Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/properties/properties.mako.rs, components/style/matching.rs, components/style/Cargo.toml, components/style/lib.rs, components/style/properties/data.py, components/style/properties/shorthand/box.mako.rs, components/style/dom.rs, components/style/keyframes.rs, components/style/properties/longhand/box.mako.rs, components/style/animation.rs
@KiChjang: components/script/dom/cssstyledeclaration.rs, components/script/dom/element.rs, components/script_layout_interface/wrapper_traits.rs, components/script/layout_wrapper.rs

highfive · 2016-08-19T14:40:03Z

Warning

These commits modify unsafe code. Please review it carefully!

SimonSapin · 2016-08-19T14:43:20Z

@bors-servo try

bors-servo · 2016-08-19T14:43:23Z

⌛ Trying commit 2f500e9 with merge 95b3287...

Merge normal and important declarations in style rules Have a single Vec instead of two. Fix #3426 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3426.  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12943)

bors-servo · 2016-08-19T14:45:39Z

⛄ The build was interrupted to prioritize another pull request.

bors-servo · 2016-08-19T14:45:44Z

⌛ Trying commit 2f500e9 with merge 0095764...

Merge normal and important declarations in style rules Have a single Vec instead of two. Fix #3426 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3426.  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12943)

bors-servo · 2016-08-20T01:18:26Z

💥 Test timed out

emilio · 2016-08-20T07:52:10Z

components/style/keyframes.rs

-            declarations: declarations.normal,
-        })
-    }
+    pub declarations: Arc<Vec<(PropertyDeclaration, Importance)>>,


Important declarations in keyframes are ignored, so the previous implementation was correct. Please make this just a Vec<PropertyDeclaration>, and point to https://drafts.csswg.org/css-animations/#keyframes:

None of the properties interact with the cascade (so using !important on them is invalid and will cause the
property to be ignored).

style::animation::compute_style_for_animation_step crates a style::selector_matching::DeclarationBlock struct from this. That struct contains Arc<Vec<(PropertyDeclaration, Importance)>>. So having dummy Importance::Normal values in Keyframe enables doing this by cloning an Arc (incrementing a refcount) rather than creating a new Vec.

I’ve added a doc-comment saying this.

emilio · 2016-08-20T08:37:25Z

Arguably should have reviewed commit by commit to avoid leaving so much noise, but it's probably late for that.

The approach looks good to me. Probably @heycam may want to take a look at this too. I'm still confused by the deduplicate_property_declarations thing, but the rest makes sense to me.

@bors-servo: retry

bors-servo · 2016-08-20T08:37:41Z

⌛ Trying commit 2f500e9 with merge 7ed02d2...

Merge normal and important declarations in style rules Have a single Vec instead of two. Fix #3426 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3426.  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12943)

bors-servo · 2016-08-20T09:36:57Z

💔 Test failed - linux-rel

highfive · 2016-08-20T09:37:00Z

  ▶ FAIL [expected PASS] /css-style-attr-1_dev/html4/style-attr-cascade-002.htm
  └   → /css-style-attr-1_dev/html4/style-attr-cascade-002.htm bf25a67d9f07765e90c077aec3051e5effa9ed08
/css-style-attr-1_dev/html4/reference/ref-green.htm 5570f2b94aae1cac828af0c91bc632190eb0f0c1
Testing bf25a67d9f07765e90c077aec3051e5effa9ed08 == 5570f2b94aae1cac828af0c91bc632190eb0f0c1

  ▶ FAIL [expected PASS] /css-variables-1_dev/html/variable-reference-13.htm
  └   → /css-variables-1_dev/html/variable-reference-13.htm bf1aa2f1153c018c97c7fdd1ac77c71ff66e899d
/css-variables-1_dev/html/support/color-green-ref.htm 90de59877a1f34c922b370c162008e33df4c6f30
Testing bf1aa2f1153c018c97c7fdd1ac77c71ff66e899d == 90de59877a1f34c922b370c162008e33df4c6f30

  ▶ FAIL [expected PASS] /css21_dev/html4/style-attr-cascade-002.htm
  └   → /css21_dev/html4/style-attr-cascade-002.htm bf25a67d9f07765e90c077aec3051e5effa9ed08
/css21_dev/html4/reference/ref-green.htm 5570f2b94aae1cac828af0c91bc632190eb0f0c1
Testing bf25a67d9f07765e90c077aec3051e5effa9ed08 == 5570f2b94aae1cac828af0c91bc632190eb0f0c1

SimonSapin · 2016-08-20T13:09:11Z

I’ve pushed fixup! commits, separate for review but to be auto-squashed before merging.

These last three test failures were not intermittent. I could reliably reproduce them locally, and fixing the deduplication vs importance bug that you caught fixed them.

@bors-servo try

bors-servo · 2016-08-20T13:09:13Z

⌛ Trying commit 7b0a881 with merge e3c563c...

emilio · 2016-08-21T01:46:59Z

@bors-servo: r+

bors-servo · 2016-08-21T01:47:04Z

📌 Commit 023a3b7 has been approved by emilio

bors-servo · 2016-08-21T04:13:05Z

⌛ Testing commit 023a3b7 with merge 87fd6f1...

Merge normal and important declarations in style rules Have a single Vec instead of two. Fix #3426 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3426.  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12943)

bors-servo · 2016-08-21T04:59:26Z

💔 Test failed - linux-rel

highfive · 2016-08-21T04:59:29Z

  ▶ FAIL [expected PASS] /_mozilla/css/setpropertypriority.html
  └   → /_mozilla/css/setpropertypriority.html 1c40214a1a913cbd096a44b2d280fb3193fa09d0
/_mozilla/css/setpropertypriority_ref.html f565a85a358e1e10906e0a87eb234acff04e9bfb
Testing 1c40214a1a913cbd096a44b2d280fb3193fa09d0 == f565a85a358e1e10906e0a87eb234acff04e9bfb

emilio · 2016-08-21T05:19:55Z

components/script/dom/element.rs

+                let declarations = Arc::make_mut(&mut declarations.declarations);
+                for &mut (ref declaration, ref mut importance) in declarations {
+                    if properties.iter().any(|p| declaration.name() == **p) {
+                        *importance = new_importance


forgot to update the count in this one, that's why that test is failing, sorry for not catching it before.

… instead of the presence (`bool` flags) of each. This allows removing `recalc_any` which iterated over the `Vec`.

SimonSapin · 2016-08-21T08:43:23Z

@bors-servo r=emilio

bors-servo · 2016-08-21T08:43:24Z

📌 Commit f9150af has been approved by emilio

bors-servo · 2016-08-21T08:43:26Z

⌛ Testing commit f9150af with merge f8b2be1...

Merge normal and important declarations in style rules Have a single Vec instead of two. Fix #3426 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #3426.  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12943)

bors-servo · 2016-08-21T09:34:39Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned metajack Aug 19, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Aug 19, 2016

SimonSapin mentioned this pull request Aug 19, 2016

Drive-by fixes preparing for CSSOM #12927

Closed

5 tasks

SimonSapin force-pushed the merged-declaration-block branch from bc8543d to 2f500e9 Compare August 19, 2016 14:41

emilio reviewed Aug 20, 2016
View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Aug 20, 2016

highfive removed the S-tests-failed The changes caused existing tests to fail. label Aug 20, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Aug 21, 2016

highfive assigned emilio and unassigned metajack Aug 21, 2016

emilio mentioned this pull request Aug 21, 2016

Track only the number of important declarations in a declaration block #12959

Closed

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Aug 21, 2016

emilio reviewed Aug 21, 2016
View reviewed changes

Keep track of the number of important and normal declarations in a block

f9150af

… instead of the presence (`bool` flags) of each. This allows removing `recalc_any` which iterated over the `Vec`.

SimonSapin force-pushed the merged-declaration-block branch from 023a3b7 to f9150af Compare August 21, 2016 08:41

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Aug 21, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 21, 2016

SimonSapin mentioned this pull request Aug 21, 2016

Don’t cancel try builds when the PR is pushed servo/homu#59

Open

bors-servo merged commit f9150af into master Aug 21, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Aug 21, 2016

SimonSapin deleted the merged-declaration-block branch August 21, 2016 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge normal and important declarations in style rules #12943

Merge normal and important declarations in style rules #12943

SimonSapin commented Aug 19, 2016 •

edited by larsbergstrom

Loading

highfive commented Aug 19, 2016

highfive commented Aug 19, 2016

SimonSapin commented Aug 19, 2016

bors-servo commented Aug 19, 2016

bors-servo commented Aug 19, 2016

bors-servo commented Aug 19, 2016

bors-servo commented Aug 20, 2016

emilio Aug 20, 2016

SimonSapin Aug 20, 2016

emilio commented Aug 20, 2016 •

edited

Loading

bors-servo commented Aug 20, 2016

bors-servo commented Aug 20, 2016

highfive commented Aug 20, 2016

SimonSapin commented Aug 20, 2016 •

edited

Loading

bors-servo commented Aug 20, 2016

emilio commented Aug 21, 2016 •

edited

Loading

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

highfive commented Aug 21, 2016

emilio Aug 21, 2016

SimonSapin commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

Merge normal and important declarations in style rules #12943

Merge normal and important declarations in style rules #12943

Conversation

SimonSapin commented Aug 19, 2016 • edited by larsbergstrom Loading

highfive commented Aug 19, 2016

highfive commented Aug 19, 2016

SimonSapin commented Aug 19, 2016

bors-servo commented Aug 19, 2016

bors-servo commented Aug 19, 2016

bors-servo commented Aug 19, 2016

bors-servo commented Aug 20, 2016

emilio Aug 20, 2016

Choose a reason for hiding this comment

SimonSapin Aug 20, 2016

Choose a reason for hiding this comment

emilio commented Aug 20, 2016 • edited Loading

bors-servo commented Aug 20, 2016

bors-servo commented Aug 20, 2016

highfive commented Aug 20, 2016

SimonSapin commented Aug 20, 2016 • edited Loading

bors-servo commented Aug 20, 2016

emilio commented Aug 21, 2016 • edited Loading

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

highfive commented Aug 21, 2016

emilio Aug 21, 2016

Choose a reason for hiding this comment

SimonSapin commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

SimonSapin commented Aug 19, 2016 •

edited by larsbergstrom

Loading

emilio commented Aug 20, 2016 •

edited

Loading

SimonSapin commented Aug 20, 2016 •

edited

Loading

emilio commented Aug 21, 2016 •

edited

Loading