Bug 1440682: Make the XUL tooltip stuff saner. r=enn

We never removed the event listeners (the code was there, lol, but the function
that was supposed to call into the tooltip listener returned
NS_ERROR_NOT_IMPLEMENTED instead).

Furthermore, we added an event listener each time we reframed an element, which
is insane. Basically, each time an element with tooltip / tooltiptext gets its
frame tree reconstructed, we add the even listener, again, and we never free it.

Xidorn pointed out that this is not such a huge deal because we deduplicate
event listeners per spec, but still...

Move the code from the RestyleManager and the frame constructor to AfterSetAttr
/ BindToTree / UnbindFromTree in nsXULElement to hopefully make this saner.

MozReview-Commit-ID: 6BQbIQJ87qt

dom/xul/nsXULElement.cpp

@@ -81,6 +81,7 @@
 #include "nsIFrame.h"
 #include "nsNodeInfoManager.h"
 #include "nsXBLBinding.h"
+#include "nsXULTooltipListener.h"
 #include "mozilla/EventDispatcher.h"
 #include "mozAutoDocUpdate.h"
 #include "nsIDOMXULCommandEvent.h"
@@ -734,6 +735,19 @@ private:
   nsCOMPtr<nsIDocument> mDocument;
 };
 
+static bool
+NeedTooltipSupport(const nsXULElement& aXULElement)
+{
+  if (aXULElement.NodeInfo()->Equals(nsGkAtoms::treechildren)) {
+    // treechildren always get tooltip support, since cropped tree cells show
+    // their full text in a tooltip.
+    return true;
+  }
+
+  return aXULElement.GetBoolAttr(nsGkAtoms::tooltip) ||
+         aXULElement.GetBoolAttr(nsGkAtoms::tooltiptext);
+}
+
 nsresult
 nsXULElement::BindToTree(nsIDocument* aDocument,
                          nsIContent* aParent,
@@ -786,6 +800,10 @@ nsXULElement::BindToTree(nsIDocument* aDocument,
     }
   }
 
+  if (doc && NeedTooltipSupport(*this)) {
+      AddTooltipSupport();
+  }
+
   if (aDocument) {
       NS_ASSERTION(!nsContentUtils::IsSafeToRunScript(),
                    "Missing a script blocker!");
@@ -799,6 +817,10 @@ nsXULElement::BindToTree(nsIDocument* aDocument,
 void
 nsXULElement::UnbindFromTree(bool aDeep, bool aNullParent)
 {
+    if (NeedTooltipSupport(*this)) {
+        RemoveTooltipSupport();
+    }
+
     // mControllers can own objects that are implemented
     // in JavaScript (such as some implementations of
     // nsIControllers.  These objects prevent their global
@@ -1212,6 +1234,17 @@ nsXULElement::AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
             }
         }
 
+        if (aName == nsGkAtoms::tooltip || aName == nsGkAtoms::tooltiptext) {
+            if (!!aValue != !!aOldValue &&
+                IsInComposedDoc() &&
+                !NodeInfo()->Equals(nsGkAtoms::treechildren)) {
+                if (aValue) {
+                    AddTooltipSupport();
+                } else {
+                    RemoveTooltipSupport();
+                }
+            }
+        }
         // XXX need to check if they're changing an event handler: if
         // so, then we need to unhook the old one.  Or something.
     }
@@ -1220,6 +1253,28 @@ nsXULElement::AfterSetAttr(int32_t aNamespaceID, nsAtom* aName,
                                          aValue, aOldValue, aSubjectPrincipal, aNotify);
 }
 
+void
+nsXULElement::AddTooltipSupport()
+{
+  nsXULTooltipListener* listener = nsXULTooltipListener::GetInstance();
+  if (!listener) {
+    return;
+  }
+
+  listener->AddTooltipSupport(this);
+}
+
+void
+nsXULElement::RemoveTooltipSupport()
+{
+  nsXULTooltipListener* listener = nsXULTooltipListener::GetInstance();
+  if (!listener) {
+    return;
+  }
+
+  listener->RemoveTooltipSupport(this);
+}
+
 bool
 nsXULElement::ParseAttribute(int32_t aNamespaceID,
                              nsAtom* aAttribute,

dom/xul/nsXULElement.h

@@ -754,6 +754,9 @@ protected:
     void RemoveBroadcaster(const nsAString & broadcasterId);
 
 protected:
+    void AddTooltipSupport();
+    void RemoveTooltipSupport();
+
     // Internal accessor. This shadows the 'Slots', and returns
     // appropriate value.
     nsIControllers *Controllers() {

layout/base/GeckoRestyleManager.cpp

@@ -394,19 +394,6 @@ GeckoRestyleManager::AttributeChanged(Element* aElement,
          tag == nsGkAtoms::listcell))
       return;
   }
