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 831078 675288974991830a083a7d8d4f761fa8ab4db493
parent 831077 03b5cb64d690ad396470e00487e1ef8d5efa9f37
child 831079 f7d550d783449a155a5681f0db3939bd9ecad58b
push id118868
push userbmo:zjz@zjz.name
push dateFri, 24 Aug 2018 07:04:39 +0000
reviewersdholbert
bugs1485465, 1425700
milestone63.0a1
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;
 }