Bug 1440682: Make the XUL tooltip stuff saner. r=enn
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 23 Feb 2018 19:25:34 +0100
changeset 460285 e3cce6ae4b1569a180aba116908d432483fa7b04
parent 460284 0dfc8b69311128b0b4cf356a6b7151daf77fa986
child 460286 00b83d62b16b9ddd27b76f144c9a8cf6d8ecfd19
push id1683
push usersfraser@mozilla.com
push dateThu, 26 Apr 2018 16:43:40 +0000
treeherdermozilla-release@5af6cb21869d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersenn
bugs1440682
milestone60.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 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
dom/xul/nsXULElement.h
layout/base/GeckoRestyleManager.cpp
layout/base/nsCSSFrameConstructor.cpp
layout/xul/nsIRootBox.h
layout/xul/nsRootBoxFrame.cpp
layout/xul/nsXULTooltipListener.cpp
layout/xul/nsXULTooltipListener.h
--- a/dom/xul/nsXULElement.cpp
+++ b/dom/xul/nsXULElement.cpp
@@ -76,16 +76,17 @@
 #include "nsQueryObject.h"
 #include <algorithm>
 #include "nsIDOMChromeWindow.h"
 
 #include "nsReadableUtils.h"
 #include "nsIFrame.h"
 #include "nsNodeInfoManager.h"
 #include "nsXBLBinding.h"
+#include "nsXULTooltipListener.h"
 #include "mozilla/EventDispatcher.h"
 #include "mozAutoDocUpdate.h"
 #include "nsIDOMXULCommandEvent.h"
 #include "nsCCUncollectableMarker.h"
 #include "nsICSSDeclaration.h"
 #include "nsLayoutUtils.h"
 
 #include "mozilla/dom/XULElementBinding.h"
@@ -729,16 +730,29 @@ public:
     mDocument->WarnOnceAbout(nsIDocument::eImportXULIntoContent, false);
     return NS_OK;
   }
 
 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,
                          nsIContent* aBindingParent,
                          bool aCompileEventHandlers)
 {
   if (!aBindingParent &&
       aDocument &&
@@ -781,29 +795,37 @@ nsXULElement::BindToTree(nsIDocument* aD
       // pulling in xul.css.
       // Note that add-ons may introduce bindings that cause this assertion to
       // fire.
       NS_ASSERTION(IsInVideoControls(this),
                    "Unexpected XUL element in non-XUL doc");
     }
   }
 
+  if (doc && NeedTooltipSupport(*this)) {
+      AddTooltipSupport();
+  }
+
   if (aDocument) {
       NS_ASSERTION(!nsContentUtils::IsSafeToRunScript(),
                    "Missing a script blocker!");
       // We're in a document now.  Kick off the frame load.
       LoadSrc();
   }
 
   return rv;
 }
 
 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
     // object's script object from being garbage collected,
     // which means JS continues to hold an owning reference
     // to the nsGlobalWindow, which owns the document,
     // which owns this content.  That's a cycle, so we break
     // it here.  (It might be better to break this by releasing
@@ -1207,24 +1229,57 @@ nsXULElement::AfterSetAttr(int32_t aName
                 } else if (aName == nsGkAtoms::drawintitlebar) {
                     SetDrawsInTitlebar(false);
                 } else if (aName == nsGkAtoms::drawtitle) {
                     SetDrawsTitle(false);
                 }
             }
         }
 
