Bug 1543672 - The counters code should use atoms rather than strings. r=mats,boris
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 15 Apr 2019 20:11:45 +0000
changeset 469560 3654d1720dab
parent 469559 81be11ad9dfb
child 469561 ddb852bfc987
push id35874
push userccoroiu@mozilla.com
push dateTue, 16 Apr 2019 04:04:58 +0000
treeherdermozilla-central@be3f40425b52 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats, boris
bugs1543672
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 1543672 - The counters code should use atoms rather than strings. r=mats,boris Servo already atomizes the counter names, it makes no sense to copy the string rather than bumping the refcount. Differential Revision: https://phabricator.services.mozilla.com/D27061
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCounterManager.cpp
layout/base/nsCounterManager.h
layout/generic/nsBulletFrame.cpp
layout/style/nsStyleStruct.h
servo/components/style/properties/gecko.mako.rs
servo/components/style/values/generics/counters.rs
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -9551,18 +9551,17 @@ inline void nsCSSFrameConstructor::Const
     // display:list-item boxes affects the start value of the "list-item"
     // counter when an <ol reversed> element doesn't have an explicit start
     // value.
     if (!listItemListIsDirty &&
         iter.item().mComputedStyle->StyleList()->mMozListReversed ==
             StyleMozListReversed::True &&
         iter.item().mComputedStyle->StyleDisplay()->mDisplay ==
             StyleDisplay::ListItem) {
-      auto* list =
-          mCounterManager.CounterListFor(NS_LITERAL_STRING("list-item"));
+      auto* list = mCounterManager.CounterListFor(nsGkAtoms::list_item);
       list->SetDirty();
       CountersDirty();
       listItemListIsDirty = true;
     }
     ConstructFramesFromItem(aState, iter, aParentFrame, aFrameList);
   }
 
   VerifyGridFlexContainerChildren(aParentFrame, aFrameList);
