Bug 1473180 part 5 - Remove DeclarationPushMode. r?emilio draft
authorXidorn Quan <me@upsuper.org>
Wed, 18 Jul 2018 21:43:44 +1000
changeset 820140 4d6292b1edd2ab39dd0002f35de46a7120283dcf
parent 820139 05ce3e0531dcbe9da89d1a2ae4efe629cddc73d0
push id116733
push userxquan@mozilla.com
push dateWed, 18 Jul 2018 23:58:21 +0000
reviewersemilio
bugs1473180
milestone63.0a1
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,
 
@@ -450,96 +437,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;
             }
 
@@ -1378,17 +1343,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 {
@@ -4960,17 +4958,16 @@ pub unsafe extern "C" fn Servo_StyleSet_
                             }
 
                             id
                         }
                         PropertyDeclarationId::Custom(..) => {
                             custom_properties.push(
                                 declaration.clone(),
                                 Importance::Normal,
-                                DeclarationPushMode::Append,
                             );
                             continue;
                         }
                     };
 
                     if properties_set_at_current_offset.contains(id) {
                         continue;
                     }