Fix CSSStyleDeclaration::setPropertyPriority and some refactoring #6741

SimonSapin · 2015-07-24T22:37:14Z

hoppipolla-critic-bot · 2015-07-24T22:37:22Z

Critic review: https://critic.hoppipolla.co.uk/r/5646

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

Ms2ger · 2015-07-29T10:51:08Z

Reviewed 1 of 1 files at r1, 6 of 6 files at r2, 4 of 4 files at r3, 4 of 4 files at r4.
Review status: 4 of 9 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

^{components/script/dom/cssstyledeclaration.rs, line 80 [r4] (raw file):}
… so this didn't work out?

^{components/script/dom/cssstyledeclaration.rs, line 81 [r3] (raw file):}
We prefer implementing for &T (or T, perhaps).

^{components/script/dom/cssstyledeclaration.rs, line 81 [r5] (raw file):}
Seriously?

^{components/script/dom/cssstyledeclaration.rs, line 160 [r3] (raw file):}
Why the result local?

^{components/script/dom/cssstyledeclaration.rs, line 235 [r3] (raw file):}
I guess modern Rust would drop the .into_iter() entirely.

^{components/script/dom/cssstyledeclaration.rs, line 271 [r2] (raw file):}
Can you find someone who knows this code?

^{components/style/build.rs, line 26 [r1] (raw file):}
One import per line

^{components/style/properties.mako.rs, line 6186 [r2] (raw file):}
Spurious space after <

^{tests/ref/basic.list, line 280 [r2] (raw file):}
I'd really rather you added this under tests/wpt/mozilla/tests/css/.

Comments from the review on Reviewable.io

SimonSapin · 2015-07-29T19:14:35Z

Review status: 4 of 9 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

^{components/script/dom/cssstyledeclaration.rs, line 81 [r3] (raw file):}
Done.

^{components/script/dom/cssstyledeclaration.rs, line 81 [r4] (raw file):}
This was a messed up cherry-pick. I rewrote the whole branch to clean this up.

^{components/script/dom/cssstyledeclaration.rs, line 160 [r3] (raw file):}
To work around dropck being too conservative. rust-lang/rust#22252 I’v added a FIXME comment.

^{components/script/dom/cssstyledeclaration.rs, line 234 [r3] (raw file):}
Done

^{components/style/build.rs, line 26 [r1] (raw file):}
Done.

^{components/style/properties.mako.rs, line 6194 [r2] (raw file):}
This space works around what seems to be a Mako bug. I’ve added a comment.

^{tests/ref/basic.list, line 280 [r2] (raw file):}
Done.

Comments from the review on Reviewable.io

SimonSapin · 2015-07-29T19:17:58Z

I had done something wrong when cherry-picking commits before submitting this PR, so I rebased/squashed/cherry-picked/massaged the whole thing again.

r?

metajack · 2015-07-29T19:31:26Z

@Ms2ger ping

bors-servo · 2015-07-30T01:43:03Z

