Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly. r?emilio draft
authorXidorn Quan <me@upsuper.org>
Wed, 11 Jul 2018 10:54:47 +1000 (2018-07-11)
changeset 816375 00bd38dd57c5b5afbb5ad189a74c4ccc557312e3
parent 816374 f11561ab9199ce1e640ceac79b2abf4dcfcfdb87
child 817117 fca2dadc3e6407da6a63a5e588f1063135a95461
push id115819
push userxquan@mozilla.com
push dateWed, 11 Jul 2018 03:46:48 +0000 (2018-07-11)
reviewersemilio
bugs1461285
milestone63.0a1
Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly. r?emilio MozReview-Commit-ID: 6BzPTNjXjyA
modules/libpref/init/StaticPrefList.h
servo/components/style/properties/declaration_block.rs
servo/ports/geckolib/glue.rs
testing/web-platform/meta/css/cssom/cssstyledeclaration-setter-order.html.ini
--- a/modules/libpref/init/StaticPrefList.h
+++ b/modules/libpref/init/StaticPrefList.h
@@ -270,16 +270,34 @@ VARCACHE_PREF(
 #endif
 VARCACHE_PREF(
   "layout.css.frames-timing.enabled",
    layout_css_frames_timing_enabled,
   bool, PREF_VALUE
 )
 #undef PREF_VALUE
 
+// When the pref is true, CSSStyleDeclaration.setProperty always appends
+// new declarations (and discards old ones if they exist), otherwise, it
+// will update in-place when given property exists in the block, and
+// avoid updating at all when the existing property declaration is
+// identical to the new one.
+// See bug 1415330, bug 1460295, and bug 1461285 for some background.
+#ifdef RELEASE_OR_BETA
+# define PREF_VALUE false
+#else
+# define PREF_VALUE true
+#endif
+VARCACHE_PREF(
+  "layout.css.property-append-only",
+   layout_css_property_append_only,
+   bool, PREF_VALUE
+)
+#undef PREF_VALUE
+
 // Should the :visited selector ever match (otherwise :link matches instead)?
 VARCACHE_PREF(
   "layout.css.visited_links_enabled",
    layout_css_visited_links_enabled,
   bool, true
 )
 
 // Pref to control whether @-moz-document rules are enabled in content pages.
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -42,18 +42,22 @@ impl AnimationRules {
 /// Enum for how a given declaration should be pushed into a declaration block.
 #[derive(Clone, Copy, Debug, Eq, Hash, MallocSizeOf, PartialEq)]
 pub enum DeclarationPushMode {
     /// Mode used when declarations were obtained from CSS parsing.
     /// If there is an existing declaration of the same property with a higher
     /// importance, the new declaration will be discarded. Otherwise, it will
     /// be appended to the end of the declaration block.
     Parsing,
+    /// In this mode, if there is an existing declaration of the same property,
+    /// the value is updated in-place. Otherwise it's appended. This is one
+    /// possible behavior of CSSOM.
+    Update,
     /// In this mode, the new declaration is always pushed to the end of the
-    /// declaration block. This is for CSSOM.
+    /// declaration block. This is another possible behavior of CSSOM.
     Append,
 }
 
 /// A declaration [importance][importance].
 ///
 /// [importance]: https://drafts.csswg.org/css-cascade/#importance
 #[derive(Clone, Copy, Debug, Eq, Hash, MallocSizeOf, PartialEq)]
 pub enum Importance {
@@ -409,20 +413,59 @@ impl PropertyDeclarationBlock {
             }
             Err(longhand_or_custom) => {
                 // Step 3
                 self.get(longhand_or_custom).map_or(Importance::Normal, |(_, importance)| importance)
             }
         }
     }
 
+    /// Returns whether the property is definitely new for this declaration
+    /// block. It returns true when the declaration is a non-custom longhand
+    /// and it doesn't exist in the block, and returns false otherwise.
+    #[inline]
+    fn is_definitely_new(&self, decl: &PropertyDeclaration) -> bool {
+        match decl.id() {
+            PropertyDeclarationId::Longhand(id) => !self.longhands.contains(id),
+            PropertyDeclarationId::Custom(..) => false,
+        }
+    }
+
+    /// Returns whether calling extend with `DeclarationPushMode::Update`
+    /// will cause this declaration block to change.
+    pub fn will_change_in_update_mode(
+        &self,
+        source_declarations: &SourcePropertyDeclaration,
+        importance: Importance,
+    ) -> bool {
+        // XXX The type of parameter seems to be necessary because otherwise
+        // the compiler complains about `decl` not living long enough in the
+        // all_shorthand expression. Why?
+        let needs_update = |decl: &_| {
+            if self.is_definitely_new(decl) {
+                return true;
+            }
+            self.declarations.iter().enumerate()
+                .find(|&(_, ref slot)| slot.id() == decl.id())
+                .map_or(true, |(i, slot)| {
+                    let important = self.declarations_importance[i];
+                    *slot != *decl || important != importance.important()
+                })
+        };
+        source_declarations.declarations.iter().any(&needs_update) ||
+            source_declarations.all_shorthand.declarations().any(|decl| needs_update(&decl))
+    }
+
     /// Adds or overrides the declaration for a given property in this block.
     ///
     /// See the documentation of `push` to see what impact `source` has when the
     /// property is already there.
+    ///
+    /// When calling with `DeclarationPushMode::Update`, this should not change
+    /// anything if `will_change_in_update_mode` returns false.
     pub fn extend(
         &mut self,
         mut drain: SourcePropertyDeclarationDrain,
         importance: Importance,
         mode: DeclarationPushMode,
     ) -> bool {
         match mode {
             DeclarationPushMode::Parsing => {
@@ -468,26 +511,17 @@ impl PropertyDeclarationBlock {
     ///
     /// Returns whether the declaration has changed.
     pub fn push(
         &mut self,
         declaration: PropertyDeclaration,
         importance: Importance,
         mode: DeclarationPushMode,
     ) -> bool {
-        let longhand_id = match declaration.id() {
-            PropertyDeclarationId::Longhand(id) => Some(id),
-            PropertyDeclarationId::Custom(..) => None,
-        };
-
-        let definitely_new = longhand_id.map_or(false, |id| {
-            !self.longhands.contains(id)
-        });
-
-        if !definitely_new {
+        if !self.is_definitely_new(&declaration) {
             let mut index_to_remove = None;
             for (i, slot) in self.declarations.iter_mut().enumerate() {
                 if slot.id() != declaration.id() {
                     continue;
                 }
 
                 if matches!(mode, DeclarationPushMode::Parsing) {
                     let important = self.declarations_importance[i];
@@ -509,31 +543,40 @@ impl PropertyDeclarationBlock {
 
                         if let PropertyDeclaration::Display(new_display) = declaration {
                             if display::should_ignore_parsed_value(old_display, new_display) {
                                 return false;
                             }
                         }
                     }
                 }
+                if matches!(mode, DeclarationPushMode::Update) {
+                    let important = self.declarations_importance[i];
+                    if *slot == declaration && important == importance.important() {
+                        return false;
+                    }
+                    *slot = declaration;
+                    self.declarations_importance.set(i, importance.important());
+                    return true;
+                }
 
                 index_to_remove = Some(i);
                 break;
             }
 
             if let Some(index) = index_to_remove {
                 self.declarations.remove(index);
                 self.declarations_importance.remove(index);
                 self.declarations.push(declaration);
                 self.declarations_importance.push(importance.important());
                 return true;
             }
         }
 
-        if let Some(id) = longhand_id {
+        if let PropertyDeclarationId::Longhand(id) = declaration.id() {
             self.longhands.insert(id);
         }
         self.declarations.push(declaration);
         self.declarations_importance.push(importance.important());
         true
     }
 
     /// Returns the first declaration that would be removed by removing
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -3578,26 +3578,43 @@ fn set_property(
         quirks_mode,
         reporter.as_ref().map(|r| r as &ParseErrorReporter),
     );
 
     if result.is_err() {
         return false;
     }
 
+    let importance = if is_important { Importance::Important } else { Importance::Normal };
+    let append_only = unsafe {
+        structs::StaticPrefs_sVarCache_layout_css_property_append_only
+    };
+    let mode = if append_only {
+        DeclarationPushMode::Append
+    } else {
+        let will_change = read_locked_arc(declarations, |decls: &PropertyDeclarationBlock| {
+            decls.will_change_in_update_mode(&source_declarations, importance)
+        });
+        if !will_change {
+            return false;
+        }
+        DeclarationPushMode::Update
+    };
+
     before_change_closure.invoke();
 
-    let importance = if is_important { Importance::Important } else { Importance::Normal };
-    write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
+    let result = write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
         decls.extend(
             source_declarations.drain(),
             importance,
-            DeclarationPushMode::Append
+            mode,
         )
-    })
+    });
+    debug_assert!(result);
+    true
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_DeclarationBlock_SetProperty(
     declarations: RawServoDeclarationBlockBorrowed,
     property: *const nsACString,
     value: *const nsACString,
     is_important: bool,
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/meta/css/cssom/cssstyledeclaration-setter-order.html.ini
@@ -0,0 +1,2 @@
+[cssstyledeclaration-setter-order.html]
+  prefs: [layout.css.property-append-only:true]