Bug 1394297 - style: Don't mutate style structs when attempting to store the same value. r?emilio draft
authorCameron McCormack <cam@mcc.id.au>
Mon, 28 Aug 2017 17:44:05 +0800
changeset 654166 09328243635f9d76fee2c3f125f4e1958ec0334b
parent 653709 f819969d7619f01e806e2685b8b3196f64624551
child 728485 cff5f8076b01acdc2e09262d5790b2f4f39c0fd3
push id76487
push userbmo:cam@mcc.id.au
push dateMon, 28 Aug 2017 09:46:01 +0000
reviewersemilio
bugs1394297
milestone57.0a1
Bug 1394297 - style: Don't mutate style structs when attempting to store the same value. r?emilio MozReview-Commit-ID: 2ha3VyT4wHd
layout/style/CounterStyleManager.h
layout/style/ServoBindings.cpp
layout/style/ServoBindings.h
servo/components/style/gecko/values.rs
servo/components/style/properties/longhand/box.mako.rs
servo/components/style/properties/properties.mako.rs
--- a/layout/style/CounterStyleManager.h
+++ b/layout/style/CounterStyleManager.h
@@ -232,16 +232,21 @@ public:
   bool operator!() const { return !mRaw; }
   bool operator==(const CounterStylePtr& aOther) const
     { return mRaw == aOther.mRaw; }
   bool operator!=(const CounterStylePtr& aOther) const
     { return mRaw != aOther.mRaw; }
 
   bool IsResolved() const { return !IsUnresolved(); }
   inline void Resolve(CounterStyleManager* aManager);
+  nsIAtom* AsAtom() const
+  {
+    MOZ_ASSERT(IsUnresolved());
+    return reinterpret_cast<nsIAtom*>(mRaw & ~eMask);
+  }
 
 private:
   CounterStyle* Get() const
   {
     MOZ_ASSERT(IsResolved());
     return reinterpret_cast<CounterStyle*>(mRaw & ~eMask);
   }
   template<typename T>
@@ -261,21 +266,16 @@ private:
     eAnonymousCounterStyle = 1,
     eUnresolvedAtom = 2,
     eMask = 3,
   };
 
   Type GetType() const { return static_cast<Type>(mRaw & eMask); }
   bool IsUnresolved() const { return GetType() == eUnresolvedAtom; }
   bool IsAnonymous() const { return GetType() == eAnonymousCounterStyle; }
