servo: Merge #20582 - Have CSSOM append rather than replace slot in declaration block (from upsuper:cssom-append); r=emilio
authorXidorn Quan <me@upsuper.org>
Sat, 07 Apr 2018 09:18:28 -0400
changeset 412279 62908a56c59f34e4dd5175717228cd514ac65c60
parent 412278 7ba4b70ba892ed1ba575c683408eb378a52aba0b
child 412280 77cedae32cfaaddb7209fdac072e6ecdf78b11d9
push id101872
push userrgurzau@mozilla.com
push dateSat, 07 Apr 2018 22:16:03 +0000
treeherdermozilla-inbound@aacc170ff3f6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1415330
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #20582 - Have CSSOM append rather than replace slot in declaration block (from upsuper:cssom-append); r=emilio The early return for identical setting in importance matching as well as the comment before `index_to_remove` are removed because the order is web-exposing regardless of whether it's from CSSOM or parsing. e.g. `top: 1px; left: 2px; top: 1px;` is effectively `left: 2px; top: 1px;`, not `top: 1px; left: 2px;`. This is patch for [bug 1415330](https://bugzilla.mozilla.org/show_bug.cgi?id=1415330), for spec change in w3c/csswg-drafts#2516. And corresponding test will be added in w3c/web-platform-tests#10354. Source-Repo: https://github.com/servo/servo Source-Revision: ccc9d1c4c2877ebc82158b91dc27e1be74fae237
servo/components/style/properties/declaration_block.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -513,63 +513,43 @@ impl PropertyDeclarationBlock {
         if !definitely_new {
             let mut index_to_remove = None;
             for (i, slot) in self.declarations.iter_mut().enumerate() {
                 if slot.id() != declaration.id() {
                     continue;
                 }
 
                 let important = self.declarations_importance.get(i as u32);
-                match (important, importance.important()) {
-                    (false, true) => {}
+                // For declarations from parsing, non-important declarations
+                // shouldn't override existing important one.
+                if important && !importance.important() &&
+                    matches!(source, DeclarationSource::Parsing) {
+                    return true;
+                }
 
-                    (true, false) => {
-                        // For declarations set from the OM, more-important
-                        // declarations are overridden.
-                        if !matches!(source, DeclarationSource::CssOm) {
-                            return false
+                if matches!(source, DeclarationSource::Parsing) {
+                    // As a compatibility hack, specially on Android,
+                    // don't allow to override a prefixed webkit display
+                    // value with an unprefixed version from parsing
+                    // code.
+                    //
+                    // TODO(emilio): Unship.
+                    if let PropertyDeclaration::Display(old_display) = *slot {
+                        use properties::longhands::display::computed_value::T as display;
+
+                        if let PropertyDeclaration::Display(new_display) = declaration {
+                            if display::should_ignore_parsed_value(old_display, new_display) {
+                                return false;
+                            }
                         }
                     }
-                    _ => if *slot == declaration {
-                        return false;
-                    }
                 }
 
-                match source {
-                    // CSSOM preserves the declaration position, and
-                    // overrides importance.
-                    DeclarationSource::CssOm => {
-                        *slot = declaration;
-                        self.declarations_importance.set(i as u32, importance.important());
-                        return true;
-                    }
-                    DeclarationSource::Parsing => {
-                        // As a compatibility hack, specially on Android,
-                        // don't allow to override a prefixed webkit display
-                        // value with an unprefixed version from parsing
-                        // code.
-                        //
-                        // TODO(emilio): Unship.
-                        if let PropertyDeclaration::Display(old_display) = *slot {
-                            use properties::longhands::display::computed_value::T as display;
-
-                            if let PropertyDeclaration::Display(new_display) = declaration {
-                                if display::should_ignore_parsed_value(old_display, new_display) {
-                                    return false;
-                                }
-                            }
-                        }
-
-                        // NOTE(emilio): We could avoid this and just override for
-                        // properties not affected by logical props, but it's not
-                        // clear it's worth it given the `definitely_new` check.
-                        index_to_remove = Some(i);
-                        break;
-                    }
-                }
+                index_to_remove = Some(i);
+                break;
             }
 
             if let Some(index) = index_to_remove {
                 self.declarations.remove(index);
                 self.declarations_importance.remove(index as u32);
                 self.declarations.push(declaration);
                 self.declarations_importance.push(importance.important());
                 return true;