Bug 1473180 part 5 - Remove DeclarationPushMode. r=emilio
authorXidorn Quan <me@upsuper.org>
Thu, 19 Jul 2018 10:11:04 +1000
changeset 484997 d487d2a5bc3eefcb7ca4db26426e4f3f100e4029
parent 484996 75345fe6e02d9f90fc3266a432205528d85eeccf
child 484998 5a79aaf6a5ce27eb5a70a08d943b585cf3885673
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1473180
milestone63.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
Bug 1473180 part 5 - Remove DeclarationPushMode. r=emilio MozReview-Commit-ID: LFgYeKE1SNk
servo/components/style/properties/declaration_block.rs
servo/components/style/stylesheets/keyframes_rule.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/properties/declaration_block.rs
+++ b/servo/components/style/properties/declaration_block.rs
@@ -59,29 +59,16 @@ enum DeclarationUpdate {
 /// a `SourcePropertyDeclaration`.
 #[derive(Default)]
 pub struct SourcePropertyDeclarationUpdate {
     updates: SubpropertiesVec<DeclarationUpdate>,
     new_count: usize,
     any_removal: bool,
 }
 
-/// 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, the new declaration is always pushed to the end of the
-    /// 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 {
     /// Indicates a declaration without `!important`.
     Normal,
 
@@ -467,96 +454,74 @@ impl PropertyDeclarationBlock {
     /// 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.
     pub fn extend(
         &mut self,
         mut drain: SourcePropertyDeclarationDrain,
         importance: Importance,
-        mode: DeclarationPushMode,
     ) -> bool {
-        match mode {
-            DeclarationPushMode::Parsing => {
-                let all_shorthand_len = match drain.all_shorthand {
-                    AllShorthand::NotSet => 0,
-                    AllShorthand::CSSWideKeyword(_) |
-                    AllShorthand::WithVariables(_) => shorthands::ALL_SHORTHAND_MAX_LEN,
-                };
-                let push_calls_count =
-                    drain.declarations.len() + all_shorthand_len;
+        let all_shorthand_len = match drain.all_shorthand {
+            AllShorthand::NotSet => 0,
+            AllShorthand::CSSWideKeyword(_) |
+            AllShorthand::WithVariables(_) => shorthands::ALL_SHORTHAND_MAX_LEN,
+        };
+        let push_calls_count =
+            drain.declarations.len() + all_shorthand_len;
 
-                // With deduplication the actual length increase may be less than this.
-                self.declarations.reserve(push_calls_count);
-            }
-            _ => {
-                // Don't bother reserving space, since it's usually the case
-                // that CSSOM just overrides properties and we don't need to use
-                // more memory. See bug 1424346 for an example where this
-                // matters.
-                //
-                // TODO: Would it be worth to call reserve() just if it's empty?
-            }
-        }
+        // With deduplication the actual length increase may be less than this.
+        self.declarations.reserve(push_calls_count);
 
         let mut changed = false;
         for decl in &mut drain.declarations {
-            changed |= self.push(
-                decl,
-                importance,
-                mode,
-            );
+            changed |= self.push(decl, importance);
         }
         drain.all_shorthand.declarations().fold(changed, |changed, decl| {
-            changed | self.push(decl, importance, mode)
+            changed | self.push(decl, importance)
         })
     }
 
     /// Adds or overrides the declaration for a given property in this block.
     ///
-    /// Depending on the value of `mode`, this has a different behavior in the
-    /// presence of another declaration with the same ID in the declaration
-    /// block.
+    /// Returns whether the declaration has changed.
     ///
-    /// Returns whether the declaration has changed.
+    /// This is only used for parsing and internal use.
     pub fn push(
         &mut self,
         declaration: PropertyDeclaration,
         importance: Importance,
-        mode: DeclarationPushMode,
     ) -> bool {
         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];
+                let important = self.declarations_importance[i];
 
-                    // For declarations from parsing, non-important declarations
-                    // shouldn't override existing important one.
-                    if important && !importance.important() {
-                        return false;
-                    }
+                // For declarations from parsing, non-important declarations
+                // shouldn't override existing important one.
+                if important && !importance.important() {
+                    return false;
+                }
 
-                    // 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;
+                // 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 let PropertyDeclaration::Display(new_display) = declaration {
+                        if display::should_ignore_parsed_value(old_display, new_display) {
+                            return false;
                         }
                     }
                 }
 
                 index_to_remove = Some(i);
                 break;
             }
 
