Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly. r?emilio
MozReview-Commit-ID: 6BzPTNjXjyA
--- 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]