Bug 1551991 - Remove nsCSSValue usage of GetSymbols() GetAdditiveSymbols(). r=jwatt
☠☠ backed out by 283b94c196a1 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 16 May 2019 14:31:42 +0000
changeset 474084 d81e4aa6bf0c5701bf5369b4f764caa09cf803ea
parent 474083 c354e61f2a347779dbc2f31ca384046f8a85953a
child 474085 e12a979de502bf14147c43b34d437449bbd74552
push id36022
push userncsoregi@mozilla.com
push dateThu, 16 May 2019 21:55:16 +0000
treeherdermozilla-central@96802be91766 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs1551991
milestone68.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 1551991 - Remove nsCSSValue usage of GetSymbols() GetAdditiveSymbols(). r=jwatt Differential Revision: https://phabricator.services.mozilla.com/D31319
layout/style/CounterStyleManager.cpp
layout/style/CounterStyleManager.h
layout/style/ServoStyleConstsInlines.h
servo/ports/geckolib/cbindgen.toml
servo/ports/geckolib/glue.rs
--- a/layout/style/CounterStyleManager.cpp
+++ b/layout/style/CounterStyleManager.cpp
@@ -19,20 +19,17 @@
 #include "nsTArray.h"
 #include "nsTHashtable.h"
 #include "nsUnicodeProperties.h"
 #include "mozilla/ServoBindings.h"
 #include "mozilla/ServoStyleSet.h"
 
 namespace mozilla {
 
-struct AdditiveSymbol {
-  CounterValue weight;
-  nsString symbol;
-};
+using AdditiveSymbol = StyleAdditiveSymbol;
 
 struct NegativeType {
   nsString before, after;
 };
 
 struct PadType {
   int32_t width;
   nsString symbol;
@@ -44,38 +41,38 @@ struct PadType {
 // than this, the whole counter text will be dropped as well.
 // The spec requires user agents to support at least 60 Unicode code-
 // points for counter text. However, this constant only limits the
 // length in 16-bit units. So it has to be at least 120, since code-
 // points outside the BMP will need 2 16-bit units.
 #define LENGTH_LIMIT 150
 
 static bool GetCyclicCounterText(CounterValue aOrdinal, nsAString& aResult,
-                                 const nsTArray<nsString>& aSymbols) {
+                                 Span<const nsString> aSymbols) {
   MOZ_ASSERT(aSymbols.Length() >= 1, "No symbol available for cyclic counter.");
   auto n = aSymbols.Length();
   CounterValue index = (aOrdinal - 1) % n;
   aResult = aSymbols[index >= 0 ? index : index + n];
   return true;
 }
 
 static bool GetFixedCounterText(CounterValue aOrdinal, nsAString& aResult,
                                 CounterValue aStart,
-                                const nsTArray<nsString>& aSymbols) {
+                                Span<const nsString> aSymbols) {
   CounterValue index = aOrdinal - aStart;
   if (index >= 0 && index < CounterValue(aSymbols.Length())) {
     aResult = aSymbols[index];
     return true;
   } else {
     return false;
   }
 }
 
 static bool GetSymbolicCounterText(CounterValue aOrdinal, nsAString& aResult,
-                                   const nsTArray<nsString>& aSymbols) {
+                                   Span<const nsString> aSymbols) {
   MOZ_ASSERT(aSymbols.Length() >= 1,
              "No symbol available for symbolic counter.");
   MOZ_ASSERT(aOrdinal >= 0, "Invalid ordinal.");
   if (aOrdinal == 0) {
     return false;
   }
 
   aResult.Truncate();
@@ -91,17 +88,17 @@ static bool GetSymbolicCounterText(Count
     for (size_t i = 0; i < len; ++i) {
       aResult.Append(symbol);
     }
   }
   return true;
 }
 
 static bool GetAlphabeticCounterText(CounterValue aOrdinal, nsAString& aResult,
-                                     const nsTArray<nsString>& aSymbols) {
+                                     Span<const nsString> aSymbols) {
   MOZ_ASSERT(aSymbols.Length() >= 2, "Too few symbols for alphabetic counter.");
   MOZ_ASSERT(aOrdinal >= 0, "Invalid ordinal.");
   if (aOrdinal == 0) {
     return false;
   }
 
   auto n = aSymbols.Length();
   // The precise length of this array should be
@@ -117,17 +114,17 @@ static bool GetAlphabeticCounterText(Cou
   aResult.Truncate();
   for (auto i = indexes.Length(); i > 0; --i) {
     aResult.Append(aSymbols[indexes[i - 1]]);
   }
   return true;
 }
 
 static bool GetNumericCounterText(CounterValue aOrdinal, nsAString& aResult,
-                                  const nsTArray<nsString>& aSymbols) {
+                                  Span<const nsString> aSymbols) {
   MOZ_ASSERT(aSymbols.Length() >= 2, "Too few symbols for numeric counter.");
   MOZ_ASSERT(aOrdinal >= 0, "Invalid ordinal.");
 
   if (aOrdinal == 0) {
     aResult = aSymbols[0];
     return true;
   }
 
@@ -141,21 +138,21 @@ static bool GetNumericCounterText(Counte
   aResult.Truncate();
   for (auto i = indexes.Length(); i > 0; --i) {
     aResult.Append(aSymbols[indexes[i - 1]]);
   }
   return true;
 }
 
 static bool GetAdditiveCounterText(CounterValue aOrdinal, nsAString& aResult,
-                                   const nsTArray<AdditiveSymbol>& aSymbols) {
+                                   Span<const AdditiveSymbol> aSymbols) {
   MOZ_ASSERT(aOrdinal >= 0, "Invalid ordinal.");
 
   if (aOrdinal == 0) {
-    const AdditiveSymbol& last = aSymbols.LastElement();
+    const AdditiveSymbol& last = aSymbols[aSymbols.Length() - 1];
     if (last.weight == 0) {
       aResult = last.symbol;
       return true;
     }
     return false;
   }
 
   aResult.Truncate();
@@ -993,18 +990,18 @@ class CustomCounterStyle final : public 
   ~CustomCounterStyle() {}
 
   nsCSSValue GetDesc(nsCSSCounterDesc aDesc) const {
     nsCSSValue value;
     Servo_CounterStyleRule_GetDescriptor(mRule, aDesc, &value);
     return value;
   }
 
-  const nsTArray<nsString>& GetSymbols();
-  const nsTArray<AdditiveSymbol>& GetAdditiveSymbols();
+  Span<const nsString> GetSymbols();
+  Span<const AdditiveSymbol> GetAdditiveSymbols();
 
   // The speak-as values of counter styles may form a loop, and the
   // loops may have complex interaction with the loop formed by
   // extending. To solve this problem, the computation of speak-as is
   // divided into two phases:
   // 1. figure out the raw value, by ComputeRawSpeakAs, and
   // 2. eliminate loop, by ComputeSpeakAs.
   // See comments before the definitions of these methods for details.
@@ -1040,18 +1037,18 @@ class CustomCounterStyle final : public 
     FLAG_PREFIX_INITED = 1 << 5,
     FLAG_SUFFIX_INITED = 1 << 6,
     FLAG_PAD_INITED = 1 << 7,
     FLAG_SPEAKAS_INITED = 1 << 8,
   };
   uint16_t mFlags;
 
   // Fields below will be initialized when necessary.
-  nsTArray<nsString> mSymbols;
-  nsTArray<AdditiveSymbol> mAdditiveSymbols;
+  StyleOwnedSlice<nsString> mSymbols;
+  StyleOwnedSlice<AdditiveSymbol> mAdditiveSymbols;
   NegativeType mNegative;
   nsString mPrefix, mSuffix;
   PadType mPad;
 
   // CounterStyleManager will guarantee that none of the pointers below
   // refers to a freed CounterStyle. There are two possible cases where
   // the manager will release its reference to a CounterStyle: 1. the
   // manager itself is released, 2. a rule is invalidated. In the first
@@ -1303,41 +1300,28 @@ bool CustomCounterStyle::GetInitialCount
       return GetExtendsRoot()->GetInitialCounterText(aOrdinal, aWritingMode,
                                                      aResult, aIsRTL);
     default:
       MOZ_ASSERT_UNREACHABLE("Invalid system.");
       return false;
   }
 }
 
-const nsTArray<nsString>& CustomCounterStyle::GetSymbols() {
+Span<const nsString> CustomCounterStyle::GetSymbols() {
   if (mSymbols.IsEmpty()) {
-    nsCSSValue values = GetDesc(eCSSCounterDesc_Symbols);
-    for (const nsCSSValueList* item = values.GetListValue(); item;
-         item = item->mNext) {
-      nsString* symbol = mSymbols.AppendElement();
-      item->mValue.GetStringValue(*symbol);
-    }
-    mSymbols.Compact();
+    Servo_CounterStyleRule_GetSymbols(mRule, &mSymbols);
   }
-  return mSymbols;
+  return mSymbols.AsSpan();
 }
 
-const nsTArray<AdditiveSymbol>& CustomCounterStyle::GetAdditiveSymbols() {
+Span<const AdditiveSymbol> CustomCounterStyle::GetAdditiveSymbols() {
   if (mAdditiveSymbols.IsEmpty()) {
-    nsCSSValue values = GetDesc(eCSSCounterDesc_AdditiveSymbols);
-    for (const nsCSSValuePairList* item = values.GetPairListValue(); item;
-         item = item->mNext) {
-      AdditiveSymbol* symbol = mAdditiveSymbols.AppendElement();
-      symbol->weight = item->mXValue.GetIntValue();
-      item->mYValue.GetStringValue(symbol->symbol);
-    }
-    mAdditiveSymbols.Compact();
+    Servo_CounterStyleRule_GetAdditiveSymbols(mRule, &mAdditiveSymbols);
   }
-  return mAdditiveSymbols;
+  return mAdditiveSymbols.AsSpan();
 }
 
 // This method is used to provide the computed value for 'auto'.
 uint8_t CustomCounterStyle::GetSpeakAsAutoValue() {
   uint8_t system = mSystem;
   if (IsExtendsSystem()) {
     CounterStyle* root = GetExtendsRoot();
     if (!root->IsCustomStyle()) {
--- a/layout/style/CounterStyleManager.h
+++ b/layout/style/CounterStyleManager.h
@@ -113,17 +113,17 @@ class AnonymousCounterStyle final : publ
   virtual bool GetInitialCounterText(CounterValue aOrdinal,
                                      WritingMode aWritingMode,
                                      nsAString& aResult, bool& aIsRTL) override;
 
   virtual AnonymousCounterStyle* AsAnonymous() override { return this; }
 
   bool IsSingleString() const { return mSingleString; }
   uint8_t GetSystem() const { return mSystem; }
-  const nsTArray<nsString>& GetSymbols() const { return mSymbols; }
+  Span<const nsString> GetSymbols() const { return MakeSpan(mSymbols); }
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AnonymousCounterStyle)
 
  private:
   ~AnonymousCounterStyle() {}
 
   bool mSingleString;
   uint8_t mSystem;
--- a/layout/style/ServoStyleConstsInlines.h
+++ b/layout/style/ServoStyleConstsInlines.h
@@ -29,28 +29,41 @@ inline StyleOwnedSlice<T>::StyleOwnedSli
     size_t i = 0;
     for (const T& elem : aOther.AsSpan()) {
       new (ptr + i++) T(elem);
     }
   }
 }
 
 template <typename T>
-inline StyleOwnedSlice<T>::~StyleOwnedSlice() {
+inline StyleOwnedSlice<T>::StyleOwnedSlice(StyleOwnedSlice&& aOther) {
+  len = aOther.len;
+  ptr = aOther.ptr;
+  aOther.ptr = (T*)alignof(T);
+  aOther.len = 0;
+}
+
+template <typename T>
+inline void StyleOwnedSlice<T>::Clear() {
   if (!len) {
     return;
   }
   for (size_t i : IntegerRange(len)) {
     ptr[i].~T();
   }
   free(ptr);
   ptr = (T*)alignof(T);
   len = 0;
 }
 
+template <typename T>
+inline StyleOwnedSlice<T>::~StyleOwnedSlice() {
+  Clear();
+}
+
 // This code is basically a C++ port of the Arc::clone() implementation in
 // servo/components/servo_arc/lib.rs.
 static constexpr const size_t kStaticRefcount =
     std::numeric_limits<size_t>::max();
 static constexpr const size_t kMaxRefcount =
     std::numeric_limits<intptr_t>::max();
 static constexpr const uint32_t kArcSliceCanary = 0xf3f3f3f3;
 
--- a/servo/ports/geckolib/cbindgen.toml
+++ b/servo/ports/geckolib/cbindgen.toml
@@ -350,17 +350,24 @@ renaming_overrides_prefixing = true
   inline nscolor ToColor() const;
 """
 
 "OwnedSlice" = """
   StyleOwnedSlice() :
     ptr((T*)alignof(T)),
     len(0) {}
 
-  inline StyleOwnedSlice(const StyleOwnedSlice& aOther);
+  // Should be easily implementable if wanted, but the default implementation would leak.
+  StyleOwnedSlice& operator=(const StyleOwnedSlice&) = delete;
+  StyleOwnedSlice& operator=(StyleOwnedSlice&&) = delete;
+
+  inline StyleOwnedSlice(const StyleOwnedSlice&);
+  inline StyleOwnedSlice(StyleOwnedSlice&&);
+
+  inline void Clear();
 
   inline ~StyleOwnedSlice();
 
   Span<const T> AsSpan() const {
     return MakeSpan(ptr, len);
   }
 
   size_t Length() const {
@@ -376,16 +383,21 @@ renaming_overrides_prefixing = true
   bool operator!=(const StyleOwnedSlice& other) const {
     return !(*this == other);
   }
 """
 
 "ArcSlice" = """
   inline StyleArcSlice();
   inline StyleArcSlice(const StyleArcSlice& aOther);
+
+  // Should be easily implementable if wanted, but the default implementation would leak.
+  StyleArcSlice& operator=(const StyleArcSlice&) = delete;
+  StyleArcSlice& operator=(StyleArcSlice&&) = delete;
+
   inline explicit StyleArcSlice(const StyleForgottenArcSlicePtr<T>& aPtr);
   inline ~StyleArcSlice();
   inline Span<const T> AsSpan() const;
   inline size_t Length() const;
   inline bool IsEmpty() const;
   inline bool operator==(const StyleArcSlice& other) const;
   inline bool operator!=(const StyleArcSlice& other) const;
 """
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -2961,16 +2961,17 @@ pub unsafe extern "C" fn Servo_CounterSt
 
 fn symbol_to_string(s: &counter_style::Symbol) -> nsString {
     match *s {
         counter_style::Symbol::String(ref s) => nsString::from(&**s),
         counter_style::Symbol::Ident(ref i) => nsString::from(i.0.as_slice())
     }
 }
 
+// TODO(emilio): Cbindgen could be used to simplify a bunch of code here.
 #[no_mangle]
 pub unsafe extern "C" fn Servo_CounterStyleRule_GetPad(
     rule: &RawServoCounterStyleRule,
     width: &mut i32,
     symbol: &mut nsString,
 ) -> bool {
     read_locked_arc(rule, |rule: &CounterStyleRule| {
         let pad = match rule.pad() {
@@ -3076,16 +3077,53 @@ pub unsafe extern "C" fn Servo_CounterSt
             IsOrdinalInRange::InRange
         } else {
             IsOrdinalInRange::NotInRange
         }
     })
 }
 
 #[no_mangle]
+pub unsafe extern "C" fn Servo_CounterStyleRule_GetSymbols(
+    rule: &RawServoCounterStyleRule,
+    symbols: &mut style::OwnedSlice<nsString>,
+) {
+    read_locked_arc(rule, |rule: &CounterStyleRule| {
+        *symbols = match rule.symbols() {
+            Some(s) => s.0.iter().map(symbol_to_string).collect(),
+            None => style::OwnedSlice::default(),
+        };
+    })
+}
+
+#[repr(C)]
+pub struct AdditiveSymbol {
+    pub weight: i32,
+    pub symbol: nsString,
+}
+
+#[no_mangle]
+pub unsafe extern "C" fn Servo_CounterStyleRule_GetAdditiveSymbols(
+    rule: &RawServoCounterStyleRule,
+    symbols: &mut style::OwnedSlice<AdditiveSymbol>,
+) {
+    read_locked_arc(rule, |rule: &CounterStyleRule| {
+        *symbols = match rule.additive_symbols() {
+            Some(s) => s.0.iter().map(|s| {
+                AdditiveSymbol {
+                    weight: s.weight.value(),
+                    symbol: symbol_to_string(&s.symbol),
+                }
+            }).collect(),
+            None => style::OwnedSlice::default(),
+        };
+    })
+}
+
+#[no_mangle]
 pub unsafe extern "C" fn Servo_CounterStyleRule_GetSystem(
     rule: &RawServoCounterStyleRule,
 ) -> u8 {
     use style::counter_style::System;
     read_locked_arc(rule, |rule: &CounterStyleRule| {
         match *rule.resolved_system() {
             System::Cyclic => structs::NS_STYLE_COUNTER_SYSTEM_CYCLIC,
             System::Numeric => structs::NS_STYLE_COUNTER_SYSTEM_NUMERIC,