-
-  if (aAttribute == nsGkAtoms::tooltiptext ||
-      aAttribute == nsGkAtoms::tooltip)
-  {
-    nsIRootBox* rootBox = nsIRootBox::GetRootBox(PresContext()->GetPresShell());
-    if (rootBox) {
-      if (aModType == MutationEventBinding::REMOVAL)
-        rootBox->RemoveTooltipSupport(aElement);
-      if (aModType == MutationEventBinding::ADDITION)
-        rootBox->AddTooltipSupport(aElement);
-    }
-  }
-
 #endif // MOZ_XUL
 
   if (primaryFrame) {

layout/base/nsCSSFrameConstructor.cpp

@@ -4275,19 +4275,6 @@ nsCSSFrameConstructor::ConstructFrameFromItemInternal(FrameConstructionItem& aIt
     }
   }
 
-#ifdef MOZ_XUL
-  // More icky XUL stuff
-  if (aItem.mNameSpaceID == kNameSpaceID_XUL &&
-      (aItem.mTag == nsGkAtoms::treechildren || // trees always need titletips
-       content->AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::tooltiptext) ||
-       content->AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::tooltip))) {
-    nsIRootBox* rootBox = nsIRootBox::GetRootBox(mPresShell);
-    if (rootBox) {
-      rootBox->AddTooltipSupport(content);
-    }
-  }
-#endif
-
   NS_ASSERTION(newFrame->IsFrameOfType(nsIFrame::eLineParticipant) ==
                ((bits & FCDATA_IS_LINE_PARTICIPANT) != 0),
                "Incorrectly set FCDATA_IS_LINE_PARTICIPANT bits");

layout/xul/nsIRootBox.h

@@ -30,9 +30,6 @@ public:
   virtual mozilla::dom::Element* GetDefaultTooltip() = 0;
   virtual void SetDefaultTooltip(mozilla::dom::Element* aTooltip) = 0;
 
-  virtual nsresult AddTooltipSupport(nsIContent* aNode) = 0;
-  virtual nsresult RemoveTooltipSupport(nsIContent* aNode) = 0;
-
   static nsIRootBox* GetRootBox(nsIPresShell* aShell);
 };
 

layout/xul/nsRootBoxFrame.cpp

@@ -12,7 +12,6 @@
 #include "nsStackLayout.h"
 #include "nsIRootBox.h"
 #include "nsIContent.h"
-#include "nsXULTooltipListener.h"
 #include "nsFrameManager.h"
 #include "mozilla/BasicEvents.h"
 
@@ -57,8 +56,6 @@ public:
   virtual void SetPopupSetFrame(nsPopupSetFrame* aPopupSet) override;
   virtual Element* GetDefaultTooltip() override;
   virtual void SetDefaultTooltip(Element* aTooltip) override;
-  virtual nsresult AddTooltipSupport(nsIContent* aNode) override;
-  virtual nsresult RemoveTooltipSupport(nsIContent* aNode) override;
 
   virtual void AppendFrames(ChildListID     aListID,
                             nsFrameList&    aFrameList) override;
@@ -243,28 +240,6 @@ nsRootBoxFrame::SetDefaultTooltip(Element* aTooltip)
   mDefaultTooltip = aTooltip;
 }
 
-nsresult
-nsRootBoxFrame::AddTooltipSupport(nsIContent* aNode)
-{
-  NS_ENSURE_TRUE(aNode, NS_ERROR_NULL_POINTER);
-
-  nsXULTooltipListener *listener = nsXULTooltipListener::GetInstance();
-  if (!listener)
-    return NS_ERROR_OUT_OF_MEMORY;
-
-  return listener->AddTooltipSupport(aNode);
-}
-
-nsresult
-nsRootBoxFrame::RemoveTooltipSupport(nsIContent* aNode)
-{
-  // XXjh yuck, I'll have to implement a way to get at
-  // the tooltip listener for a given node to make
-  // this work.  Not crucial, we aren't removing
-  // tooltips from any nodes in the app just yet.
-  return NS_ERROR_NOT_IMPLEMENTED;
-}
-
 NS_QUERYFRAME_HEAD(nsRootBoxFrame)
   NS_QUERYFRAME_ENTRY(nsIRootBox)
 NS_QUERYFRAME_TAIL_INHERITING(nsBoxFrame)

layout/xul/nsXULTooltipListener.cpp

@@ -35,7 +35,7 @@
 using namespace mozilla;
 using namespace mozilla::dom;
 
-nsXULTooltipListener* nsXULTooltipListener::mInstance = nullptr;
+nsXULTooltipListener* nsXULTooltipListener::sInstance = nullptr;
 
 //////////////////////////////////////////////////////////////////////////
 //// nsISupports