@@ -1395,17 +1360,16 @@ pub fn parse_property_declaration_list(
     };
     let mut iter = DeclarationListParser::new(input, parser);
     while let Some(declaration) = iter.next() {
         match declaration {
             Ok(importance) => {
                 block.extend(
                     iter.parser.declarations.drain(),
                     importance,
-                    DeclarationPushMode::Parsing,
                 );
             }
             Err((error, slice)) => {
                 iter.parser.declarations.clear();
 
                 // If the unrecognized property looks like a vendor-specific property,
                 // silently ignore it instead of polluting the error output.
                 if let ParseErrorKind::Custom(StyleParseErrorKind::UnknownVendorProperty) = error.kind {
--- a/servo/components/style/stylesheets/keyframes_rule.rs
+++ b/servo/components/style/stylesheets/keyframes_rule.rs
@@ -3,17 +3,17 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! Keyframes: https://drafts.csswg.org/css-animations/#keyframes
 
 use cssparser::{AtRuleParser, CowRcStr, Parser, ParserInput, QualifiedRuleParser, RuleListParser};
 use cssparser::{parse_one_rule, DeclarationListParser, DeclarationParser, SourceLocation, Token};
 use error_reporting::ContextualParseError;
 use parser::ParserContext;
-use properties::{DeclarationPushMode, Importance, PropertyDeclaration};
+use properties::{Importance, PropertyDeclaration};
 use properties::{LonghandId, PropertyDeclarationBlock, PropertyId};
 use properties::{PropertyDeclarationId, SourcePropertyDeclaration};
 use properties::LonghandIdSet;
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
 use servo_arc::Arc;
 use shared_lock::{DeepCloneParams, DeepCloneWithLock, SharedRwLock, SharedRwLockReadGuard};
 use shared_lock::{Locked, ToCssWithGuard};
 use std::fmt::{self, Write};
@@ -549,17 +549,16 @@ impl<'a, 'i> QualifiedRuleParser<'i> for
         let mut iter = DeclarationListParser::new(input, parser);
         let mut block = PropertyDeclarationBlock::new();
         while let Some(declaration) = iter.next() {
             match declaration {
                 Ok(()) => {
                     block.extend(
                         iter.parser.declarations.drain(),
                         Importance::Normal,
-                        DeclarationPushMode::Parsing,
                     );
                 },
                 Err((error, slice)) => {
                     iter.parser.declarations.clear();
                     let location = error.location;
                     let error =
                         ContextualParseError::UnsupportedKeyframePropertyDeclaration(slice, error);
                     context.log_css_error(location, error);
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -122,17 +122,17 @@ use style::gecko_bindings::structs::nsTA
 use style::gecko_bindings::structs::nsresult;
 use style::gecko_bindings::sugar::ownership::{FFIArcHelpers, HasFFI, HasArcFFI};
 use style::gecko_bindings::sugar::ownership::{HasSimpleFFI, Strong};
 use style::gecko_bindings::sugar::refptr::RefPtr;
 use style::gecko_properties;
 use style::invalidation::element::restyle_hints;
 use style::media_queries::MediaList;
 use style::parser::{Parse, ParserContext, self};
-use style::properties::{ComputedValues, DeclarationPushMode, Importance};
+use style::properties::{ComputedValues, Importance};
 use style::properties::{LonghandId, LonghandIdSet, PropertyDeclarationBlock, PropertyId};
 use style::properties::{PropertyDeclarationId, ShorthandId};
 use style::properties::{SourcePropertyDeclaration, StyleBuilder};
 use style::properties::{parse_one_declaration_into, parse_style_attribute};
 use style::properties::animated_properties::AnimationValue;
 use style::properties::animated_properties::compare_property_priority;
 use style::rule_cache::RuleCacheConditions;
 use style::rule_tree::{CascadeLevel, StrongRuleNode};
@@ -3271,17 +3271,16 @@ pub extern "C" fn Servo_ParseProperty(
 
     match result {
         Ok(()) => {
             let global_style_data = &*GLOBAL_STYLE_DATA;
             let mut block = PropertyDeclarationBlock::new();
             block.extend(
                 declarations.drain(),
                 Importance::Normal,
-                DeclarationPushMode::Append,
             );
             Arc::new(global_style_data.shared_lock.wrap(block)).into_strong()
         }
         Err(_) => RawServoDeclarationBlockStrong::null()
     }
 }
 
 #[no_mangle]
@@ -3628,17 +3627,16 @@ pub unsafe extern "C" fn Servo_Declarati
 pub unsafe extern "C" fn Servo_DeclarationBlock_SetPropertyToAnimationValue(
     declarations: RawServoDeclarationBlockBorrowed,
     animation_value: RawServoAnimationValueBorrowed,
 ) -> bool {
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
         decls.push(
             AnimationValue::as_arc(&animation_value).uncompute(),
             Importance::Normal,
-            DeclarationPushMode::Append,
         )
     })
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_DeclarationBlock_SetPropertyById(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
@@ -3919,17 +3917,17 @@ pub unsafe extern "C" fn Servo_Declarati
     use style::properties::{PropertyDeclaration, LonghandId};
     use style::properties::longhands::_x_lang::computed_value::T as Lang;
 
     let long = get_longhand_from_id!(property);
     let prop = match_wrap_declared! { long,
         XLang => Lang(Atom::from_raw(value)),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 #[allow(unreachable_code)]
 pub extern "C" fn Servo_DeclarationBlock_SetKeywordValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
@@ -3995,17 +3993,17 @@ pub extern "C" fn Servo_DeclarationBlock
         WhiteSpace => longhands::white_space::SpecifiedValue::from_gecko_keyword(value),
         CaptionSide => longhands::caption_side::SpecifiedValue::from_gecko_keyword(value),
         BorderTopStyle => BorderStyle::from_gecko_keyword(value),
         BorderRightStyle => BorderStyle::from_gecko_keyword(value),
         BorderBottomStyle => BorderStyle::from_gecko_keyword(value),
         BorderLeftStyle => BorderStyle::from_gecko_keyword(value),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetIntValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
     value: i32
@@ -4016,17 +4014,17 @@ pub extern "C" fn Servo_DeclarationBlock
 
     let long = get_longhand_from_id!(property);
     let prop = match_wrap_declared! { long,
         XSpan => Span(value),
         // Gecko uses Integer values to signal that it is relative
         MozScriptLevel => MozScriptLevel::Relative(value),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetPixelValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
     value: f32
@@ -4071,17 +4069,17 @@ pub extern "C" fn Servo_DeclarationBlock
             Box::new(BorderCornerRadius::new(length.clone(), length))
         },
         BorderBottomRightRadius => {
             let length = LengthOrPercentage::from(nocalc);
             Box::new(BorderCornerRadius::new(length.clone(), length))
         },
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetLengthValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
@@ -4109,17 +4107,17 @@ pub extern "C" fn Servo_DeclarationBlock
     };
 
     let prop = match_wrap_declared! { long,
         Width => MozLength::LengthOrPercentageOrAuto(nocalc.into()),
         FontSize => LengthOrPercentage::from(nocalc).into(),
         MozScriptMinSize => MozScriptMinSize(nocalc),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetNumberValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
     value: f32,
@@ -4131,17 +4129,17 @@ pub extern "C" fn Servo_DeclarationBlock
     let long = get_longhand_from_id!(property);
 
     let prop = match_wrap_declared! { long,
         MozScriptSizeMultiplier => MozScriptSizeMultiplier(value),
         // Gecko uses Number values to signal that it is absolute
         MozScriptLevel => MozScriptLevel::MozAbsolute(value as i32),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetPercentValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
     value: f32,
@@ -4159,17 +4157,17 @@ pub extern "C" fn Servo_DeclarationBlock
         Width => MozLength::LengthOrPercentageOrAuto(pc.into()),
         MarginTop => pc.into(),
         MarginRight => pc.into(),
         MarginBottom => pc.into(),
         MarginLeft => pc.into(),
         FontSize => LengthOrPercentage::from(pc).into(),
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetAutoValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
 ) {
@@ -4183,17 +4181,17 @@ pub extern "C" fn Servo_DeclarationBlock
         Height => MozLength::auto(),
         Width => MozLength::auto(),
         MarginTop => auto,
         MarginRight => auto,
         MarginBottom => auto,
         MarginLeft => auto,
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetCurrentColor(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
 ) {
@@ -4205,17 +4203,17 @@ pub extern "C" fn Servo_DeclarationBlock
 
     let prop = match_wrap_declared! { long,
         BorderTopColor => cc,
         BorderRightColor => cc,
         BorderBottomColor => cc,
         BorderLeftColor => cc,
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetColorValue(
     declarations: RawServoDeclarationBlockBorrowed,
     property: nsCSSPropertyID,
     value: structs::nscolor,
@@ -4233,17 +4231,17 @@ pub extern "C" fn Servo_DeclarationBlock
         BorderTopColor => color,
         BorderRightColor => color,
         BorderBottomColor => color,
         BorderLeftColor => color,
         Color => longhands::color::SpecifiedValue(color),
         BackgroundColor => color,
     };
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(prop, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(prop, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetFontFamily(
     declarations: RawServoDeclarationBlockBorrowed,
     value: *const nsAString,
 ) {
@@ -4254,17 +4252,17 @@ pub extern "C" fn Servo_DeclarationBlock
     let string = unsafe { (*value).to_string() };
     let mut input = ParserInput::new(&string);
     let mut parser = Parser::new(&mut input);
     let result = FontFamily::parse_specified(&mut parser);
     if let Ok(family) = result {
         if parser.is_exhausted() {
             let decl = PropertyDeclaration::FontFamily(family);
             write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-                decls.push(decl, Importance::Normal, DeclarationPushMode::Append);
+                decls.push(decl, Importance::Normal);
             })
         }
     }
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetBackgroundImage(
     declarations: RawServoDeclarationBlockBorrowed,
@@ -4287,32 +4285,32 @@ pub extern "C" fn Servo_DeclarationBlock
         QuirksMode::NoQuirks,
         None,
     );
     let url = SpecifiedImageUrl::parse_from_string(string.into(), &context);
     let decl = PropertyDeclaration::BackgroundImage(BackgroundImage(
         vec![Either::Second(Image::Url(url))]
     ));
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(decl, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(decl, Importance::Normal);
     });
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_DeclarationBlock_SetTextDecorationColorOverride(
     declarations: RawServoDeclarationBlockBorrowed,
 ) {
     use style::properties::PropertyDeclaration;
     use style::values::specified::text::TextDecorationLine;
 
     let mut decoration = TextDecorationLine::none();
     decoration |= TextDecorationLine::COLOR_OVERRIDE;
     let decl = PropertyDeclaration::TextDecorationLine(decoration);
     write_locked_arc(declarations, |decls: &mut PropertyDeclarationBlock| {
-        decls.push(decl, Importance::Normal, DeclarationPushMode::Append);
+        decls.push(decl, Importance::Normal);
     })
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_CSSSupports2(
     property: *const nsACString,
     value: *const nsACString,
 ) -> bool {
@@ -4974,17 +4972,16 @@ pub unsafe extern "C" fn Servo_StyleSet_
                             }
 
                             id.to_physical(writing_mode)
                         }
                         PropertyDeclarationId::Custom(..) => {
                             custom_properties.push(
                                 declaration.clone(),
                                 Importance::Normal,
-                                DeclarationPushMode::Append,
                             );
                             continue;
                         }
                     };
 
                     if properties_set_at_current_offset.contains(id) {
                         continue;
                     }