☔ The latest upstream changes (presumably #6798) made this pull request unmergeable. Please resolve the merge conflicts.

SimonSapin · 2015-07-30T07:22:01Z

Rebased.

Ms2ger · 2015-07-30T08:07:00Z

Reviewed 4 of 4 files at r5, 2 of 2 files at r6, 12 of 12 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from the review on Reviewable.io

Ms2ger · 2015-07-30T11:16:19Z

@pcwalton or @jdm do either of you feel comfortable reviewing the interaction with style?

jdm · 2015-07-30T11:21:07Z

I could if necessary and with some time spent re-acquainting myself with the code.

bors-servo · 2015-07-31T00:22:07Z

☔ The latest upstream changes (presumably #6835) made this pull request unmergeable. Please resolve the merge conflicts.


        Show Mako template errors on the console


        Fix CSSStyleDeclaration.setPropertyPriority

Before, it was a complicated no-op. (`parse_style_attribute` expects input like `a: b; c: d;`, when given just a name it return an empty vector.)


        Add a test for CSSStyleDeclaration.setPropertyPriority


        Refactor CSSStyleDeclaration::setProperty to not synthesize a `name: …

…value` string to parse.


        Remove some PropertyDeclaration cloning.

SimonSapin · 2015-07-31T06:35:22Z

Rebased.

pcwalton · 2015-08-01T16:16:09Z

@bors-servo: r+

bors-servo · 2015-08-01T16:16:09Z

📌 Commit 0b3e3bc has been approved by pcwalton

bors-servo · 2015-08-01T16:55:54Z

⌛ Testing commit 0b3e3bc with merge 1d2bc86...


        Auto merge of #6741 - servo:fix-setpropertypriority, r=pcwalton

@Ms2ger

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring r? @Ms2ger  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6741)

bors-servo · 2015-08-01T17:14:28Z

💔 Test failed - mac2

SimonSapin · 2015-08-01T19:49:12Z

I didn’t remember why PartialEq was derived on stylesheets. Turns out, it was for unit tests. Carrying @pcwalton’s r+ to the same branch with the last commit removed, reproduced below for posterity.

@bors-servo r=pcwalton 06ba62b

commit 0b3e3bc3e9477e617ba784c4bb9e5b32482d0d04
Author: Simon Sapin <simon.sapin@exyr.org>
Date:   Sat Jul 25 00:43:27 2015 +0200

    Remove PartialEq from CSS stylesheets, rules, and declarations.

    We shouldn’t be doing deep comparison of such large objects.

diff --git a/components/style/properties.mako.rs b/components/style/properties.mako.rs
index 5d15cd1..8b2d635 100644
--- a/components/style/properties.mako.rs
+++ b/components/style/properties.mako.rs
@@ -5522,7 +5522,7 @@ mod property_bit_field {

 /// Declarations are stored in reverse order.
 /// Overridden declarations are skipped.
-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub struct PropertyDeclarationBlock {
     pub important: Arc<Vec<PropertyDeclaration>>,
     pub normal: Arc<Vec<PropertyDeclaration>>,
@@ -5669,7 +5669,6 @@ impl<T: ToCss> DeclaredValue<T> {
     }
 }

-#[derive(PartialEq)]
 pub enum PropertyDeclaration {
     % for property in LONGHANDS:
         ${property.camel_case}(DeclaredValue<longhands::${property.ident}::SpecifiedValue>),
diff --git a/components/style/stylesheets.rs b/components/style/stylesheets.rs
index 3093f8e..d77980d 100644
--- a/components/style/stylesheets.rs
+++ b/components/style/stylesheets.rs
@@ -38,7 +38,7 @@ pub enum Origin {
 }


-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub struct Stylesheet {
     /// List of rules in the order they were found (important for
     /// cascading order)
@@ -47,7 +47,7 @@ pub struct Stylesheet {
 }


-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub enum CSSRule {
     Charset(String),
     Namespace(Option<String>, Namespace),
@@ -57,7 +57,7 @@ pub enum CSSRule {
     Viewport(ViewportRule),
 }

-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub struct MediaRule {
     pub media_queries: MediaQueryList,
     pub rules: Vec<CSSRule>,
@@ -70,7 +70,7 @@ impl MediaRule {
     }
 }

-#[derive(Debug, PartialEq)]
+#[derive(Debug)]
 pub struct StyleRule {
     pub selectors: Vec<Selector>,
     pub declarations: PropertyDeclarationBlock,

bors-servo · 2015-08-01T21:00:07Z

⌛ Testing commit 06ba62b with merge c6b0435...


        Auto merge of #6741 - servo:fix-setpropertypriority, r=pcwalton

@Ms2ger

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring r? @Ms2ger  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6741)

bors-servo · 2015-08-01T21:39:48Z

💔 Test failed - mac3

SimonSapin · 2015-08-01T22:05:55Z

@bors-servo retry

/css21_dev/html4/z-index-012.htm
--------------------------------
TIMEOUT expected FAIL [Parent]

#6892

bors-servo · 2015-08-01T22:05:57Z

⚡ Previous build results for android, gonk, linux1, linux2, linux3, mac1, mac2 are reusable. Rebuilding only mac3...

bors-servo · 2015-08-01T22:53:26Z

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

highfive added the S-awaiting-review label Jul 24, 2015

SimonSapin mentioned this pull request Jul 29, 2015

Optimize `SetProperty` for inline styles #6823

Closed

jdm assigned Ms2ger Jul 29, 2015

jdm mentioned this pull request Jul 29, 2015

Links from github comments don't go anywhere Reviewable/Reviewable#140

Closed

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 29, 2015

SimonSapin force-pushed the fix-setpropertypriority branch from 1cc096f to a8e8211 Jul 29, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 29, 2015

metajack added the S-needs-rebase label Jul 30, 2015

SimonSapin force-pushed the fix-setpropertypriority branch from a8e8211 to 0169858 Jul 30, 2015

Ms2ger added S-awaiting-review and removed S-awaiting-review S-needs-rebase labels Jul 30, 2015

SimonSapin added 5 commits Jul 24, 2015

Show Mako template errors on the console

1094ce2

Fix CSSStyleDeclaration.setPropertyPriority

1033886

Before, it was a complicated no-op. (`parse_style_attribute` expects input like `a: b; c: d;`, when given just a name it return an empty vector.)

Add a test for CSSStyleDeclaration.setPropertyPriority

e298434

Refactor CSSStyleDeclaration::setProperty to not synthesize a `name: …

d2bd070

…value` string to parse.

Remove some PropertyDeclaration cloning.

Loading status checks…

06ba62b

SimonSapin force-pushed the fix-setpropertypriority branch from 0169858 to 0b3e3bc Jul 31, 2015

jdm added S-awaiting-merge and removed S-awaiting-review labels Aug 1, 2015

SimonSapin force-pushed the fix-setpropertypriority branch from 0b3e3bc to 06ba62b Aug 1, 2015

highfive added S-awaiting-review and removed S-awaiting-merge labels Aug 1, 2015

SimonSapin mentioned this pull request Aug 1, 2015

Intermittent timeout /css21_dev/html4/z-index-012.htm #6892

Closed

jdm added S-awaiting-merge and removed S-awaiting-review labels Aug 1, 2015

bors-servo merged commit 06ba62b into master Aug 1, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

SimonSapin deleted the fix-setpropertypriority branch Aug 1, 2015

servo / servo

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring #6741

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring #6741

SimonSapin commented Jul 24, 2015

hoppipolla-critic-bot commented Jul 24, 2015

Ms2ger commented Jul 29, 2015

SimonSapin commented Jul 29, 2015

SimonSapin commented Jul 29, 2015

metajack commented Jul 29, 2015

bors-servo commented Jul 30, 2015

SimonSapin commented Jul 30, 2015

Ms2ger commented Jul 30, 2015

Ms2ger commented Jul 30, 2015

jdm commented Jul 30, 2015

bors-servo commented Jul 31, 2015

SimonSapin commented Jul 31, 2015

pcwalton commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

SimonSapin commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

SimonSapin commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

servo / servo

Join GitHub today

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring #6741

Fix CSSStyleDeclaration::setPropertyPriority and some refactoring #6741

Conversation

SimonSapin commented Jul 24, 2015

hoppipolla-critic-bot commented Jul 24, 2015

Ms2ger commented Jul 29, 2015

SimonSapin commented Jul 29, 2015

SimonSapin commented Jul 29, 2015

metajack commented Jul 29, 2015

bors-servo commented Jul 30, 2015

SimonSapin commented Jul 30, 2015

Ms2ger commented Jul 30, 2015

Ms2ger commented Jul 30, 2015

jdm commented Jul 30, 2015

bors-servo commented Jul 31, 2015

SimonSapin commented Jul 31, 2015

pcwalton commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

SimonSapin commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015

SimonSapin commented Aug 1, 2015

bors-servo commented Aug 1, 2015

bors-servo commented Aug 1, 2015