-  nsIAtom* AsAtom()
-  {
-    MOZ_ASSERT(IsUnresolved());
-    return reinterpret_cast<nsIAtom*>(mRaw & ~eMask);
-  }
   AnonymousCounterStyle* AsAnonymous()
   {
     MOZ_ASSERT(IsAnonymous());
     return static_cast<AnonymousCounterStyle*>(
       reinterpret_cast<CounterStyle*>(mRaw & ~eMask));
   }
 
   void Reset()
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -1449,57 +1449,80 @@ Gecko_SetCounterStyleToString(CounterSty
 
 void
 Gecko_CopyCounterStyle(CounterStylePtr* aDst, const CounterStylePtr* aSrc)
 {
   *aDst = *aSrc;
 }
 
 bool
-Gecko_CounterStyle_IsNone(const CounterStylePtr* aPtr) {
+Gecko_CounterStyle_IsUnresolved(const CounterStylePtr* aPtr)
+{
+  MOZ_ASSERT(aPtr);
+  return !aPtr->IsResolved();
+}
+
+bool
+Gecko_CounterStyle_IsNone(const CounterStylePtr* aPtr)
+{
   MOZ_ASSERT(aPtr);
   return (*aPtr)->IsNone();
 }
 
 bool
-Gecko_CounterStyle_IsName(const CounterStylePtr* aPtr) {
-  return !Gecko_CounterStyle_IsNone(aPtr) && !(*aPtr)->AsAnonymous();
+Gecko_CounterStyle_IsName(const CounterStylePtr* aPtr)
+{
+  return aPtr->IsResolved() &&
+         !Gecko_CounterStyle_IsNone(aPtr) &&
+         !(*aPtr)->AsAnonymous();
 }
 
 void
 Gecko_CounterStyle_GetName(const CounterStylePtr* aPtr,
-                           nsAString* aResult) {
+                           nsAString* aResult)
+{
   MOZ_ASSERT(Gecko_CounterStyle_IsName(aPtr));
   (*aPtr)->GetStyleName(*aResult);
 }
 
+nsIAtom*
+Gecko_CounterStyle_GetAtom(const CounterStylePtr* aPtr)
+{
+  MOZ_ASSERT(!aPtr->IsResolved());
+  return do_AddRef(aPtr->AsAtom()).take();
+}
+
 const nsTArray<nsString>&
-Gecko_CounterStyle_GetSymbols(const CounterStylePtr* aPtr) {
+Gecko_CounterStyle_GetSymbols(const CounterStylePtr* aPtr)
+{
   MOZ_ASSERT((*aPtr)->AsAnonymous());
   AnonymousCounterStyle* anonymous = (*aPtr)->AsAnonymous();
   return anonymous->GetSymbols();
 }
 
 uint8_t
-Gecko_CounterStyle_GetSystem(const CounterStylePtr* aPtr) {
+Gecko_CounterStyle_GetSystem(const CounterStylePtr* aPtr)
+{
   MOZ_ASSERT((*aPtr)->AsAnonymous());
   AnonymousCounterStyle* anonymous = (*aPtr)->AsAnonymous();
   return anonymous->GetSystem();
 }
 
 bool
-Gecko_CounterStyle_IsSingleString(const CounterStylePtr* aPtr) {
+Gecko_CounterStyle_IsSingleString(const CounterStylePtr* aPtr)
+{
   MOZ_ASSERT(aPtr);
   AnonymousCounterStyle* anonymous = (*aPtr)->AsAnonymous();
   return anonymous ? anonymous->IsSingleString() : false;
 }
 
 void
 Gecko_CounterStyle_GetSingleString(const CounterStylePtr* aPtr,
-                                   nsAString* aResult) {
+                                   nsAString* aResult)
+{
   MOZ_ASSERT(Gecko_CounterStyle_IsSingleString(aPtr));
   const nsTArray<nsString>& symbols = Gecko_CounterStyle_GetSymbols(aPtr);
   MOZ_ASSERT(symbols.Length() == 1);
   aResult->Assign(symbols[0]);
 }
 
 already_AddRefed<css::URLValue>
 ServoBundledURI::IntoCssUrl()
--- a/layout/style/ServoBindings.h
+++ b/layout/style/ServoBindings.h
@@ -327,20 +327,22 @@ void Gecko_SetCounterStyleToName(mozilla
 void Gecko_SetCounterStyleToSymbols(mozilla::CounterStylePtr* ptr,
                                     uint8_t symbols_type,
                                     nsACString const* const* symbols,
                                     uint32_t symbols_count);
 void Gecko_SetCounterStyleToString(mozilla::CounterStylePtr* ptr,
                                    const nsACString* symbol);
 void Gecko_CopyCounterStyle(mozilla::CounterStylePtr* dst,
                             const mozilla::CounterStylePtr* src);
+bool Gecko_CounterStyle_IsUnresolved(const mozilla::CounterStylePtr* ptr);
 bool Gecko_CounterStyle_IsNone(const mozilla::CounterStylePtr* ptr);
 bool Gecko_CounterStyle_IsName(const mozilla::CounterStylePtr* ptr);
 void Gecko_CounterStyle_GetName(const mozilla::CounterStylePtr* ptr,
                                 nsAString* result);
+nsIAtom* Gecko_CounterStyle_GetAtom(const mozilla::CounterStylePtr* ptr);
 const nsTArray<nsString>& Gecko_CounterStyle_GetSymbols(const mozilla::CounterStylePtr* ptr);
 uint8_t Gecko_CounterStyle_GetSystem(const mozilla::CounterStylePtr* ptr);
 bool Gecko_CounterStyle_IsSingleString(const mozilla::CounterStylePtr* ptr);
 void Gecko_CounterStyle_GetSingleString(const mozilla::CounterStylePtr* ptr,
                                         nsAString* result);
 
 // background-image style.
 void Gecko_SetNullImageValue(nsStyleImage* image);
--- a/servo/components/style/gecko/values.rs
+++ b/servo/components/style/gecko/values.rs
@@ -1,16 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #![allow(unsafe_code)]
 
 //! Different kind of helpers to interact with Gecko values.
 
+use Atom;
 use app_units::Au;
 use counter_style::Symbol;
 use cssparser::RGBA;
 use gecko_bindings::structs::{CounterStylePtr, nsStyleCoord};
 use gecko_bindings::structs::{StyleGridTrackBreadth, StyleShapeRadius};
 use gecko_bindings::sugar::ns_style_coord::{CoordData, CoordDataMut, CoordDataValue};
 use media_queries::Device;
 use nsstring::{nsACString, nsCString};
@@ -474,26 +475,33 @@ impl CounterStyleOrNone {
                                      symbols.as_ptr(), symbols.len() as u32) };
             }
         }
     }
 
     /// Convert Gecko CounterStylePtr to CounterStyleOrNone.
     pub fn from_gecko_value(gecko_value: &CounterStylePtr) -> Self {
         use counter_style::{Symbol, Symbols};
+        use gecko_bindings::bindings::Gecko_CounterStyle_GetAtom;
         use gecko_bindings::bindings::Gecko_CounterStyle_GetName;
         use gecko_bindings::bindings::Gecko_CounterStyle_GetSymbols;
         use gecko_bindings::bindings::Gecko_CounterStyle_GetSystem;
         use gecko_bindings::bindings::Gecko_CounterStyle_IsName;
         use gecko_bindings::bindings::Gecko_CounterStyle_IsNone;
+        use gecko_bindings::bindings::Gecko_CounterStyle_IsUnresolved;
         use values::CustomIdent;
         use values::generics::SymbolsType;
 
         if unsafe { Gecko_CounterStyle_IsNone(gecko_value) } {
             CounterStyleOrNone::None
+        } else if unsafe { Gecko_CounterStyle_IsUnresolved(gecko_value) } {
+            let atom = unsafe {
+                Atom::from_addrefed(Gecko_CounterStyle_GetAtom(gecko_value))
+            };
+            CounterStyleOrNone::Name(CustomIdent(atom))
         } else if unsafe { Gecko_CounterStyle_IsName(gecko_value) } {
             ns_auto_string!(name);
             unsafe { Gecko_CounterStyle_GetName(gecko_value, &mut *name) };
             CounterStyleOrNone::Name(CustomIdent((&*name).into()))
         } else {
             let system = unsafe { Gecko_CounterStyle_GetSystem(gecko_value) };
             let symbol_type = SymbolsType::from_gecko_keyword(system as u32);
             let symbols = unsafe {
--- a/servo/components/style/properties/longhand/box.mako.rs
+++ b/servo/components/style/properties/longhand/box.mako.rs
@@ -671,17 +671,16 @@
                          gecko_enum_prefix="PlaybackDirection",
                          custom_consts=animation_direction_custom_consts,
                          extra_prefixes="moz webkit",
                          spec="https://drafts.csswg.org/css-animations/#propdef-animation-direction",
                          allowed_in_keyframe_block=False)}
 
 ${helpers.single_keyword("animation-play-state",
                          "running paused",
-                         need_clone=True,
                          need_index=True,
                          animation_value_type="none",
                          vector=True,
                          extra_prefixes="moz webkit",
                          spec="https://drafts.csswg.org/css-animations/#propdef-animation-play-state",
                          allowed_in_keyframe_block=False)}
 
 ${helpers.single_keyword("animation-fill-mode",
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -2698,47 +2698,91 @@ impl<'a> StyleBuilder<'a> {
         % if property.ident == "content":
         self.flags.insert(::properties::computed_value_flags::INHERITS_CONTENT);
         % endif
 
         % if property.ident == "display":
         self.flags.insert(::properties::computed_value_flags::INHERITS_DISPLAY);
         % endif
 
+        % if property.need_clone:
+        if self.${property.style_struct.ident}.get_if_mutated().is_none() {
+            % if property.style_struct.inherited:
+            // Since this is an inherited property, there is no need to
+            // check what value we have; copying the inherited value will
+            // never have an effect.
+            return;
+            % else:
+            // Check if setting this value would have no effect.
+            if self.${property.style_struct.ident}.clone_${property.ident}() ==
+               inherited_struct.clone_${property.ident}() {
+                return;
+            }
+            % endif
+        }
+        % endif
+
         self.${property.style_struct.ident}.mutate()
             .copy_${property.ident}_from(
                 inherited_struct,
                 % if property.logical:
                 self.writing_mode,
                 % endif
             );
     }
 
     /// Reset `${property.ident}` to the initial value.
     #[allow(non_snake_case)]
     pub fn reset_${property.ident}(&mut self) {
         let reset_struct =
             self.reset_style.get_${property.style_struct.name_lower}();
 
+        % if property.need_clone:
+        if self.${property.style_struct.ident}.get_if_mutated().is_none() {
+            % if not property.style_struct.inherited:
+            // Since this is a non-inherited property, there is no need to
+            // check what value we have; copying the reset value will
+            // never have an effect.
+            return;
+            % else:
+            // Check if setting this value would have no effect.
+            if self.${property.style_struct.ident}.clone_${property.ident}() ==
+               reset_struct.clone_${property.ident}() {
+                return;
+            }
+            % endif
+        }
+        % endif
+
         self.${property.style_struct.ident}.mutate()
             .reset_${property.ident}(
                 reset_struct,
                 % if property.logical:
                 self.writing_mode,
                 % endif
             );
     }
 
     % if not property.is_vector:
     /// Set the `${property.ident}` to the computed value `value`.
     #[allow(non_snake_case)]
     pub fn set_${property.ident}(
         &mut self,
         value: longhands::${property.ident}::computed_value::T
     ) {
+        % if property.need_clone:
+        // FIXME(heycam): We only skip logical properties because we
+        // don't have clone functions for them that take a WritingMode.
+        if self.${property.style_struct.ident}.get_if_mutated().is_none() {
+            if self.${property.style_struct.ident}.clone_${property.ident}() == value {
+                return;
+            }
+        }
+        % endif
+
         <% props_need_device = ["content", "list_style_type", "font_variant_alternates"] %>
         self.${property.style_struct.ident}.mutate()
             .set_${property.ident}(
                 value,
                 % if property.logical:
                 self.writing_mode,
                 % elif product == "gecko" and property.ident in props_need_device:
                 self.device,