Bug 1181907 (Part 3) - Add CSSVariableImageTable and use it to store ImageValues generated by CSS variables. r=heycam a=ritu
☠☠ backed out by ada03589e5ea ☠ ☠
authorSeth Fowler <mark.seth.fowler@gmail.com>
Wed, 26 Aug 2015 18:19:38 -0700
changeset 289177 77513059a6e4425a19d1c803fcb8225522c375d6
parent 289176 97526d372199ca11d57c45959b853cc448a25dbd
child 289178 c3fec00fc2bd05638483035bb303ec9602176b56
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam, ritu
bugs1181907
milestone42.0a2
Bug 1181907 (Part 3) - Add CSSVariableImageTable and use it to store ImageValues generated by CSS variables. r=heycam a=ritu
layout/style/CSSVariableImageTable.h
layout/style/nsCSSDataBlock.cpp
layout/style/nsCSSValue.h
layout/style/nsStyleContext.cpp
new file mode 100644
--- /dev/null
+++ b/layout/style/CSSVariableImageTable.h
@@ -0,0 +1,176 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* 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/. */
+
+/* A global table that tracks images referenced by CSS variables. */
+
+#ifndef mozilla_CSSVariableImageTable_h
+#define mozilla_CSSVariableImageTable_h
+
+#include "nsClassHashtable.h"
+#include "nsCSSProperty.h"
+#include "nsCSSValue.h"
+#include "nsStyleContext.h"
+#include "nsTArray.h"
+
+/**
+ * CSSVariableImageTable maintains a global mapping
+ *   (nsStyleContext, nsCSSProperty) -> nsTArray<ImageValue>
+ * which allows us to track the relationship between CSS property values
+ * involving variables and any images they may reference.
+ *
+ * When properties like background-image contain a normal url(), the
+ * Declaration's data block will hold a reference to the ImageValue.  When a
+ * token stream is used, the Declaration only holds on to an
+ * nsCSSValueTokenStream object, and the ImageValue would only exist for the
+ * duration of nsRuleNode::WalkRuleTree, in the AutoCSSValueArray.  So instead
+ * when we re-parse a token stream and get an ImageValue, we record it in the
+ * CSSVariableImageTable to keep the ImageValue alive. Such ImageValues are
+ * eventually freed the next time the token stream is re-parsed, or when the
+ * associated style context is destroyed.
+ *
+ * To add ImageValues to the CSSVariableImageTable, callers should pass a lambda
+ * to CSSVariableImageTable::ReplaceAll() that calls
+ * CSSVariableImageTable::Add() for each ImageValue that needs to be added to
+ * the table. When callers are sure that the ImageValues for a given
+ * nsStyleContext won't be needed anymore, they can call
+ * CSSVariableImageTable::RemoveAll() to release them.
+ */
+
+namespace mozilla {
+namespace CSSVariableImageTable {
+
+namespace detail {
+
+typedef nsTArray<nsRefPtr<css::ImageValue>> ImageValueArray;
+typedef nsClassHashtable<nsGenericHashKey<nsCSSProperty>, ImageValueArray>
+        PerPropertyImageHashtable;
+typedef nsClassHashtable<nsPtrHashKey<nsStyleContext>, PerPropertyImageHashtable>
+        CSSVariableImageHashtable;
+
+inline CSSVariableImageHashtable& GetTable()
+{
+  static CSSVariableImageHashtable imageTable;
+  return imageTable;
+}
+
+#ifdef DEBUG
+inline bool& IsReplacing()
+{
+  static bool isReplacing = false;
+  return isReplacing;
+}
+#endif
+
+} // namespace detail
+
+/**
+ * ReplaceAll() allows callers to replace the ImageValues associated with a
+ * (nsStyleContext, nsCSSProperty) pair. The memory used by the previous list of
+ * ImageValues is automatically released.
+ *
+ * @param aContext The style context the ImageValues are associated with.
+ * @param aProp    The CSS property the ImageValues are associated with.
+ * @param aFunc    A lambda that calls CSSVariableImageTable::Add() to add new
+ *                 ImageValues which will replace the old ones.
+ */
+template <typename Lambda>
+inline void ReplaceAll(nsStyleContext* aContext,
+                       nsCSSProperty aProp,
+                       Lambda aFunc)
+{
+  MOZ_ASSERT(aContext);
+
+  auto& imageTable = detail::GetTable();
+
+  // Clear the existing image array, if any, for this property.
+  {
+    auto* perPropertyImageTable = imageTable.Get(aContext);
+    auto* imageList = perPropertyImageTable ? perPropertyImageTable->Get(aProp)
+                                            : nullptr;
+    if (imageList) {
+      imageList->ClearAndRetainStorage();
+    }
+  }
+
+#ifdef DEBUG
+  MOZ_ASSERT(!detail::IsReplacing());
+  detail::IsReplacing() = true;
+#endif
+
+  aFunc();
+
+#ifdef DEBUG
+  detail::IsReplacing() = false;
+#endif
+
+  // Clean up.
+  auto* perPropertyImageTable = imageTable.Get(aContext);
+  auto* imageList = perPropertyImageTable ? perPropertyImageTable->Get(aProp)
+                                          : nullptr;
+  if (imageList) {
+    if (imageList->IsEmpty()) {
+      // We used to have an image array for this property, but now we don't.
+      // Remove the entry in the per-property image table for this property.
+      // That may then allow us to remove the entire per-property image table.
+      perPropertyImageTable->Remove(aProp);
+      if (perPropertyImageTable->Count() == 0) {
+        imageTable.Remove(aContext);
+      }
+    } else {
+      // We still have a non-empty image array for this property. Compact the
+      // storage it's using if possible.
+      imageList->Compact();
+    }
+  }
+}
+
+/**
+ * Adds a new ImageValue @aValue to the CSSVariableImageTable, which will be
+ * associated with @aContext and @aProp.
+ *
+ * It's illegal to call this function outside of a lambda passed to
+ * CSSVariableImageTable::ReplaceAll().
+ */
+inline void
+Add(nsStyleContext* aContext, nsCSSProperty aProp, css::ImageValue* aValue)
+{
+  MOZ_ASSERT(aValue);
+  MOZ_ASSERT(aContext);
+  MOZ_ASSERT(detail::IsReplacing());
+
+  auto& imageTable = detail::GetTable();
+
+  // Ensure there's a per-property image table for this style context.
+  auto* perPropertyImageTable = imageTable.Get(aContext);
+  if (!perPropertyImageTable) {
+    perPropertyImageTable = new detail::PerPropertyImageHashtable();
+    imageTable.Put(aContext, perPropertyImageTable);
+  }
+
+  // Ensure there's an image array for this property.
+  auto* imageList = perPropertyImageTable->Get(aProp);
+  if (!imageList) {
+    imageList = new detail::ImageValueArray();
+    perPropertyImageTable->Put(aProp, imageList);
+  }
+
+  // Append the provided ImageValue to the list.
+  imageList->AppendElement(aValue);
+}
+
+/**
+ * Removes all ImageValues stored in the CSSVariableImageTable for the provided
+ * @aContext.
+ */
+inline void
+RemoveAll(nsStyleContext* aContext)
+{
+  detail::GetTable().Remove(aContext);
+}
+
+} // namespace CSSVariableImageTable
+} // namespace mozilla
+
+#endif // mozilla_CSSVariableImageTable_h
--- a/layout/style/nsCSSDataBlock.cpp
+++ b/layout/style/nsCSSDataBlock.cpp
@@ -5,26 +5,28 @@
 
 /*
  * compact representation of the property-value pairs within a CSS
  * declaration, and the code for expanding and compacting it
  */
 
 #include "nsCSSDataBlock.h"
 