--- a/layout/base/nsCounterManager.cpp
+++ b/layout/base/nsCounterManager.cpp
@@ -230,24 +230,24 @@ bool nsCounterManager::AddCounterChanges
   // for addition at the end of the list.
   for (int32_t i : IntegerRange(styleContent->CounterResetCount())) {
     dirty |= AddCounterChangeNode(aFrame, i, styleContent->CounterResetAt(i),
                                   nsCounterChangeNode::RESET);
   }
   bool hasListItemIncrement = false;
   for (int32_t i : IntegerRange(styleContent->CounterIncrementCount())) {
     const nsStyleCounterData& increment = styleContent->CounterIncrementAt(i);
-    hasListItemIncrement |= increment.mCounter.EqualsLiteral("list-item");
+    hasListItemIncrement |= increment.mCounter == nsGkAtoms::list_item;
     dirty |= AddCounterChangeNode(aFrame, i, increment,
                                   nsCounterChangeNode::INCREMENT);
   }
   if (requiresListItemIncrement && !hasListItemIncrement) {
     bool reversed =
         aFrame->StyleList()->mMozListReversed == StyleMozListReversed::True;
-    nsStyleCounterData listItemIncrement{NS_LITERAL_STRING("list-item"),
+    nsStyleCounterData listItemIncrement{nsGkAtoms::list_item,
                                          reversed ? -1 : 1};
     dirty |=
         AddCounterChangeNode(aFrame, styleContent->CounterIncrementCount() + 1,
                              listItemIncrement, nsCounterChangeNode::INCREMENT);
   }
   for (int32_t i : IntegerRange(styleContent->CounterSetCount())) {
     dirty |= AddCounterChangeNode(aFrame, i, styleContent->CounterSetAt(i),
                                   nsCounterChangeNode::SET);
@@ -273,17 +273,18 @@ bool nsCounterManager::AddCounterChangeN
   // Don't call Calc() if the list is already dirty -- it'll be recalculated
   // anyway, and trying to calculate with a dirty list doesn't work.
   if (MOZ_LIKELY(!counterList->IsDirty())) {
     node->Calc(counterList);
   }
   return false;
 }
 
-nsCounterList* nsCounterManager::CounterListFor(const nsAString& aCounterName) {
+nsCounterList* nsCounterManager::CounterListFor(nsAtom* aCounterName) {
+  MOZ_ASSERT(aCounterName);
   return mNames.LookupForAdd(aCounterName).OrInsert([]() {
     return new nsCounterList();
   });
 }
 
 void nsCounterManager::RecalcAll() {
   for (auto iter = mNames.Iter(); !iter.Done(); iter.Next()) {
     nsCounterList* list = iter.UserData();
@@ -312,17 +313,17 @@ bool nsCounterManager::DestroyNodesFor(n
   }
   return destroyedAny;
 }
 
 #ifdef DEBUG
 void nsCounterManager::Dump() {
   printf("\n\nCounter Manager Lists:\n");
   for (auto iter = mNames.Iter(); !iter.Done(); iter.Next()) {
-    printf("Counter named \"%s\":\n", NS_ConvertUTF16toUTF8(iter.Key()).get());
+    printf("Counter named \"%s\":\n", nsAtomCString(iter.Key()).get());
 
     nsCounterList* list = iter.UserData();
     int32_t i = 0;
     for (nsCounterNode* node = list->First(); node; node = list->Next(node)) {
       const char* types[] = {"RESET", "SET", "INCREMENT", "USE"};
       printf(
           "  Node #%d @%p frame=%p index=%d type=%s valAfter=%d\n"
           "       scope-start=%p scope-prev=%p",
--- a/layout/base/nsCounterManager.h
+++ b/layout/base/nsCounterManager.h
@@ -209,17 +209,17 @@ class nsCounterList : public nsGenConLis
  */
 class nsCounterManager {
  public:
   // Returns true if dirty
   bool AddCounterChanges(nsIFrame* aFrame);
 
   // Gets the appropriate counter list, creating it if necessary.
   // Guaranteed to return non-null. (Uses an infallible hashtable API.)
-  nsCounterList* CounterListFor(const nsAString& aCounterName);
+  nsCounterList* CounterListFor(nsAtom* aCounterName);
 
   // Clean up data in any dirty counter lists.
   void RecalcAll();
 
   // Set all counter lists dirty
   void SetAllDirty();
 
   // Destroy nodes for the frame in any lists, and return whether any
@@ -257,12 +257,12 @@ class nsCounterManager {
   }
 
  private:
   // for |AddCounterChanges| only
   bool AddCounterChangeNode(nsIFrame* aFrame, int32_t aIndex,
                             const nsStyleCounterData& aCounterData,
                             nsCounterNode::Type aType);
 
-  nsClassHashtable<nsStringHashKey, nsCounterList> mNames;
+  nsClassHashtable<nsRefPtrHashKey<nsAtom>, nsCounterList> mNames;
 };
 
 #endif /* nsCounterManager_h_ */
--- a/layout/generic/nsBulletFrame.cpp
+++ b/layout/generic/nsBulletFrame.cpp
@@ -822,17 +822,17 @@ ImgDrawResult nsBulletFrame::PaintBullet
 
   return br->Paint(aRenderingContext, aPt, aDirtyRect, aFlags,
                    aDisableSubpixelAA, this);
 }
 
 int32_t nsBulletFrame::Ordinal(bool aDebugFromA11y) const {
   auto* fc = PresShell()->FrameConstructor();
   auto* cm = fc->CounterManager();
-  auto* list = cm->CounterListFor(NS_LITERAL_STRING("list-item"));
+  auto* list = cm->CounterListFor(nsGkAtoms::list_item);
   MOZ_ASSERT(aDebugFromA11y || (list && !list->IsDirty()));
   nsIFrame* listItem = GetParent()->GetContent()->GetPrimaryFrame();
   int32_t value = 0;
   for (auto* node = list->First(); node; node = list->Next(node)) {
     if (node->mPseudoFrame == listItem) {
       value = node->mValueAfter;
     }
   }
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -2373,17 +2373,17 @@ class nsStyleContentData {
 
   const nsStyleContentAttr* GetAttr() const {
     MOZ_ASSERT(mType == StyleContentType::Attr);
     MOZ_ASSERT(mContent.mAttr);
     return mContent.mAttr;
   }
 
   struct CounterFunction {
-    nsString mIdent;
+    RefPtr<nsAtom> mIdent;
     // This is only used when it is a counters() function.
     nsString mSeparator;
     mozilla::CounterStylePtr mCounterStyle;
 
     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CounterFunction)
 
     bool operator==(const CounterFunction& aOther) const;
     bool operator!=(const CounterFunction& aOther) const {
@@ -2435,17 +2435,17 @@ class nsStyleContentData {
     char16_t* mString;
     nsStyleContentAttr* mAttr;
     nsStyleImageRequest* mImage;
     CounterFunction* mCounters;
   } mContent;
 };
 
 struct nsStyleCounterData {
-  nsString mCounter;
+  RefPtr<nsAtom> mCounter;
   int32_t mValue;
 
   bool operator==(const nsStyleCounterData& aOther) const {
     return mValue == aOther.mValue && mCounter == aOther.mCounter;
   }
 
   bool operator!=(const nsStyleCounterData& aOther) const {
     return !(*this == aOther);
@@ -2484,50 +2484,48 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsSt
     return mIncrements[aIndex];
   }
 
   void AllocateCounterIncrements(uint32_t aCount) {
     mIncrements.Clear();
     mIncrements.SetLength(aCount);
   }
 
-  void SetCounterIncrementAt(uint32_t aIndex, const nsString& aCounter,
+  void SetCounterIncrementAt(uint32_t aIndex, nsAtom* aCounter,
                              int32_t aIncrement) {
     mIncrements[aIndex].mCounter = aCounter;
     mIncrements[aIndex].mValue = aIncrement;
   }
 
   uint32_t CounterResetCount() const { return mResets.Length(); }
   const nsStyleCounterData& CounterResetAt(uint32_t aIndex) const {
     return mResets[aIndex];
   }
 
   void AllocateCounterResets(uint32_t aCount) {
     mResets.Clear();
     mResets.SetLength(aCount);
   }
 
-  void SetCounterResetAt(uint32_t aIndex, const nsString& aCounter,
-                         int32_t aValue) {
+  void SetCounterResetAt(uint32_t aIndex, nsAtom* aCounter, int32_t aValue) {
     mResets[aIndex].mCounter = aCounter;
     mResets[aIndex].mValue = aValue;
   }
 
   uint32_t CounterSetCount() const { return mSets.Length(); }
   const nsStyleCounterData& CounterSetAt(uint32_t aIndex) const {
     return mSets[aIndex];
   }
 
   void AllocateCounterSets(uint32_t aCount) {
     mSets.Clear();
     mSets.SetLength(aCount);
   }
 
-  void SetCounterSetAt(uint32_t aIndex, const nsString& aCounter,
-                       int32_t aValue) {
+  void SetCounterSetAt(uint32_t aIndex, nsAtom* aCounter, int32_t aValue) {
     mSets[aIndex].mCounter = aCounter;
     mSets[aIndex].mValue = aValue;
   }
 
  protected:
   nsTArray<nsStyleContentData> mContents;
   nsTArray<nsStyleCounterData> mIncrements;
   nsTArray<nsStyleCounterData> mResets;
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -4450,26 +4450,28 @@ clip-path
             let ptr = vec.as_mut_ptr();
             mem::forget(vec);
             ptr
         }
 
         fn set_counter_function(
             data: &mut nsStyleContentData,
             content_type: StyleContentType,
-            name: &CustomIdent,
+            name: CustomIdent,
             sep: &str,
             style: CounterStyleOrNone,
         ) {
             debug_assert!(content_type == StyleContentType::Counter ||
                           content_type == StyleContentType::Counters);
             let counter_func = unsafe {
                 bindings::Gecko_SetCounterFunction(data, content_type).as_mut().unwrap()
             };
-            counter_func.mIdent.assign(name.0.as_slice());
+            counter_func.mIdent.set_move(unsafe {
+                RefPtr::from_addrefed(name.0.into_addrefed())
+            });
             if content_type == StyleContentType::Counters {
                 counter_func.mSeparator.assign_str(sep);
             }
             style.to_gecko_value(&mut counter_func.mCounterStyle);
         }
 
         match v {
             Content::None |
@@ -4488,24 +4490,24 @@ clip-path
                 }
                 self.gecko.mContents[0].mType = StyleContentType::AltContent;
             },
             Content::Items(items) => {
                 unsafe {
                     Gecko_ClearAndResizeStyleContents(&mut self.gecko,
                                                       items.len() as u32);
                 }
-                for (i, item) in items.into_iter().enumerate() {
+                for (i, item) in items.into_vec().into_iter().enumerate() {
                     // NB: Gecko compares the mString value if type is not image
                     // or URI independently of whatever gets there. In the quote
                     // cases, they set it to null, so do the same here.
                     unsafe {
                         *self.gecko.mContents[i].mContent.mString.as_mut() = ptr::null_mut();
                     }
-                    match *item {
+                    match item {
                         ContentItem::String(ref value) => {
                             self.gecko.mContents[i].mType = StyleContentType::String;
                             unsafe {
                                 // NB: we share allocators, so doing this is fine.
                                 *self.gecko.mContents[i].mContent.mString.as_mut() =
                                     as_utf16_and_forget(&value);
                             }
                         }
@@ -4531,32 +4533,32 @@ clip-path
                         ContentItem::OpenQuote
                             => self.gecko.mContents[i].mType = StyleContentType::OpenQuote,
                         ContentItem::CloseQuote
                             => self.gecko.mContents[i].mType = StyleContentType::CloseQuote,
                         ContentItem::NoOpenQuote
                             => self.gecko.mContents[i].mType = StyleContentType::NoOpenQuote,
                         ContentItem::NoCloseQuote
                             => self.gecko.mContents[i].mType = StyleContentType::NoCloseQuote,
-                        ContentItem::Counter(ref name, ref style) => {
+                        ContentItem::Counter(name, style) => {
                             set_counter_function(
                                 &mut self.gecko.mContents[i],
                                 StyleContentType::Counter,
-                                &name,
+                                name,
                                 "",
-                                style.clone(),
+                                style,
                             );
                         }
-                        ContentItem::Counters(ref name, ref sep, ref style) => {
+                        ContentItem::Counters(name, sep, style) => {
                             set_counter_function(
                                 &mut self.gecko.mContents[i],
                                 StyleContentType::Counters,
-                                &name,
+                                name,
                                 &sep,
-                                style.clone(),
+                                style,
                             );
                         }
                         ContentItem::Url(ref url) => {
                             unsafe {
                                 bindings::Gecko_SetContentDataImageValue(
                                     &mut self.gecko.mContents[i],
                                     url.url_value_ptr(),
                                 )
@@ -4622,17 +4624,19 @@ clip-path
                             };
                             (ns, Atom::from_raw(s.mName.mRawPtr))
                         };
                         ContentItem::Attr(Attr { namespace, attribute })
                     },
                     StyleContentType::Counter | StyleContentType::Counters => {
                         let gecko_function =
                             unsafe { &**gecko_content.mContent.mCounters.as_ref() };
-                        let ident = CustomIdent(Atom::from(&*gecko_function.mIdent));
+                        let ident = CustomIdent(unsafe {
+                            Atom::from_raw(gecko_function.mIdent.mRawPtr)
+                        });
                         let style =
                             CounterStyleOrNone::from_gecko_value(&gecko_function.mCounterStyle);
                         let style = match style {
                             Either::First(counter_style) => counter_style,
                             Either::Second(_) =>
                                 unreachable!("counter function shouldn't have single string type"),
                         };
                         if gecko_content.mType == StyleContentType::Counter {
@@ -4659,18 +4663,20 @@ clip-path
 
     % for counter_property in ["Increment", "Reset", "Set"]:
         pub fn set_counter_${counter_property.lower()}(
             &mut self,
             v: longhands::counter_${counter_property.lower()}::computed_value::T
         ) {
             unsafe {
                 bindings::Gecko_ClearAndResizeCounter${counter_property}s(&mut self.gecko, v.len() as u32);
-                for (i, ref pair) in v.iter().enumerate() {
-                    self.gecko.m${counter_property}s[i].mCounter.assign(pair.name.0.as_slice());
+                for (i, pair) in v.0.into_vec().into_iter().enumerate() {
+                    self.gecko.m${counter_property}s[i].mCounter.set_move(
+                        RefPtr::from_addrefed(pair.name.0.into_addrefed())
+                    );
                     self.gecko.m${counter_property}s[i].mValue = pair.value;
                 }
             }
         }
 
         pub fn copy_counter_${counter_property.lower()}_from(&mut self, other: &Self) {
             unsafe {
                 bindings::Gecko_CopyCounter${counter_property}sFrom(&mut self.gecko, &other.gecko)
@@ -4685,17 +4691,19 @@ clip-path
             &self
         ) -> longhands::counter_${counter_property.lower()}::computed_value::T {
             use crate::values::generics::counters::CounterPair;
             use crate::values::CustomIdent;
 
             longhands::counter_${counter_property.lower()}::computed_value::T::new(
                 self.gecko.m${counter_property}s.iter().map(|ref gecko_counter| {
                     CounterPair {
-                        name: CustomIdent(Atom::from(gecko_counter.mCounter.to_string())),
+                        name: CustomIdent(unsafe {
+                            Atom::from_raw(gecko_counter.mCounter.mRawPtr)
+                        }),
                         value: gecko_counter.mValue,
                     }
                 }).collect()
             )
         }
     % endfor
 </%self:impl_trait>
 
--- a/servo/components/style/values/generics/counters.rs
+++ b/servo/components/style/values/generics/counters.rs
@@ -40,17 +40,17 @@ pub struct CounterPair<Integer> {
     MallocSizeOf,
     PartialEq,
     SpecifiedValueInfo,
     ToComputedValue,
     ToCss,
     ToResolvedValue,
     ToShmem,
 )]
-pub struct CounterIncrement<I>(Counters<I>);
+pub struct CounterIncrement<I>(pub Counters<I>);
 
 impl<I> CounterIncrement<I> {
     /// Returns a new value for `counter-increment`.
     #[inline]
     pub fn new(counters: Vec<CounterPair<I>>) -> Self {
         CounterIncrement(Counters(counters.into_boxed_slice()))
     }
 }
@@ -72,17 +72,17 @@ impl<I> Deref for CounterIncrement<I> {
     MallocSizeOf,
     PartialEq,
     SpecifiedValueInfo,
     ToComputedValue,
     ToCss,
     ToResolvedValue,
     ToShmem,
 )]
-pub struct CounterSetOrReset<I>(Counters<I>);
+pub struct CounterSetOrReset<I>(pub Counters<I>);
 
 impl<I> CounterSetOrReset<I> {
     /// Returns a new value for `counter-set` / `counter-reset`.
     #[inline]
     pub fn new(counters: Vec<CounterPair<I>>) -> Self {
         CounterSetOrReset(Counters(counters.into_boxed_slice()))
     }
 }
@@ -97,30 +97,34 @@ impl<I> Deref for CounterSetOrReset<I> {
 }
 
 /// A generic value for lists of counters.
 ///
 /// Keyword `none` is represented by an empty vector.
 #[derive(
     Clone,
     Debug,
+    Default,
     MallocSizeOf,
     PartialEq,
     SpecifiedValueInfo,
     ToComputedValue,
     ToCss,
     ToResolvedValue,
     ToShmem,
 )]
 pub struct Counters<I>(#[css(iterable, if_empty = "none")] Box<[CounterPair<I>]>);
 
-impl<I> Default for Counters<I> {
+impl<I> Counters<I> {
+    /// Move out the Box into a vector. This could just return the Box<>, but
+    /// Vec<> is a bit more convenient because Box<[T]> doesn't implement
+    /// IntoIter: https://github.com/rust-lang/rust/issues/59878
     #[inline]
-    fn default() -> Self {
-        Counters(vec![].into_boxed_slice())
+    pub fn into_vec(self) -> Vec<CounterPair<I>> {
+        self.0.into_vec()
     }
 }
 
 #[cfg(feature = "servo")]
 type CounterStyleType = ListStyleType;
 
 #[cfg(feature = "gecko")]
 type CounterStyleType = CounterStyleOrNone;