@@ -50,28 +50,26 @@ nsXULTooltipListener::nsXULTooltipListener()
   , mLastTreeRow(-1)
 #endif
 {
-  if (sTooltipListenerCount++ == 0) {
-    // register the callback so we get notified of updates
-    Preferences::RegisterCallback(ToolbarTipsPrefChanged,
-                                  "browser.chrome.toolbar_tips");
+  // FIXME(emilio): This can be faster, this should use BoolVarCache.
+  //
+  // register the callback so we get notified of updates
+  Preferences::RegisterCallback(ToolbarTipsPrefChanged,
+                                "browser.chrome.toolbar_tips");
 
-    // Call the pref callback to initialize our state.
-    ToolbarTipsPrefChanged("browser.chrome.toolbar_tips", nullptr);
-  }
+  // Call the pref callback to initialize our state.
+  ToolbarTipsPrefChanged("browser.chrome.toolbar_tips", nullptr);
 }
 
 nsXULTooltipListener::~nsXULTooltipListener()
 {
-  if (nsXULTooltipListener::mInstance == this) {
-    ClearTooltipCache();
-  }
+  MOZ_ASSERT(sInstance == this);
+  sInstance = nullptr;
+
   HideTooltip();
 
-  if (--sTooltipListenerCount == 0) {
-    // Unregister our pref observer
-    Preferences::UnregisterCallback(ToolbarTipsPrefChanged,
-                                    "browser.chrome.toolbar_tips");
-  }
+  // Unregister our pref observer
+  Preferences::UnregisterCallback(ToolbarTipsPrefChanged,
+                                  "browser.chrome.toolbar_tips");
 }
 
 NS_IMPL_ISUPPORTS(nsXULTooltipListener, nsIDOMEventListener)
@@ -295,13 +293,12 @@ nsXULTooltipListener::ToolbarTipsPrefChanged(const char *aPref,
 //// nsXULTooltipListener
 
 bool nsXULTooltipListener::sShowTooltips = false;
-uint32_t nsXULTooltipListener::sTooltipListenerCount = 0;
 
-nsresult
+void
 nsXULTooltipListener::AddTooltipSupport(nsIContent* aNode)
 {
-  if (!aNode)
-    return NS_ERROR_NULL_POINTER;
+  MOZ_ASSERT(aNode);
+  MOZ_ASSERT(this == sInstance);
 
   aNode->AddSystemEventListener(NS_LITERAL_STRING("mouseout"), this,
                                 false, false);
@@ -313,23 +310,22 @@ nsXULTooltipListener::AddTooltipSupport(nsIContent* aNode)
                                 false, false);
   aNode->AddSystemEventListener(NS_LITERAL_STRING("dragstart"), this,
                                 true, false);
-
-  return NS_OK;
 }
 
-nsresult
+void
 nsXULTooltipListener::RemoveTooltipSupport(nsIContent* aNode)
 {
-  if (!aNode)
-    return NS_ERROR_NULL_POINTER;
+  MOZ_ASSERT(aNode);
+  MOZ_ASSERT(this == sInstance);
+
+  // The last reference to us can go after some of these calls.
+  RefPtr<nsXULTooltipListener> instance = this;
 
   aNode->RemoveSystemEventListener(NS_LITERAL_STRING("mouseout"), this, false);
   aNode->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"), this, false);
   aNode->RemoveSystemEventListener(NS_LITERAL_STRING("mousedown"), this, false);
   aNode->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"), this, false);
   aNode->RemoveSystemEventListener(NS_LITERAL_STRING("dragstart"), this, true);
-
-  return NS_OK;
 }
 
 #ifdef MOZ_XUL
@@ -713,7 +709,7 @@ nsXULTooltipListener::KillTooltipTimer()
 void
 nsXULTooltipListener::sTooltipCallback(nsITimer *aTimer, void *aListener)
 {
-  RefPtr<nsXULTooltipListener> instance = mInstance;
+  RefPtr<nsXULTooltipListener> instance = sInstance;
   if (instance)
     instance->ShowTooltip();
 }

layout/xul/nsXULTooltipListener.h

@@ -31,14 +31,13 @@ public:
   void MouseOut(nsIDOMEvent* aEvent);
   void MouseMove(nsIDOMEvent* aEvent);
 
-  nsresult AddTooltipSupport(nsIContent* aNode);
-  nsresult RemoveTooltipSupport(nsIContent* aNode);
+  void AddTooltipSupport(nsIContent* aNode);
+  void RemoveTooltipSupport(nsIContent* aNode);
   static nsXULTooltipListener* GetInstance() {
-    if (!mInstance)
-      mInstance = new nsXULTooltipListener();
-    return mInstance;
+    if (!sInstance)
+      sInstance = new nsXULTooltipListener();
+    return sInstance;
   }
-  static void ClearTooltipCache() { mInstance = nullptr; }
 
 protected:
 
@@ -47,7 +46,6 @@ protected:
 
   // pref callback for when the "show tooltips" pref changes
   static bool sShowTooltips;
-  static uint32_t sTooltipListenerCount;
 
   void KillTooltipTimer();
 
@@ -66,7 +64,7 @@ protected:
   // can be really used (i.e. tooltip is not a menu).
   nsresult GetTooltipFor(nsIContent* aTarget, nsIContent** aTooltip);
 
-  static nsXULTooltipListener* mInstance;
+  static nsXULTooltipListener* sInstance;
   static void ToolbarTipsPrefChanged(const char *aPref, void *aClosure);
 
   nsWeakPtr mSourceNode;