+#include "CSSVariableImageTable.h"
 #include "mozilla/css/Declaration.h"
 #include "mozilla/css/ImageLoader.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/WritingModes.h"
 #include "nsIDocument.h"
 #include "nsRuleData.h"
 #include "nsStyleContext.h"
 #include "nsStyleSet.h"
 
 using namespace mozilla;
+using namespace mozilla::css;
 
 /**
  * Does a fast move of aSource to aDest.  The previous value in
  * aDest is cleanly destroyed, and aSource is cleared.  Returns
  * true if, before the copy, the value at aSource compared unequal
  * to the value at aDest; false otherwise.
  */
 static bool
@@ -46,64 +48,69 @@ ShouldIgnoreColors(nsRuleData *aRuleData
 }
 
 /**
  * Tries to call |nsCSSValue::StartImageLoad()| on an image source.
  * Image sources are specified by |url()| or |-moz-image-rect()| function.
  */
 static void
 TryToStartImageLoadOnValue(const nsCSSValue& aValue, nsIDocument* aDocument,
-                           nsCSSValueTokenStream* aTokenStream)
+                           nsStyleContext* aContext, nsCSSProperty aProperty,
+                           bool aForTokenStream)
 {
   MOZ_ASSERT(aDocument);
 
   if (aValue.GetUnit() == eCSSUnit_URL) {
     aValue.StartImageLoad(aDocument);
-    if (aTokenStream) {
-      aTokenStream->mImageValues.PutEntry(aValue.GetImageStructValue());
+    if (aForTokenStream && aContext) {
+      CSSVariableImageTable::Add(aContext, aProperty,
+                                 aValue.GetImageStructValue());
     }
   }
   else if (aValue.GetUnit() == eCSSUnit_Image) {
     // If we already have a request, see if this document needs to clone it.
     imgIRequest* request = aValue.GetImageValue(nullptr);
 
     if (request) {
-      mozilla::css::ImageValue* imageValue = aValue.GetImageStructValue();
+      ImageValue* imageValue = aValue.GetImageStructValue();
       aDocument->StyleImageLoader()->MaybeRegisterCSSImage(imageValue);
-      if (aTokenStream) {
-        aTokenStream->mImageValues.PutEntry(imageValue);
+      if (aForTokenStream && aContext) {
+        CSSVariableImageTable::Add(aContext, aProperty, imageValue);
       }
     }
   }
   else if (aValue.EqualsFunction(eCSSKeyword__moz_image_rect)) {
     nsCSSValue::Array* arguments = aValue.GetArrayValue();
     MOZ_ASSERT(arguments->Count() == 6, "unexpected num of arguments");
 
     const nsCSSValue& image = arguments->Item(1);
-    TryToStartImageLoadOnValue(image, aDocument, aTokenStream);
+    TryToStartImageLoadOnValue(image, aDocument, aContext, aProperty,
+                               aForTokenStream);
   }
 }
 
 static void
 TryToStartImageLoad(const nsCSSValue& aValue, nsIDocument* aDocument,
-                    nsCSSProperty aProperty,
-                    nsCSSValueTokenStream* aTokenStream)
+                    nsStyleContext* aContext, nsCSSProperty aProperty,
+                    bool aForTokenStream)
 {
   if (aValue.GetUnit() == eCSSUnit_List) {
     for (const nsCSSValueList* l = aValue.GetListValue(); l; l = l->mNext) {
-      TryToStartImageLoad(l->mValue, aDocument, aProperty, aTokenStream);
+      TryToStartImageLoad(l->mValue, aDocument, aContext, aProperty,
+                          aForTokenStream);
     }
   } else if (nsCSSProps::PropHasFlags(aProperty,
                                       CSS_PROPERTY_IMAGE_IS_IN_ARRAY_0)) {
     if (aValue.GetUnit() == eCSSUnit_Array) {
       TryToStartImageLoadOnValue(aValue.GetArrayValue()->Item(0), aDocument,
-                                 aTokenStream);
+                                 aContext, aProperty, aForTokenStream);
     }
   } else {
-    TryToStartImageLoadOnValue(aValue, aDocument, aTokenStream);
+    TryToStartImageLoadOnValue(aValue, aDocument, aContext, aProperty,
+                               aForTokenStream);
   }
 }
 
 static inline bool
 ShouldStartImageLoads(nsRuleData *aRuleData, nsCSSProperty aProperty)
 {
   // Don't initiate image loads for if-visited styles.  This is
   // important because:
@@ -124,32 +131,27 @@ MapSinglePropertyInto(nsCSSProperty aPro
 {
     MOZ_ASSERT(aValue->GetUnit() != eCSSUnit_Null, "oops");
 
     // Although aTarget is the nsCSSValue we are going to write into,
     // we also look at its value before writing into it.  This is done
     // when aTarget is a token stream value, which is the case when we
     // have just re-parsed a property that had a variable reference (in
     // nsCSSParser::ParsePropertyWithVariableReferences).  TryToStartImageLoad
-    // then records any resulting ImageValue objects on the
-    // nsCSSValueTokenStream object we found on aTarget.  See the comment
-    // above nsCSSValueTokenStream::mImageValues for why.
+    // then records any resulting ImageValue objects in the
+    // CSSVariableImageTable, to give them the appropriate lifetime.
     MOZ_ASSERT(aTarget->GetUnit() == eCSSUnit_TokenStream ||
                aTarget->GetUnit() == eCSSUnit_Null,
                "aTarget must only be a token stream (when re-parsing "
                "properties with variable references) or null");
 
-    nsCSSValueTokenStream* tokenStream =
-        aTarget->GetUnit() == eCSSUnit_TokenStream ?
-            aTarget->GetTokenStreamValue() :
-            nullptr;
-
     if (ShouldStartImageLoads(aRuleData, aProp)) {
         nsIDocument* doc = aRuleData->mPresContext->Document();
-        TryToStartImageLoad(*aValue, doc, aProp, tokenStream);
+        TryToStartImageLoad(*aValue, doc, aRuleData->mStyleContext, aProp,
+                            aTarget->GetUnit() == eCSSUnit_TokenStream);
     }
     *aTarget = *aValue;
     if (nsCSSProps::PropHasFlags(aProp,
             CSS_PROPERTY_IGNORED_WHEN_COLORS_DISABLED) &&
         ShouldIgnoreColors(aRuleData))
     {
         if (aProp == eCSSProperty_background_color) {
             // Force non-'transparent' background
@@ -697,17 +699,19 @@ nsCSSExpandedDataBlock::MapRuleInfoInto(
     uint8_t wm = WritingMode(aRuleData->mStyleContext).GetBits();
     aRuleData->mConditions.SetWritingModeDependency(wm);
   }
 
   nsCSSValue* dest = aRuleData->ValueFor(physicalProp);
   MOZ_ASSERT(dest->GetUnit() == eCSSUnit_TokenStream &&
              dest->GetTokenStreamValue()->mPropertyID == aPropID);
 
-  MapSinglePropertyInto(physicalProp, src, dest, aRuleData);
+  CSSVariableImageTable::ReplaceAll(aRuleData->mStyleContext, aPropID, [=] {
+    MapSinglePropertyInto(physicalProp, src, dest, aRuleData);
+  });
 }
 
 #ifdef DEBUG
 void
 nsCSSExpandedDataBlock::DoAssertInitialState()
 {
     mPropertiesSet.AssertIsEmpty("not initial state");
     mPropertiesImportant.AssertIsEmpty("not initial state");
--- a/layout/style/nsCSSValue.h
+++ b/layout/style/nsCSSValue.h
@@ -1537,28 +1537,16 @@ public:
   nsCOMPtr<nsIURI> mBaseURI;
   nsCOMPtr<nsIURI> mSheetURI;
   nsCOMPtr<nsIPrincipal> mSheetPrincipal;
   // XXX Should store sheet here (see Bug 952338)
   // mozilla::CSSStyleSheet* mSheet;
   uint32_t mLineNumber;
   uint32_t mLineOffset;
 
-  // This table is used to hold a reference on to any ImageValue that results
-  // from re-parsing this token stream at computed value time.  When properties
-  // like background-image contain a normal url(), the Declaration's data block
-  // will hold a reference to the ImageValue.  When a token stream is used,
-  // the Declaration only holds on to this nsCSSValueTokenStream object, and
-  // the ImageValue would only exist for the duration of
-  // nsRuleNode::WalkRuleTree, in the AutoCSSValueArray.  So instead when
-  // we re-parse a token stream and get an ImageValue, we record it in this
-  // table so that the Declaration can be the object that keeps holding
-  // a reference to it.
-  nsTHashtable<nsRefPtrHashKey<mozilla::css::ImageValue> > mImageValues;
-
 private:
   nsCSSValueTokenStream(const nsCSSValueTokenStream& aOther) = delete;
   nsCSSValueTokenStream& operator=(const nsCSSValueTokenStream& aOther) = delete;
 };
 
 class nsCSSValueFloatColor final {
 public:
   nsCSSValueFloatColor(float aComponent1, float aComponent2, float aComponent3,
--- a/layout/style/nsStyleContext.cpp
+++ b/layout/style/nsStyleContext.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* 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/. */
  
 /* the interface (to internal code) for retrieving computed style data */
 
+#include "CSSVariableImageTable.h"
 #include "mozilla/DebugOnly.h"
 
 #include "nsCSSAnonBoxes.h"
 #include "nsStyleConsts.h"
 #include "nsString.h"
 #include "nsPresContext.h"
 #include "nsIStyleRule.h"
 
@@ -160,16 +161,19 @@ nsStyleContext::~nsStyleContext()
     mParent->Release();
   }
 
   // Free up our data structs.
   mCachedInheritedData.DestroyStructs(mBits, presContext);
   if (mCachedResetData) {
     mCachedResetData->Destroy(mBits, presContext);
   }
+
+  // Free any ImageValues we were holding on to for CSS variable values.
+  CSSVariableImageTable::RemoveAll(this);
 }
 
 #ifdef DEBUG
 void
 nsStyleContext::AssertStructsNotUsedElsewhere(
                                        nsStyleContext* aDestroyingContext,
                                        int32_t aLevels) const
 {