Bug 1485465 - Cleanup SVG mapped attribute parser. r=dholbert
☠☠ backed out by 51b6bdb9197c ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 22 Aug 2018 21:30:36 +0200
changeset 488175 675288974991830a083a7d8d4f761fa8ab4db493
parent 488174 03b5cb64d690ad396470e00487e1ef8d5efa9f37
child 488176 f7d550d783449a155a5681f0db3939bd9ecad58b
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1485465, 1425700
milestone63.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 1485465 - Cleanup SVG mapped attribute parser. r=dholbert * Remove use counters since they're useless right now and I'm re-implementing them in bug 1425700 (I'll cleanup UseCounterFor and friends later). * Rename some stuff to be clearer (TakeDeclarationBlock instead of GetDeclarationBlock). * Don't allocate a new URLExtraData for each mapped attribute, cache them in the parser instead. CLOSED TREE Differential Revision: https://phabricator.services.mozilla.com/D4006
dom/svg/nsSVGElement.cpp
--- a/dom/svg/nsSVGElement.cpp
+++ b/dom/svg/nsSVGElement.cpp
@@ -1123,159 +1123,125 @@ nsSVGElement::IsFocusableInternal(int32_
   return isFocusable;
 }
 
 //------------------------------------------------------------------------
 // Helper class: MappedAttrParser, for parsing values of mapped attributes
 
 namespace {
 
-class MOZ_STACK_CLASS MappedAttrParser {
+class MOZ_STACK_CLASS MappedAttrParser
+{
 public:
-  MappedAttrParser(css::Loader* aLoader,
-                   nsIURI* aDocURI,
-                   already_AddRefed<nsIURI> aBaseURI,
-                   nsSVGElement* aElement);
-  ~MappedAttrParser();
-
-  // Parses a mapped attribute value.
+  explicit MappedAttrParser(nsSVGElement& aElement)
+    : mElement(aElement)
+  { }
+
+  ~MappedAttrParser()
+  {
+    MOZ_ASSERT(!mDecl,
+               "If mDecl was initialized, it should have been returned via "
+               "TakeDeclarationBlock (and have its pointer cleared)");
+  }
+
   void ParseMappedAttrValue(nsAtom* aMappedAttrName,
                             const nsAString& aMappedAttrValue);
 
   // If we've parsed any values for mapped attributes, this method returns the
-  // already_AddRefed css::Declaration that incorporates the parsed
-  // values. Otherwise, this method returns null.
-  already_AddRefed<DeclarationBlock> GetDeclarationBlock();
+  // already_AddRefed css::Declaration that incorporates the parsed values.
+  // Otherwise, this method returns null.
+  already_AddRefed<DeclarationBlock> TakeDeclarationBlock()
+  {
+    return mDecl.forget();
+  }
+
+  URLExtraData& EnsureExtraData()
+  {
+    if (!mExtraData) {
+      nsIDocument& doc = *mElement.OwnerDoc();
+      mExtraData = new URLExtraData(mElement.GetBaseURI(),
+                                    do_AddRef(doc.GetDocumentURI()),
+                                    do_AddRef(mElement.NodePrincipal()));
+    }
+    return *mExtraData;
+  }
 
 private:
-  // MEMBER DATA
-  // -----------
-  css::Loader*      mLoader;
-
-  // Arguments for nsCSSParser::ParseProperty
-  nsIURI*           mDocURI;
-  nsCOMPtr<nsIURI>  mBaseURI;
-
-  // Declaration for storing parsed values (lazily initialized)
+  // Declaration for storing parsed values (lazily initialized).
   RefPtr<DeclarationBlock> mDecl;
-
-  // For reporting use counters
-  nsSVGElement*     mElement;
+  // URL data for parsing stuff. Also lazy.
+  RefPtr<URLExtraData> mExtraData;
+  nsSVGElement& mElement;
 };
 
-MappedAttrParser::MappedAttrParser(css::Loader* aLoader,
-                                   nsIURI* aDocURI,
-                                   already_AddRefed<nsIURI> aBaseURI,
-                                   nsSVGElement* aElement)
-  : mLoader(aLoader)
-  , mDocURI(aDocURI)
-  , mBaseURI(aBaseURI)
-  , mElement(aElement)
-{
-}
-
-MappedAttrParser::~MappedAttrParser()
-{
-  MOZ_ASSERT(!mDecl,
-             "If mDecl was initialized, it should have been returned via "
-             "GetDeclarationBlock (and had its pointer cleared)");
-}
-
 void
 MappedAttrParser::ParseMappedAttrValue(nsAtom* aMappedAttrName,
                                        const nsAString& aMappedAttrValue)
 {
   if (!mDecl) {
     mDecl = new DeclarationBlock();
   }
 
-  // Get the nsCSSPropertyID ID for our mapped attribute.
   nsCSSPropertyID propertyID =
     nsCSSProps::LookupProperty(nsDependentAtomString(aMappedAttrName));
   if (propertyID != eCSSProperty_UNKNOWN) {
-    bool changed = false; // outparam for ParseProperty.
     NS_ConvertUTF16toUTF8 value(aMappedAttrValue);
-    // FIXME (bug 1343964): Figure out a better solution for sending the base uri to servo
-    RefPtr<URLExtraData> data = new URLExtraData(mBaseURI, mDocURI,
-                                                 mElement->NodePrincipal());
-    changed = Servo_DeclarationBlock_SetPropertyById(
-      mDecl->Raw(), propertyID, &value, false, data,
-      ParsingMode::AllowUnitlessLength,
-      mElement->OwnerDoc()->GetCompatibilityMode(), mLoader, { });
-
-    if (changed) {
-      // The normal reporting of use counters by the nsCSSParser won't happen
-      // since it doesn't have a sheet.
-      if (nsCSSProps::IsShorthand(propertyID)) {
-        CSSPROPS_FOR_SHORTHAND_SUBPROPERTIES(subprop, propertyID,
-                                             CSSEnabledState::eForAllContent) {
-          UseCounter useCounter = nsCSSProps::UseCounterFor(*subprop);
-          if (useCounter != eUseCounter_UNKNOWN) {
-            mElement->OwnerDoc()->SetDocumentAndPageUseCounter(useCounter);
-          }
-        }
-      } else {
-        UseCounter useCounter = nsCSSProps::UseCounterFor(propertyID);
-        if (useCounter != eUseCounter_UNKNOWN) {
-          mElement->OwnerDoc()->SetDocumentAndPageUseCounter(useCounter);
-        }
-      }
-    }
+    nsIDocument& doc = *mElement.OwnerDoc();
+    Servo_DeclarationBlock_SetPropertyById(mDecl->Raw(),
+                                           propertyID,
+                                           &value,
+                                           false,
+                                           &EnsureExtraData(),
+                                           ParsingMode::AllowUnitlessLength,
+                                           doc.GetCompatibilityMode(),
+                                           doc.CSSLoader(),
+                                           { });
     return;
   }
   MOZ_ASSERT(aMappedAttrName == nsGkAtoms::lang,
              "Only 'lang' should be unrecognized!");
   // nsCSSParser doesn't know about 'lang', so we need to handle it specially.
   if (aMappedAttrName == nsGkAtoms::lang) {
     propertyID = eCSSProperty__x_lang;
     RefPtr<nsAtom> atom = NS_Atomize(aMappedAttrValue);
     Servo_DeclarationBlock_SetIdentStringValue(mDecl->Raw(), propertyID, atom);
   }
 }
 
-already_AddRefed<DeclarationBlock>
-MappedAttrParser::GetDeclarationBlock()
-{
-  return mDecl.forget();
-}
-
 } // namespace
 
 //----------------------------------------------------------------------
 // Implementation Helpers:
 
 void
 nsSVGElement::UpdateContentDeclarationBlock()
 {
-  NS_ASSERTION(!mContentDeclarationBlock,
-               "we already have a content declaration block");
-
-  uint32_t attrCount = mAttrs.AttrCount();
-  if (!attrCount) {
-    // nothing to do
-    return;
-  }
-
-  nsIDocument* doc = OwnerDoc();
-  MappedAttrParser mappedAttrParser(doc->CSSLoader(), doc->GetDocumentURI(),
-                                    GetBaseURI(), this);
-
-  for (uint32_t i = 0; i < attrCount; ++i) {
-    const nsAttrName* attrName = mAttrs.AttrNameAt(i);
-    if (!attrName->IsAtom() || !IsAttributeMapped(attrName->Atom()))
+  MOZ_ASSERT(!mContentDeclarationBlock,
+             "we already have a content declaration block");
+
+  MappedAttrParser parser(*this);
+
+  uint32_t i = 0;
+  while (BorrowedAttrInfo info = GetAttrInfoAt(i++)) {
+    const nsAttrName* attrName = info.mName;
+    if (!attrName->IsAtom() || !IsAttributeMapped(attrName->Atom())) {
       continue;
-
+    }
+
+    // FIXME(emilio): This check is dead, since IsAtom() implies that
+    // NamespaceID() == None.
     if (attrName->NamespaceID() != kNameSpaceID_None &&
         !attrName->Equals(nsGkAtoms::lang, kNameSpaceID_XML)) {
       continue;
     }
 
     if (attrName->Equals(nsGkAtoms::lang, kNameSpaceID_None) &&
         HasAttr(kNameSpaceID_XML, nsGkAtoms::lang)) {
-      continue; // xml:lang has precedence
+      // xml:lang has precedence, and will get set via Gecko_GetXMLLangValue().
+      continue;
     }
 
     if (IsSVGElement(nsGkAtoms::svg)) {
       // Special case: we don't want <svg> 'width'/'height' mapped into style
       // if the attribute value isn't a valid <length> according to SVG (which
       // only supports a subset of the CSS <length> values). We don't enforce
       // this by checking the attribute value in SVGSVGElement::
       // IsAttributeMapped since we don't want that method to depend on the
@@ -1287,20 +1253,21 @@ nsSVGElement::UpdateContentDeclarationBl
       }
       if (attrName->Atom() == nsGkAtoms::height &&
           !GetAnimatedLength(nsGkAtoms::height)->HasBaseVal()) {
         continue;
       }
     }
 
     nsAutoString value;
-    mAttrs.AttrAt(i)->ToString(value);
-    mappedAttrParser.ParseMappedAttrValue(attrName->Atom(), value);
+    info.mValue->ToString(value);
+    parser.ParseMappedAttrValue(attrName->Atom(), value);
   }
-  mContentDeclarationBlock = mappedAttrParser.GetDeclarationBlock();
+
+  mContentDeclarationBlock = parser.TakeDeclarationBlock();
 }
 
 const DeclarationBlock*
 nsSVGElement::GetContentDeclarationBlock() const
 {
   return mContentDeclarationBlock;
 }