Refactor adding and removing named and IDed elements

Source/core/dom/Element.cpp

Index: Source/core/dom/Element.cpp
diff --git a/Source/core/dom/Element.cpp b/Source/core/dom/Element.cpp
index eac6701bb21300997e9e7664766516463f8ec1e5..2837350b9d8728553aa8a73241bd59155362403c 100644
--- a/Source/core/dom/Element.cpp
+++ b/Source/core/dom/Element.cpp
@@ -1216,9 +1216,11 @@ bool Element::isInert() const
 
 Node::InsertionNotificationRequest Element::insertedInto(ContainerNode* insertionPoint)
 {
+    bool wasInDocument = inDocument();
     // need to do superclass processing first so inDocument() is true
     // by the time we reach updateId
     ContainerNode::insertedInto(insertionPoint);
+    ASSERT(!wasInDocument || inDocument());

[adamk, 2013/08/09 00:59:11]

How useful is this ASSERT?

 
     if (containsFullScreenElement() && parentElement() && !parentElement()->containsFullScreenElement())
         setContainsFullScreenElementOnAncestorsCrossingFrameBoundaries(true);
@@ -1238,21 +1240,29 @@ Node::InsertionNotificationRequest Element::insertedInto(ContainerNode* insertio
     if (isUpgradedCustomElement() && inDocument())
         CustomElement::didEnterDocument(this, document());
 
-    TreeScope* scope = insertionPoint->treeScope();
-    if (scope != treeScope())
+    TreeScope* newScope = insertionPoint->treeScope();
+    if (newScope != treeScope())
         return InsertionDone;
 
+    HTMLDocument* newDocument = !wasInDocument && inDocument() && !isInShadowTree() && newScope->documentScope()->isHTMLDocument() ? toHTMLDocument(newScope->documentScope()) : 0;

[adamk, 2013/08/09 00:59:11]

Gah, this looks weird to me. Isn't newScope->documentScope() just document(),
since we've now guaranteed that newScope == treeScope()?

Also, if we're inDocument() and !isInShadowTree(), the !wasInDocument check is
redundant.

+
     const AtomicString& idValue = getIdAttribute();
-    if (!idValue.isNull())
-        updateId(scope, nullAtom, idValue);
+    if (!idValue.isNull()) {
+        updateIdForTreeScope(newScope, nullAtom, idValue);
+        if (newDocument)
+            updateIdForDocument(newDocument, nullAtom, idValue);
+    }
 
     const AtomicString& nameValue = getNameAttribute();
-    if (!nameValue.isNull())
-        updateName(nullAtom, nameValue);
+    if (!nameValue.isNull()) {
+        updateNameForTreeScope(newScope, nullAtom, nameValue);
+        if (newDocument)
+            updateNameForDocument(newDocument, nullAtom, nameValue);
+    }
 
     if (hasTagName(labelTag)) {
-        if (scope->shouldCacheLabelsByForAttribute())
-            updateLabel(scope, nullAtom, fastGetAttribute(forAttr));
+        if (newScope->shouldCacheLabelsByForAttribute())
+            updateLabel(newScope, nullAtom, fastGetAttribute(forAttr));
     }
 
     if (parentElement() && parentElement()->isInCanvasSubtree())
@@ -1284,22 +1294,31 @@ void Element::removedFrom(ContainerNode* insertionPoint)
     setSavedLayerScrollOffset(IntSize());
 
     if (insertionPoint->isInTreeScope() && treeScope() == document()) {
+        TreeScope* oldScope = insertionPoint->treeScope();
+        HTMLDocument* oldDocument = inDocument() && !isInShadowTree() && oldScope->documentScope()->isHTMLDocument() ? toHTMLDocument(oldScope->documentScope()) : 0;

[adamk, 2013/08/09 00:59:11]

Eww, this is one of those places where inDocument() means "wasInDocument"...got
me confused. We have a wasInDocument bool above, can you please use it here? And
maybe add a bool wasInShadowTree up there?

+
         const AtomicString& idValue = getIdAttribute();
-        if (!idValue.isNull())
-            updateId(insertionPoint->treeScope(), idValue, nullAtom);
+        if (!idValue.isNull()) {
+            updateIdForTreeScope(oldScope, idValue, nullAtom);
+            if (oldDocument)
+                updateIdForDocument(oldDocument, idValue, nullAtom);
+        }
 
         const AtomicString& nameValue = getNameAttribute();
-        if (!nameValue.isNull())
-            updateName(nameValue, nullAtom);
+        if (!nameValue.isNull()) {
+            updateNameForTreeScope(oldScope, nameValue, nullAtom);
+            if (oldDocument)
+                updateNameForDocument(oldDocument, nameValue, nullAtom);
+        }
 
         if (hasTagName(labelTag)) {
-            TreeScope* treeScope = insertionPoint->treeScope();
-            if (treeScope->shouldCacheLabelsByForAttribute())
-                updateLabel(treeScope, fastGetAttribute(forAttr), nullAtom);
+            if (oldScope->shouldCacheLabelsByForAttribute())
+                updateLabel(oldScope, fastGetAttribute(forAttr), nullAtom);
         }
     }
 
     ContainerNode::removedFrom(insertionPoint);
+
     if (wasInDocument) {
         if (hasPendingResources())
             document()->accessSVGExtensions()->removeElementFromPendingResources(this);
@@ -2760,19 +2779,47 @@ bool Element::hasNamedNodeMap() const
 }
 #endif
 
-inline void Element::updateName(const AtomicString& oldName, const AtomicString& newName)
+void Element::updateName(const AtomicString& oldName, const AtomicString& newName)
 {
-    if (!inDocument() || isInShadowTree())
+    if (!isInTreeScope())
         return;
 
     if (oldName == newName)
         return;
 
+    updateNameForTreeScope(treeScope(), oldName, newName);
+
+    if (!inDocument() || isInShadowTree())
+        return;
+
+    Document* htmlDocument = document();
+    if (!htmlDocument->isHTMLDocument())
+        return;
+
+    updateNameForDocument(toHTMLDocument(htmlDocument), oldName, newName);
+}
+
+void Element::updateNameForTreeScope(TreeScope* scope, const AtomicString& oldName, const AtomicString& newName)
+{
+    ASSERT(isInTreeScope());
+    ASSERT(oldName != newName);
+
+    if (!oldName.isEmpty())
+        scope->removeElementByName(oldName, this);
+    if (!newName.isEmpty())
+        scope->addElementByName(newName, this);
+}
+
+void Element::updateNameForDocument(HTMLDocument* document, const AtomicString& oldName, const AtomicString& newName)
+{
+    ASSERT(inDocument() && !isInShadowTree());
+    ASSERT(oldName != newName);
+
     if (shouldRegisterAsNamedItem())
         updateNamedItemRegistration(oldName, newName);
 }
 
-inline void Element::updateId(const AtomicString& oldId, const AtomicString& newId)
+void Element::updateId(const AtomicString& oldId, const AtomicString& newId)
 {
     if (!isInTreeScope())
         return;
@@ -2780,10 +2827,19 @@ inline void Element::updateId(const AtomicString& oldId, const AtomicString& new
     if (oldId == newId)
         return;
 
-    updateId(treeScope(), oldId, newId);
+    updateIdForTreeScope(treeScope(), oldId, newId);
+
+    if (!inDocument() || isInShadowTree())
+        return;
+
+    Document* htmlDocument = document();
+    if (!htmlDocument->isHTMLDocument())
+        return;
+
+    updateIdForDocument(toHTMLDocument(htmlDocument), oldId, newId);
 }
 
-inline void Element::updateId(TreeScope* scope, const AtomicString& oldId, const AtomicString& newId)
+void Element::updateIdForTreeScope(TreeScope* scope, const AtomicString& oldId, const AtomicString& newId)
 {
     ASSERT(isInTreeScope());
     ASSERT(oldId != newId);
@@ -2792,6 +2848,12 @@ inline void Element::updateId(TreeScope* scope, const AtomicString& oldId, const
         scope->removeElementById(oldId, this);
     if (!newId.isEmpty())
         scope->addElementById(newId, this);
+}
+
+void Element::updateIdForDocument(HTMLDocument* document, const AtomicString& oldId, const AtomicString& newId)
+{
+    ASSERT(inDocument() && !isInShadowTree());
+    ASSERT(oldId != newId);
 
     if (shouldRegisterAsExtraNamedItem())
         updateExtraNamedItemRegistration(oldId, newId);
@@ -2882,8 +2944,7 @@ void Element::didMoveToNewDocument(Document* oldDocument)
 
 void Element::updateNamedItemRegistration(const AtomicString& oldName, const AtomicString& newName)
 {
-    if (!document()->isHTMLDocument())
-        return;
+    ASSERT(document()->isHTMLDocument());
 
     if (!oldName.isEmpty())
         toHTMLDocument(document())->removeNamedItem(oldName);
@@ -2894,8 +2955,7 @@ void Element::updateNamedItemRegistration(const AtomicString& oldName, const Ato
 
 void Element::updateExtraNamedItemRegistration(const AtomicString& oldId, const AtomicString& newId)
 {
-    if (!document()->isHTMLDocument())
-        return;
+    ASSERT(document()->isHTMLDocument());
 
     if (!oldId.isEmpty())
         toHTMLDocument(document())->removeExtraNamedItem(oldId);
@@ -3033,6 +3093,10 @@ void Element::cloneAttributesFromElement(const Element& other)
         return;
     }
 
+    // We can't update window and document's named item maps since the presence of image and object elements depend on other attributes and children.
+    // Fortunately, those named item maps are only updated when this element is in the document, which should never be the case.
+    ASSERT(!inDocument());
+
     const AtomicString& oldID = getIdAttribute();
     const AtomicString& newID = other.getIdAttribute();