Bug 1443948 - Remove NODE_FORCE_XBL_BINDINGS;r=bz
authorBrian Grinstead <bgrinstead@mozilla.com>
Mon, 12 Mar 2018 13:24:10 -0700
changeset 464091 dbf853820066253e476d7fbcee35da534f9cc538
parent 464090 a7d42da37878ba41504181f7474203c6dda3694c
child 464092 29aeb9151593f4202733b0836687b5b7570eaa4c
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1443948
milestone61.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 1443948 - Remove NODE_FORCE_XBL_BINDINGS;r=bz This was used to attach a binding to a cloned node before it got inserted into the doc. This is no longer used in the browser chrome, so this patch removes the feature to prevent future usage and simplify dom code. MozReview-Commit-ID: KnkHWJ8oQig
dom/base/Element.cpp
dom/base/nsGenericDOMDataNode.cpp
dom/base/nsINode.h
dom/base/nsNodeUtils.cpp
dom/xbl/test/test_bug398492.xul
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -560,18 +560,17 @@ Element::GetBindingURL(nsIDocument *aDoc
 JSObject*
 Element::WrapObject(JSContext *aCx, JS::Handle<JSObject*> aGivenProto)
 {
   JS::Rooted<JSObject*> obj(aCx, nsINode::WrapObject(aCx, aGivenProto));
   if (!obj) {
     return nullptr;
   }
 
-  nsIDocument* doc =
-    HasFlag(NODE_FORCE_XBL_BINDINGS) ? OwnerDoc() : GetComposedDoc();
+  nsIDocument* doc = GetComposedDoc();
   if (!doc) {
     // There's no baseclass that cares about this call so we just
     // return here.
     return obj;
   }
 
   // We must ensure that the XBL Binding is installed before we hand
   // back this object.
@@ -1566,19 +1565,16 @@ Element::BindToTree(nsIDocument* aDocume
   // only assert if our parent is _changing_ while we have a parent.
   NS_PRECONDITION(!GetParent() || aParent == GetParent(),
                   "Already have a parent.  Unbind first!");
   NS_PRECONDITION(!GetBindingParent() ||
                   aBindingParent == GetBindingParent() ||
                   (!aBindingParent && aParent &&
                    aParent->GetBindingParent() == GetBindingParent()),
                   "Already have a binding parent.  Unbind first!");
-  NS_PRECONDITION(!aParent || !aDocument ||
-                  !aParent->HasFlag(NODE_FORCE_XBL_BINDINGS),
-                  "Parent in document but flagged as forcing XBL");
   NS_PRECONDITION(aBindingParent != this,
                   "Content must not be its own binding parent");
   NS_PRECONDITION(!IsRootOfNativeAnonymousSubtree() ||
                   aBindingParent == aParent,
                   "Native anonymous content must have its parent as its "
                   "own binding parent");
   NS_PRECONDITION(aBindingParent || !aParent ||
                   aBindingParent == aParent->GetBindingParent(),
@@ -1619,30 +1615,24 @@ Element::BindToTree(nsIDocument* aDocume
       SetFlags(NODE_IS_IN_SHADOW_TREE);
     }
     ShadowRoot* parentContainingShadow = aParent->GetContainingShadow();
     if (parentContainingShadow) {
       ExtendedDOMSlots()->mContainingShadow = parentContainingShadow;
     }
   }
 
-  bool hadForceXBL = HasFlag(NODE_FORCE_XBL_BINDINGS);
-
   bool hadParent = !!GetParentNode();
 
-  // Now set the parent and set the "Force attach xbl" flag if needed.
+  // Now set the parent.
   if (aParent) {
     if (!GetParent()) {
       NS_ADDREF(aParent);
     }
     mParent = aParent;
-
-    if (aParent->HasFlag(NODE_FORCE_XBL_BINDINGS)) {
-      SetFlags(NODE_FORCE_XBL_BINDINGS);
-    }
   }
   else {
     mParent = aDocument;
   }
   SetParentIsContent(aParent);
 
   // XXXbz sXBL/XBL2 issue!
 
@@ -1659,38 +1649,36 @@ Element::BindToTree(nsIDocument* aDocume
 
     // We no longer need to track the subtree pointer (and in fact we'll assert
     // if we do this any later).
     ClearSubtreeRootPointer();
 
     // Being added to a document.
     SetIsInDocument();
 
-    // Unset this flag since we now really are in a document.
-    UnsetFlags(NODE_FORCE_XBL_BINDINGS |
-               // And clear the lazy frame construction bits.
-               NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES |
+    // Clear the lazy frame construction bits.
+    UnsetFlags(NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES |
                // And the restyle bits. These shouldn't even get set if we came
                // from a Servo-styled document, but they may be set if the
                // element comes from a Gecko-backed document, see bug 1394586.
                //
                // TODO(emilio): We can remove this and assert we don't have any
                // of them when we remove the old style system.
                ELEMENT_ALL_RESTYLE_FLAGS);
   } else if (IsInShadowTree()) {
     // We're not in a document, but we did get inserted into a shadow tree.
     // Since we won't have any restyle data in the document's restyle trackers,
     // don't let us get inserted with restyle bits set incorrectly.
     //
     // Also clear all the other flags that are cleared above when we do get
     // inserted into a document.
     //
     // See the comment about the restyle bits above, it also applies.
-    UnsetFlags(NODE_FORCE_XBL_BINDINGS | NODE_NEEDS_FRAME |
-               NODE_DESCENDANTS_NEED_FRAMES | ELEMENT_ALL_RESTYLE_FLAGS);
+    UnsetFlags(NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES |
+               ELEMENT_ALL_RESTYLE_FLAGS);
   } else {
     // If we're not in the doc and not in a shadow tree,
     // update our subtree pointer.
     SetSubtreeRootPointer(aParent->SubtreeRoot());
   }
 
   if (CustomElementRegistry::IsCustomElementEnabled() && IsInComposedDoc()) {
     // Connected callback must be enqueued whenever a custom element becomes
@@ -1720,21 +1708,20 @@ Element::BindToTree(nsIDocument* aDocume
   if (IsHTMLElement()) {
     SetDirOnBind(this, aParent);
   }
 
   uint32_t editableDescendantCount = 0;
 
   UpdateEditableState(false);
 
-  // If NODE_FORCE_XBL_BINDINGS was set, or we had a pre-existing XBL binding,
+  // If we had a pre-existing XBL binding,
   // we might have anonymous children that also need to be told that they are
   // moving.
-  if (hadForceXBL ||
-      (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) && !GetShadowRoot())) {
+  if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) && !GetShadowRoot()) {
     nsXBLBinding* binding =
       OwnerDoc()->BindingManager()->GetBindingWithContent(this);
 
     if (binding) {
       binding->BindAnonymousContent(
         binding->GetAnonymousContent(),
         this,
         binding->PrototypeBinding()->ChromeOnlyContent());
@@ -1885,18 +1872,17 @@ Element::UnbindFromTree(bool aDeep, bool
 {
   NS_PRECONDITION(aDeep || (!GetUncomposedDoc() && !GetBindingParent()),
                   "Shallow unbind won't clear document and binding parent on "
                   "kids!");
 
   RemoveFromIdTable();
 
   // Make sure to unbind this node before doing the kids
-  nsIDocument* document =
-    HasFlag(NODE_FORCE_XBL_BINDINGS) ? OwnerDoc() : GetComposedDoc();
+  nsIDocument* document = GetComposedDoc();
 
   if (HasPointerLock()) {
     nsIDocument::UnlockPointer();
   }
   if (mState.HasState(NS_EVENT_STATE_FULL_SCREEN)) {
     // The element being removed is an ancestor of the full-screen element,
     // exit full-screen state.
     nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
@@ -1990,18 +1976,16 @@ Element::UnbindFromTree(bool aDeep, bool
 
   if (aNullParent || !mParent->IsInShadowTree()) {
     UnsetFlags(NODE_IS_IN_SHADOW_TREE);
 
     // Begin keeping track of our subtree root.
     SetSubtreeRootPointer(aNullParent ? this : mParent->SubtreeRoot());
   }
 
-  // Unset this since that's what the old code effectively did.
-  UnsetFlags(NODE_FORCE_XBL_BINDINGS);
   bool clearBindingParent = true;
 
 #ifdef MOZ_XUL
   if (nsXULElement* xulElem = nsXULElement::FromContent(this)) {;
     xulElem->SetXULBindingParent(nullptr);
     clearBindingParent = false;
   }
 #endif
@@ -2698,17 +2682,17 @@ Element::SetAttrAndNotify(int32_t aNames
       oldValue = nullptr;
     }
   } else {
     // No need to conditionally assign null here. If there was no previously
     // set value for the attribute, aOldValue will already be null.
     oldValue = aOldValue;
   }
 
-  if (aComposedDocument || HasFlag(NODE_FORCE_XBL_BINDINGS)) {
+  if (aComposedDocument) {
     RefPtr<nsXBLBinding> binding = GetXBLBinding();
     if (binding) {
       binding->AttributeChanged(aName, aNamespaceID, false, aNotify);
     }
   }
 
   if (CustomElementRegistry::IsCustomElementEnabled()) {
     CustomElementDefinition* definition = GetCustomElementDefinition();
@@ -3005,17 +2989,17 @@ Element::UnsetAttr(int32_t aNameSpaceID,
   }
 
   nsAttrValue oldValue;
   rv = mAttrsAndChildren.RemoveAttrAt(index, oldValue);
   NS_ENSURE_SUCCESS(rv, rv);
 
   PostIdMaybeChange(aNameSpaceID, aName, nullptr);
 
-  if (document || HasFlag(NODE_FORCE_XBL_BINDINGS)) {
+  if (document) {
     RefPtr<nsXBLBinding> binding = GetXBLBinding();
     if (binding) {
       binding->AttributeChanged(aName, aNameSpaceID, true, aNotify);
     }
   }
 
   if (CustomElementRegistry::IsCustomElementEnabled()) {
     CustomElementDefinition* definition = GetCustomElementDefinition();
--- a/dom/base/nsGenericDOMDataNode.cpp
+++ b/dom/base/nsGenericDOMDataNode.cpp
@@ -575,18 +575,17 @@ nsGenericDOMDataNode::BindToTree(nsIDocu
 
 void
 nsGenericDOMDataNode::UnbindFromTree(bool aDeep, bool aNullParent)
 {
   // Unset frame flags; if we need them again later, they'll get set again.
   UnsetFlags(NS_CREATE_FRAME_IF_NON_WHITESPACE |
              NS_REFRAME_IF_WHITESPACE);
 
-  nsIDocument* document =
-    HasFlag(NODE_FORCE_XBL_BINDINGS) ? OwnerDoc() : GetComposedDoc();
+  nsIDocument* document = GetComposedDoc();
 
   if (aNullParent) {
     if (this->IsRootOfNativeAnonymousSubtree()) {
       nsNodeUtils::NativeAnonymousChildListChange(this, true);
     }
     if (GetParent()) {
       NS_RELEASE(mParent);
     } else {
--- a/dom/base/nsINode.h
+++ b/dom/base/nsINode.h
@@ -122,24 +122,20 @@ enum {
   NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE =          NODE_FLAG_BIT(3),
 
   // Whether this node is the root of a native anonymous (from the perspective
   // of its parent) subtree.  This flag is set-once: once a node has it, it
   // must not be removed.
   // NOTE: Should only be used on nsIContent nodes
   NODE_IS_NATIVE_ANONYMOUS_ROOT =         NODE_FLAG_BIT(4),
 
-  // Forces the XBL code to treat this node as if it were
-  // in the document and therefore should get bindings attached.
-  NODE_FORCE_XBL_BINDINGS =               NODE_FLAG_BIT(5),
+  // Whether a binding manager may have a pointer to this
+  NODE_MAY_BE_IN_BINDING_MNGR =           NODE_FLAG_BIT(5),
 
-  // Whether a binding manager may have a pointer to this
-  NODE_MAY_BE_IN_BINDING_MNGR =           NODE_FLAG_BIT(6),
-
-  NODE_IS_EDITABLE =                      NODE_FLAG_BIT(7),
+  NODE_IS_EDITABLE =                      NODE_FLAG_BIT(6),
 
   // This node was created by layout as native anonymous content. This
   // generally corresponds to things created by nsIAnonymousContentCreator,
   // though there are exceptions (svg:use content does not have this flag
   // set, and any non-nsIAnonymousContentCreator callers of
   // SetIsNativeAnonymousRoot also get this flag).
   //
   // One very important aspect here is that this node is not transitive over
@@ -152,73 +148,73 @@ enum {
   // For now, this bit exists primarily to control style inheritance behavior,
   // since the nodes for which we set it are often used to implement pseudo-
   // elements, which need to inherit style from a script-visible element.
   //
   // A more general principle for this bit might be this: If the node is entirely
   // a detail of layout, is not script-observable in any way, and other engines
   // might accomplish the same task with a nodeless layout frame, then the node
   // should have this bit set.
-  NODE_IS_NATIVE_ANONYMOUS =              NODE_FLAG_BIT(8),
+  NODE_IS_NATIVE_ANONYMOUS =              NODE_FLAG_BIT(7),
 
   // Whether the node participates in a shadow tree.
-  NODE_IS_IN_SHADOW_TREE =                NODE_FLAG_BIT(9),
+  NODE_IS_IN_SHADOW_TREE =                NODE_FLAG_BIT(8),
 
   // Node has an :empty or :-moz-only-whitespace selector
-  NODE_HAS_EMPTY_SELECTOR =               NODE_FLAG_BIT(10),
+  NODE_HAS_EMPTY_SELECTOR =               NODE_FLAG_BIT(9),
 
   // A child of the node has a selector such that any insertion,
   // removal, or appending of children requires restyling the parent.
-  NODE_HAS_SLOW_SELECTOR =                NODE_FLAG_BIT(11),
+  NODE_HAS_SLOW_SELECTOR =                NODE_FLAG_BIT(10),
 
   // A child of the node has a :first-child, :-moz-first-node,
   // :only-child, :last-child or :-moz-last-node selector.
-  NODE_HAS_EDGE_CHILD_SELECTOR =          NODE_FLAG_BIT(12),
+  NODE_HAS_EDGE_CHILD_SELECTOR =          NODE_FLAG_BIT(11),
 
   // A child of the node has a selector such that any insertion or
   // removal of children requires restyling later siblings of that
   // element.  Additionally (in this manner it is stronger than
   // NODE_HAS_SLOW_SELECTOR), if a child's style changes due to any
   // other content tree changes (e.g., the child changes to or from
   // matching :empty due to a grandchild insertion or removal), the
   // child's later siblings must also be restyled.
-  NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS = NODE_FLAG_BIT(13),
+  NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS = NODE_FLAG_BIT(12),
 
   NODE_ALL_SELECTOR_FLAGS =               NODE_HAS_EMPTY_SELECTOR |
                                           NODE_HAS_SLOW_SELECTOR |
                                           NODE_HAS_EDGE_CHILD_SELECTOR |
                                           NODE_HAS_SLOW_SELECTOR_LATER_SIBLINGS,
 
   // This node needs to go through frame construction to get a frame (or
   // undisplayed entry).
-  NODE_NEEDS_FRAME =                      NODE_FLAG_BIT(14),
+  NODE_NEEDS_FRAME =                      NODE_FLAG_BIT(13),
 
   // At least one descendant in the flattened tree has NODE_NEEDS_FRAME set.
   // This should be set on every node on the flattened tree path between the
   // node(s) with NODE_NEEDS_FRAME and the root content.
-  NODE_DESCENDANTS_NEED_FRAMES =          NODE_FLAG_BIT(15),
+  NODE_DESCENDANTS_NEED_FRAMES =          NODE_FLAG_BIT(14),
 
   // Set if the node has the accesskey attribute set.
-  NODE_HAS_ACCESSKEY =                    NODE_FLAG_BIT(16),
+  NODE_HAS_ACCESSKEY =                    NODE_FLAG_BIT(15),
 
   // Set if the node has right-to-left directionality
-  NODE_HAS_DIRECTION_RTL =                NODE_FLAG_BIT(17),
+  NODE_HAS_DIRECTION_RTL =                NODE_FLAG_BIT(16),
 
   // Set if the node has left-to-right directionality
-  NODE_HAS_DIRECTION_LTR =                NODE_FLAG_BIT(18),
+  NODE_HAS_DIRECTION_LTR =                NODE_FLAG_BIT(17),
 
   NODE_ALL_DIRECTION_FLAGS =              NODE_HAS_DIRECTION_LTR |
                                           NODE_HAS_DIRECTION_RTL,
 
-  NODE_CHROME_ONLY_ACCESS =               NODE_FLAG_BIT(19),
+  NODE_CHROME_ONLY_ACCESS =               NODE_FLAG_BIT(18),
 
-  NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS =    NODE_FLAG_BIT(20),
+  NODE_IS_ROOT_OF_CHROME_ONLY_ACCESS =    NODE_FLAG_BIT(19),
 
   // Remaining bits are node type specific.
-  NODE_TYPE_SPECIFIC_BITS_OFFSET =        21
+  NODE_TYPE_SPECIFIC_BITS_OFFSET =        20
 };
 
 // Make sure we have space for our bits
 #define ASSERT_NODE_FLAGS_SPACE(n) \
   static_assert(WRAPPER_CACHE_FLAGS_BITS_USED + (n) <=                          \
                   sizeof(nsWrapperCache::FlagsType) * 8,                        \
                 "Not enough space for our bits")
 ASSERT_NODE_FLAGS_SPACE(NODE_TYPE_SPECIFIC_BITS_OFFSET);
--- a/dom/base/nsNodeUtils.cpp
+++ b/dom/base/nsNodeUtils.cpp
@@ -356,27 +356,18 @@ nsNodeUtils::LastRelease(nsINode* aNode)
     aNode->UnsetFlags(NODE_HAS_LISTENERMANAGER);
   }
 
   if (aNode->IsElement()) {
     nsIDocument* ownerDoc = aNode->OwnerDoc();
     Element* elem = aNode->AsElement();
     ownerDoc->ClearBoxObjectFor(elem);
 
-    NS_ASSERTION(aNode->HasFlag(NODE_FORCE_XBL_BINDINGS) ||
-                 !elem->GetXBLBinding(),
-                 "Non-forced node has binding on destruction");
-
-    // if NODE_FORCE_XBL_BINDINGS is set, the node might still have a binding
-    // attached
-    if (aNode->HasFlag(NODE_FORCE_XBL_BINDINGS) &&
-        ownerDoc->BindingManager()) {
-      ownerDoc->BindingManager()->RemovedFromDocument(elem, ownerDoc,
-                                                      nsBindingManager::eRunDtor);
-    }
+    NS_ASSERTION(!elem->GetXBLBinding(),
+                 "Node has binding on destruction");
   }
 
   aNode->ReleaseWrapper(aNode);
 
   FragmentOrElement::RemoveBlackMarkedNode(aNode);
 }
 
 static void
@@ -717,35 +708,16 @@ nsNodeUtils::CloneAndAdopt(nsINode *aNod
                       aReparentScope, aNodesWithProperties, cloneContent,
                       aError);
       if (NS_WARN_IF(aError.Failed())) {
         return nullptr;
       }
     }
   }
 
-  // XXX setting document on some nodes not in a document so XBL will bind
-  // and chrome won't break. Make XBL bind to document-less nodes!
-  // XXXbz Once this is fixed, fix up the asserts in all implementations of
-  // BindToTree to assert what they would like to assert, and fix the
-  // ChangeDocumentFor() call in nsXULElement::BindToTree as well.  Also,
-  // remove the UnbindFromTree call in ~nsXULElement, and add back in the
-  // precondition in nsXULElement::UnbindFromTree and remove the line in
-  // nsXULElement.h that makes nsNodeUtils a friend of nsXULElement.
-  // Note: Make sure to do this witchery _after_ we've done any deep
-  // cloning, so kids of the new node aren't confused about whether they're
-  // in a document.
-#ifdef MOZ_XUL
-  if (aClone && !aParent && aNode->IsXULElement()) {
-    if (!aNode->OwnerDoc()->IsLoadedAsInteractiveData()) {
-      clone->SetFlags(NODE_FORCE_XBL_BINDINGS);
-    }
-  }
-#endif
-
   return clone.forget();
 }
 
 
 /* static */
 void
 nsNodeUtils::UnlinkUserData(nsINode *aNode)
 {
--- a/dom/xbl/test/test_bug398492.xul
+++ b/dom/xbl/test/test_bug398492.xul
@@ -42,21 +42,18 @@ https://bugzilla.mozilla.org/show_bug.cg
       var n = $("testbox");
       var kid = n.firstChild;
       var anonKid = document.getAnonymousNodes(n)[0];
       is(anonKid instanceof XULElement, true, "Must be a XUL element");
       is(anonKid, getXBLParent(kid), "Unexpected anonymous nodes");
 
       var n2 = n.cloneNode(true);
       var kid2 = n2.firstChild;
-      var anonKid2 = document.getAnonymousNodes(n2)[0];
-      is(anonKid2 instanceof XULElement, true,
-         "Must be a XUL element after clone");
-      is(anonKid2, getXBLParent(kid2),
-         "Unexpected anonymous nodes after clone");
+      var anonKids = document.getAnonymousNodes(n2);
+      is(anonKids, null, "No XBL binding attached to cloned node");
 
       var n3 = document.createElement("hbox");
       document.documentElement.appendChild(n3);
       var kid3 = document.createTextNode("Text");
       n3.appendChild(kid3);
 
       // Note - we rely on the fact that the binding is preloaded
       // by the other hbox here, so that the binding will be applied
@@ -69,20 +66,16 @@ https://bugzilla.mozilla.org/show_bug.cg
          "Must be a XUL element after addBinding");
       is(anonKid3, getXBLParent(kid3),
          "Unexpected anonymous nodes after addBinding");
       
 
       n.removeChild(kid);
       isnot(anonKid, getXBLParent(kid),
             "Should have removed kid from insertion point");
-
-      n2.removeChild(kid2);
-      isnot(anonKid2, getXBLParent(kid2),
-            "Should have removed kid2 from insertion point");
     
       n3.removeChild(kid3);
       isnot(anonKid3, getXBLParent(kid3),
             "Should have removed kid3 from insertion point");
     
       SimpleTest.finish();
     });