+        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.
     }
 
     return nsStyledElement::AfterSetAttr(aNamespaceID, 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,
                              const nsAString& aValue,
                              nsIPrincipal* aMaybeScriptedPrincipal,
                              nsAttrValue& aResult)
 {
     // Parse into a nsAttrValue
--- a/dom/xul/nsXULElement.h
+++ b/dom/xul/nsXULElement.h
@@ -749,16 +749,19 @@ protected:
 
     void SetDrawsInTitlebar(bool aState);
     void SetDrawsTitle(bool aState);
     void UpdateBrightTitlebarForeground(nsIDocument* aDocument);
 
     void RemoveBroadcaster(const nsAString & broadcasterId);
 
 protected:
+    void AddTooltipSupport();
+    void RemoveTooltipSupport();
+
     // Internal accessor. This shadows the 'Slots', and returns
     // appropriate value.
     nsIControllers *Controllers() {
       nsExtendedDOMSlots* slots = GetExistingExtendedDOMSlots();
       return slots ? slots->mControllers.get() : nullptr;
     }
 
     void UnregisterAccessKey(const nsAString& aOldValue);
--- a/layout/base/GeckoRestyleManager.cpp
+++ b/layout/base/GeckoRestyleManager.cpp
@@ -389,29 +389,16 @@ GeckoRestyleManager::AttributeChanged(El
     nsAtom* tag = PresContext()->Document()->BindingManager()->
                      ResolveTag(aElement, &namespaceID);
 
     if (namespaceID == kNameSpaceID_XUL &&
         (tag == nsGkAtoms::listitem ||
          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) {
     // See if we have appearance information for a theme.
     const nsStyleDisplay* disp = primaryFrame->StyleDisplay();
     if (disp->mAppearance) {
       nsITheme* theme = PresContext()->GetTheme();
       if (theme && theme->ThemeSupportsWidget(PresContext(), primaryFrame, disp->mAppearance)) {
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -4270,29 +4270,16 @@ nsCSSFrameConstructor::ConstructFrameFro
 
       // Set the frame's initial child list
       // Note that MathML depends on this being called even if
       // childItems is empty!
       newFrameAsContainer->SetInitialChildList(kPrincipalList, childItems);
     }
   }
 
-#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");
 
   if (aItem.mIsAnonymousContentCreatorContent) {
     primaryFrame->AddStateBits(NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT);
   }
 
--- a/layout/xul/nsIRootBox.h
+++ b/layout/xul/nsIRootBox.h
@@ -25,16 +25,13 @@ public:
   NS_DECL_QUERYFRAME_TARGET(nsIRootBox)
 
   virtual nsPopupSetFrame* GetPopupSetFrame() = 0;
   virtual void SetPopupSetFrame(nsPopupSetFrame* aPopupSet) = 0;
 
   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);
 };
 
 #endif
 
--- a/layout/xul/nsRootBoxFrame.cpp
+++ b/layout/xul/nsRootBoxFrame.cpp
@@ -7,17 +7,16 @@
 #include "nsHTMLParts.h"
 #include "nsStyleConsts.h"
 #include "nsGkAtoms.h"
 #include "nsIPresShell.h"
 #include "nsBoxFrame.h"
 #include "nsStackLayout.h"
 #include "nsIRootBox.h"
 #include "nsIContent.h"
-#include "nsXULTooltipListener.h"
 #include "nsFrameManager.h"
 #include "mozilla/BasicEvents.h"
 
 using namespace mozilla;
 
 // Interface IDs
 
 //#define DEBUG_REFLOW
@@ -52,18 +51,16 @@ public:
 
   NS_DECL_QUERYFRAME
   NS_DECL_FRAMEARENA_HELPERS(nsRootBoxFrame)
 
   virtual nsPopupSetFrame* GetPopupSetFrame() override;
   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;
   virtual void InsertFrames(ChildListID     aListID,
                             nsIFrame*       aPrevFrame,
                             nsFrameList&    aFrameList) override;
   virtual void RemoveFrame(ChildListID     aListID,
                            nsIFrame*       aOldFrame) override;
@@ -238,38 +235,16 @@ nsRootBoxFrame::GetDefaultTooltip()
 }
 
 void
 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)
 
 #ifdef DEBUG_FRAME_DUMP
 nsresult
 nsRootBoxFrame::GetFrameName(nsAString& aResult) const
 {
--- a/layout/xul/nsXULTooltipListener.cpp
+++ b/layout/xul/nsXULTooltipListener.cpp
@@ -30,53 +30,51 @@
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/Event.h" // for nsIDOMEvent::InternalDOMEvent()
 #include "mozilla/dom/BoxObject.h"
 #include "mozilla/TextEvents.h"
 
 using namespace mozilla;
 using namespace mozilla::dom;
 
-nsXULTooltipListener* nsXULTooltipListener::mInstance = nullptr;
+nsXULTooltipListener* nsXULTooltipListener::sInstance = nullptr;
 
 //////////////////////////////////////////////////////////////////////////
 //// nsISupports
 
 nsXULTooltipListener::nsXULTooltipListener()
   : mMouseScreenX(0)
   , mMouseScreenY(0)
   , mTooltipShownOnce(false)
 #ifdef MOZ_XUL
   , mIsSourceTree(false)
   , mNeedTitletip(false)
   , 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)
 
 void
 nsXULTooltipListener::MouseOut(nsIDOMEvent* aEvent)
 {
   // reset flag so that tooltip will display on the next MouseMove
@@ -290,51 +288,49 @@ nsXULTooltipListener::ToolbarTipsPrefCha
   sShowTooltips =
     Preferences::GetBool("browser.chrome.toolbar_tips", sShowTooltips);
 }
 
 //////////////////////////////////////////////////////////////////////////
 //// 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);
   aNode->AddSystemEventListener(NS_LITERAL_STRING("mousemove"), this,
                                 false, false);
   aNode->AddSystemEventListener(NS_LITERAL_STRING("mousedown"), this,
                                 false, false);
   aNode->AddSystemEventListener(NS_LITERAL_STRING("mouseup"), this,
                                 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
 void
 nsXULTooltipListener::CheckTreeBodyMove(nsIDOMMouseEvent* aMouseEvent)
 {
   nsCOMPtr<nsIContent> sourceNode = do_QueryReferent(mSourceNode);
   if (!sourceNode)
@@ -708,17 +704,17 @@ nsXULTooltipListener::KillTooltipTimer()
     mTooltipTimer = nullptr;
     mTargetNode = nullptr;
   }
 }
 
 void
 nsXULTooltipListener::sTooltipCallback(nsITimer *aTimer, void *aListener)
 {
-  RefPtr<nsXULTooltipListener> instance = mInstance;
+  RefPtr<nsXULTooltipListener> instance = sInstance;
   if (instance)
     instance->ShowTooltip();
 }
 
 #ifdef MOZ_XUL
 nsresult
 nsXULTooltipListener::GetSourceTreeBoxObject(nsITreeBoxObject** aBoxObject)
 {
--- a/layout/xul/nsXULTooltipListener.h
+++ b/layout/xul/nsXULTooltipListener.h
@@ -26,33 +26,31 @@ class nsXULTooltipListener final : publi
 {
 public:
   NS_DECL_ISUPPORTS
   NS_DECL_NSIDOMEVENTLISTENER
 
   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:
 
   nsXULTooltipListener();
   ~nsXULTooltipListener();
 
   // pref callback for when the "show tooltips" pref changes
   static bool sShowTooltips;
-  static uint32_t sTooltipListenerCount;
 
   void KillTooltipTimer();
 
 #ifdef MOZ_XUL
   void CheckTreeBodyMove(nsIDOMMouseEvent* aMouseEvent);
   nsresult GetSourceTreeBoxObject(nsITreeBoxObject** aBoxObject);
 #endif
 
@@ -61,17 +59,17 @@ protected:
   nsresult HideTooltip();
   nsresult DestroyTooltip();
   // This method tries to find a tooltip for aTarget.
   nsresult FindTooltip(nsIContent* aTarget, nsIContent** aTooltip);
   // This method calls FindTooltip and checks that the tooltip
   // 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;
   nsWeakPtr mTargetNode;
   nsWeakPtr mCurrentTooltip;
 
   // a timer for showing the tooltip
   nsCOMPtr<nsITimer> mTooltipTimer;