Bug 493701 - part2: Use TObserverArray for DocLoader Listeners; r=bzbarsky
☠☠ backed out by 73055a66d95a ☠ ☠
authorArpad Borsos <arpad.borsos@googlemail.com>
Sat, 19 Apr 2014 09:43:34 +0200
changeset 198914 f615a0532971b0671eea443dcd95558b3bdd3fd0
parent 198913 154fb9a9e0bca82352b76893d86f3ad419067816
child 198915 17bc5713639c75e4c34d44e1325be561b9a39937
push id3624
push userasasaki@mozilla.com
push dateMon, 09 Jun 2014 21:49:01 +0000
treeherdermozilla-beta@b1a5da15899a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky
bugs493701
milestone31.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 493701 - part2: Use TObserverArray for DocLoader Listeners; r=bzbarsky * * * Bug 493701 - part 3: add and use a BackwardIterator::Remove method; r=bzbarsky
uriloader/base/nsDocLoader.cpp
uriloader/base/nsDocLoader.h
--- a/uriloader/base/nsDocLoader.cpp
+++ b/uriloader/base/nsDocLoader.cpp
@@ -77,34 +77,30 @@ nsDocLoader::RequestInfoHashInitEntry(PL
 void
 nsDocLoader::RequestInfoHashClearEntry(PLDHashTable* table,
                                        PLDHashEntryHdr* entry)
 {
   nsRequestInfo* info = static_cast<nsRequestInfo *>(entry);
   info->~nsRequestInfo();
 }
 
-struct nsListenerInfo {
-  nsListenerInfo(nsIWeakReference *aListener, unsigned long aNotifyMask) 
-    : mWeakListener(aListener),
-      mNotifyMask(aNotifyMask)
-  {
-  }
-
-  // Weak pointer for the nsIWebProgressListener...
-  nsWeakPtr mWeakListener;
-
-  // Mask indicating which notifications the listener wants to receive.
-  unsigned long mNotifyMask;
+// this is used for mListenerInfoList.Contains()
+template <>
+class nsDefaultComparator <nsDocLoader::nsListenerInfo, nsIWebProgressListener*> {
+  public:
+    bool Equals(const nsDocLoader::nsListenerInfo& aInfo,
+                nsIWebProgressListener* const& aListener) const {
+      nsCOMPtr<nsIWebProgressListener> listener =
+                                       do_QueryReferent(aInfo.mWeakListener);
+      return aListener == listener;
+    }
 };
 
-
 nsDocLoader::nsDocLoader()
   : mParent(nullptr),
-    mListenerInfoList(8),
     mCurrentSelfProgress(0),
     mMaxSelfProgress(0),
     mCurrentTotalProgress(0),
     mMaxTotalProgress(0),
     mCompletedTotalProgress(0),
     mIsLoadingDocument(false),
     mIsRestoringDocument(false),
     mDontFlushLayout(false),
@@ -370,25 +366,16 @@ nsDocLoader::Destroy()
   if (mParent) 
   {
     mParent->RemoveChildLoader(this);
   }
 
   // Release all the information about network requests...
   ClearRequestInfoHash();
 
-  // Release all the information about registered listeners...
-  int32_t count = mListenerInfoList.Count();
-  for(int32_t i = 0; i < count; i++) {
-    nsListenerInfo *info =
-      static_cast<nsListenerInfo*>(mListenerInfoList.ElementAt(i));
-
-    delete info;
-  }
-
   mListenerInfoList.Clear();
   mListenerInfoList.Compact();
 
   mDocumentRequest = 0;
 
   if (mLoadGroup)
     mLoadGroup->SetGroupObserver(nullptr);
 
@@ -876,54 +863,36 @@ void nsDocLoader::doStopDocumentLoad(nsI
 }
 
 ////////////////////////////////////////////////////////////////////////////////////
 // The following section contains support for nsIWebProgress and related stuff
 ////////////////////////////////////////////////////////////////////////////////////
 
 NS_IMETHODIMP
 nsDocLoader::AddProgressListener(nsIWebProgressListener *aListener,
-                                     uint32_t aNotifyMask)
+                                 uint32_t aNotifyMask)
 {
-  nsresult rv;
-
-  nsListenerInfo* info = GetListenerInfo(aListener);
-  if (info) {
+  if (mListenerInfoList.Contains(aListener)) {
     // The listener is already registered!
     return NS_ERROR_FAILURE;
   }
 
   nsWeakPtr listener = do_GetWeakReference(aListener);
   if (!listener) {
     return NS_ERROR_INVALID_ARG;
   }
 
-  info = new nsListenerInfo(listener, aNotifyMask);
-  if (!info) {
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  rv = mListenerInfoList.AppendElement(info) ? NS_OK : NS_ERROR_FAILURE;
-  return rv;
+  return mListenerInfoList.AppendElement(nsListenerInfo(listener, aNotifyMask)) ?
+         NS_OK : NS_ERROR_OUT_OF_MEMORY;
 }
 
 NS_IMETHODIMP
 nsDocLoader::RemoveProgressListener(nsIWebProgressListener *aListener)
 {
-  nsresult rv;
-
-  nsListenerInfo* info = GetListenerInfo(aListener);
-  if (info) {
-    rv = mListenerInfoList.RemoveElement(info) ? NS_OK : NS_ERROR_FAILURE;
-    delete info;
-  } else {
-    // The listener is not registered!
-    rv = NS_ERROR_FAILURE;
-  }
-  return rv;
+  return mListenerInfoList.RemoveElement(aListener) ? NS_OK : NS_ERROR_FAILURE;
 }
 
 NS_IMETHODIMP
 nsDocLoader::GetDOMWindow(nsIDOMWindow **aResult)
 {
   return CallGetInterface(this, aResult);
 }
 
@@ -1175,16 +1144,38 @@ void nsDocLoader::ClearInternalProgress(
 
   mCurrentSelfProgress  = mMaxSelfProgress  = 0;
   mCurrentTotalProgress = mMaxTotalProgress = 0;
   mCompletedTotalProgress = 0;
 
   mProgressStateFlags = nsIWebProgressListener::STATE_STOP;
 }
 
+/**
+ * |_code| is executed for every listener matching |_flag|
+ * |listener| should be used inside |_code| as the nsIWebProgressListener var.
+ */
+#define NOTIFY_LISTENERS(_flag, _code)                     \
+PR_BEGIN_MACRO                                             \
+  nsCOMPtr<nsIWebProgressListener> listener;               \
+  ListenerArray::BackwardIterator iter(mListenerInfoList); \
+  while (iter.HasMore()) {                                 \
+    nsListenerInfo &info = iter.GetNext();                 \
+    if (!(info.mNotifyMask & (_flag))) {                   \
+      continue;                                            \
+    }                                                      \
+    listener = do_QueryReferent(info.mWeakListener);       \
+    if (!listener) {                                       \
+      iter.Remove();                                       \
+      continue;                                            \
+    }                                                      \
+    _code                                                  \
+  }                                                        \
+  mListenerInfoList.Compact();                             \
+PR_END_MACRO
 
 void nsDocLoader::FireOnProgressChange(nsDocLoader *aLoadInitiator,
                                        nsIRequest *request,
                                        int64_t aProgress,
                                        int64_t aProgressMax,
                                        int64_t aProgressDelta,
                                        int64_t aTotalProgress,
                                        int64_t aMaxTotalProgress)
@@ -1201,48 +1192,22 @@ void nsDocLoader::FireOnProgressChange(n
   nsAutoCString buffer;
 
   GetURIStringFromRequest(request, buffer);
   PR_LOG(gDocLoaderLog, PR_LOG_DEBUG, 
          ("DocLoader:%p: Progress (%s): curSelf: %d maxSelf: %d curTotal: %d maxTotal %d\n",
           this, buffer.get(), aProgress, aProgressMax, aTotalProgress, aMaxTotalProgress));
 #endif /* DEBUG */
 
-  /*
-   * First notify any listeners of the new progress info...
-   *
-   * Operate the elements from back to front so that if items get
-   * get removed from the list it won't affect our iteration
-   */
-  nsCOMPtr<nsIWebProgressListener> listener;
-  int32_t count = mListenerInfoList.Count();
-
-  while (--count >= 0) {
-    nsListenerInfo *info;
-
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(count));
-    if (!info || !(info->mNotifyMask & nsIWebProgress::NOTIFY_PROGRESS)) {
-      continue;
-    }
-
-    listener = do_QueryReferent(info->mWeakListener);
-    if (!listener) {
-      // the listener went away. gracefully pull it out of the list.
-      mListenerInfoList.RemoveElementAt(count);
-      delete info;
-      continue;
-    }
-
+  NOTIFY_LISTENERS(nsIWebProgress::NOTIFY_PROGRESS,
     // XXX truncates 64-bit to 32-bit
     listener->OnProgressChange(aLoadInitiator,request,
                                int32_t(aProgress), int32_t(aProgressMax),
                                int32_t(aTotalProgress), int32_t(aMaxTotalProgress));
-  }
-
-  mListenerInfoList.Compact();
+  );
 
   // Pass the notification up to the parent...
   if (mParent) {
     mParent->FireOnProgressChange(aLoadInitiator, request,
                                   aProgress, aProgressMax,
                                   aProgressDelta,
                                   aTotalProgress, aMaxTotalProgress);
   }
@@ -1295,215 +1260,92 @@ void nsDocLoader::DoFireOnStateChange(ns
   GetURIStringFromRequest(aRequest, buffer);
   PR_LOG(gDocLoaderLog, PR_LOG_DEBUG, 
          ("DocLoader:%p: Status (%s): code: %x\n",
          this, buffer.get(), aStateFlags));
 #endif /* DEBUG */
 
   NS_ASSERTION(aRequest, "Firing OnStateChange(...) notification with a NULL request!");
 
-  /*                                                                           
-   * First notify any listeners of the new state info...
-   *
-   * Operate the elements from back to front so that if items get
-   * get removed from the list it won't affect our iteration
-   */
-  nsCOMPtr<nsIWebProgressListener> listener;
-  int32_t count = mListenerInfoList.Count();
-  int32_t notifyMask = (aStateFlags >> 16) & nsIWebProgress::NOTIFY_STATE_ALL;
-
-  while (--count >= 0) {
-    nsListenerInfo *info;
-
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(count));
-    if (!info || !(info->mNotifyMask & notifyMask)) {
-      continue;
-    }
-
-    listener = do_QueryReferent(info->mWeakListener);
-    if (!listener) {
-      // the listener went away. gracefully pull it out of the list.
-      mListenerInfoList.RemoveElementAt(count);
-      delete info;
-      continue;
-    }
-
+  NOTIFY_LISTENERS(((aStateFlags >> 16) & nsIWebProgress::NOTIFY_STATE_ALL),
     listener->OnStateChange(aProgress, aRequest, aStateFlags, aStatus);
-  }
-
-  mListenerInfoList.Compact();
+  );
 }
 
 
 
 void
 nsDocLoader::FireOnLocationChange(nsIWebProgress* aWebProgress,
                                   nsIRequest* aRequest,
                                   nsIURI *aUri,
                                   uint32_t aFlags)
 {
-  /*                                                                           
-   * First notify any listeners of the new state info...
-   *
-   * Operate the elements from back to front so that if items get
-   * get removed from the list it won't affect our iteration
-   */
-  nsCOMPtr<nsIWebProgressListener> listener;
-  int32_t count = mListenerInfoList.Count();
-
-  while (--count >= 0) {
-    nsListenerInfo *info;
-
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(count));
-    if (!info || !(info->mNotifyMask & nsIWebProgress::NOTIFY_LOCATION)) {
-      continue;
-    }
-
-    listener = do_QueryReferent(info->mWeakListener);
-    if (!listener) {
-      // the listener went away. gracefully pull it out of the list.
-      mListenerInfoList.RemoveElementAt(count);
-      delete info;
-      continue;
-    }
-
+  NOTIFY_LISTENERS(nsIWebProgress::NOTIFY_LOCATION,
     PR_LOG(gDocLoaderLog, PR_LOG_DEBUG, ("DocLoader [%p] calling %p->OnLocationChange", this, listener.get()));
     listener->OnLocationChange(aWebProgress, aRequest, aUri, aFlags);
-  }
-
-  mListenerInfoList.Compact();
+  );
 
   // Pass the notification up to the parent...
   if (mParent) {
     mParent->FireOnLocationChange(aWebProgress, aRequest, aUri, aFlags);
   }
 }
 
 void
 nsDocLoader::FireOnStatusChange(nsIWebProgress* aWebProgress,
                                 nsIRequest* aRequest,
                                 nsresult aStatus,
                                 const char16_t* aMessage)
 {
-  /*                                                                           
-   * First notify any listeners of the new state info...
-   *
-   * Operate the elements from back to front so that if items get
-   * get removed from the list it won't affect our iteration
-   */
-  nsCOMPtr<nsIWebProgressListener> listener;
-  int32_t count = mListenerInfoList.Count();
-
-  while (--count >= 0) {
-    nsListenerInfo *info;
-
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(count));
-    if (!info || !(info->mNotifyMask & nsIWebProgress::NOTIFY_STATUS)) {
-      continue;
-    }
-
-    listener = do_QueryReferent(info->mWeakListener);
-    if (!listener) {
-      // the listener went away. gracefully pull it out of the list.
-      mListenerInfoList.RemoveElementAt(count);
-      delete info;
-      continue;
-    }
-
+  NOTIFY_LISTENERS(nsIWebProgress::NOTIFY_STATUS,
     listener->OnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
-  }
-  mListenerInfoList.Compact();
+  );
   
   // Pass the notification up to the parent...
   if (mParent) {
     mParent->FireOnStatusChange(aWebProgress, aRequest, aStatus, aMessage);
   }
 }
 
 bool
 nsDocLoader::RefreshAttempted(nsIWebProgress* aWebProgress,
                               nsIURI *aURI,
                               int32_t aDelay,
                               bool aSameURI)
 {
   /*
    * Returns true if the refresh may proceed,
    * false if the refresh should be blocked.
-   *
-   * First notify any listeners of the refresh attempt...
-   *
-   * Iterate the elements from back to front so that if items
-   * get removed from the list it won't affect our iteration
    */
   bool allowRefresh = true;
-  int32_t count = mListenerInfoList.Count();
 
-  while (--count >= 0) {
-    nsListenerInfo *info;
-
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(count));
-    if (!info || !(info->mNotifyMask & nsIWebProgress::NOTIFY_REFRESH)) {
-      continue;
-    }
-
-    nsCOMPtr<nsIWebProgressListener> listener =
-      do_QueryReferent(info->mWeakListener);
-    if (!listener) {
-      // the listener went away. gracefully pull it out of the list.
-      mListenerInfoList.RemoveElementAt(count);
-      delete info;
-      continue;
-    }
-
+  NOTIFY_LISTENERS(nsIWebProgress::NOTIFY_REFRESH,
     nsCOMPtr<nsIWebProgressListener2> listener2 =
-      do_QueryReferent(info->mWeakListener);
+      do_QueryReferent(info.mWeakListener);
     if (!listener2)
       continue;
 
     bool listenerAllowedRefresh;
     nsresult listenerRV = listener2->OnRefreshAttempted(
         aWebProgress, aURI, aDelay, aSameURI, &listenerAllowedRefresh);
     if (NS_FAILED(listenerRV))
       continue;
 
     allowRefresh = allowRefresh && listenerAllowedRefresh;
-  }
-
-  mListenerInfoList.Compact();
+  );
 
   // Pass the notification up to the parent...
   if (mParent) {
     allowRefresh = allowRefresh &&
       mParent->RefreshAttempted(aWebProgress, aURI, aDelay, aSameURI);
   }
 
   return allowRefresh;
 }
 
-nsListenerInfo * 
-nsDocLoader::GetListenerInfo(nsIWebProgressListener *aListener)
-{
-  int32_t i, count;
-  nsListenerInfo *info;
-
-  nsCOMPtr<nsISupports> listener1 = do_QueryInterface(aListener);
-  count = mListenerInfoList.Count();
-  for (i=0; i<count; i++) {
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(i));
-
-    NS_ASSERTION(info, "There should NEVER be a null listener in the list");
-    if (info) {
-      nsCOMPtr<nsISupports> listener2 = do_QueryReferent(info->mWeakListener);
-      if (listener1 == listener2)
-        return info;
-    }
-  }
-  return nullptr;
-}
-
 nsresult nsDocLoader::AddRequestInfo(nsIRequest *aRequest)
 {
   if (!PL_DHashTableOperate(&mRequestInfoHash, aRequest, PL_DHASH_ADD)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   return NS_OK;
 }
@@ -1618,45 +1460,19 @@ NS_IMETHODIMP nsDocLoader::OnSecurityCha
 {
   //
   // Fire progress notifications out to any registered nsIWebProgressListeners.  
   //
   
   nsCOMPtr<nsIRequest> request = do_QueryInterface(aContext);
   nsIWebProgress* webProgress = static_cast<nsIWebProgress*>(this);
 
-  /*                                                                           
-   * First notify any listeners of the new state info...
-   *
-   * Operate the elements from back to front so that if items get
-   * get removed from the list it won't affect our iteration
-   */
-  nsCOMPtr<nsIWebProgressListener> listener;
-  int32_t count = mListenerInfoList.Count();
-
-  while (--count >= 0) {
-    nsListenerInfo *info;
-
-    info = static_cast<nsListenerInfo*>(mListenerInfoList.SafeElementAt(count));
-    if (!info || !(info->mNotifyMask & nsIWebProgress::NOTIFY_SECURITY)) {
-      continue;
-    }
-
-    listener = do_QueryReferent(info->mWeakListener);
-    if (!listener) {
-      // the listener went away. gracefully pull it out of the list.
-      mListenerInfoList.RemoveElementAt(count);
-      delete info;
-      continue;
-    }
-
+  NOTIFY_LISTENERS(nsIWebProgress::NOTIFY_SECURITY,
     listener->OnSecurityChange(webProgress, request, aState);
-  }
-
-  mListenerInfoList.Compact();
+  );
 
   // Pass the notification up to the parent...
   if (mParent) {
     mParent->OnSecurityChange(aContext, aState);
   }
   return NS_OK;
 }
 
--- a/uriloader/base/nsDocLoader.h
+++ b/uriloader/base/nsDocLoader.h
@@ -12,33 +12,30 @@
 #include "nsIDocumentLoader.h"
 #include "nsIWebProgress.h"
 #include "nsIWebProgressListener.h"
 #include "nsIRequestObserver.h"
 #include "nsWeakReference.h"
 #include "nsILoadGroup.h"
 #include "nsCOMArray.h"
 #include "nsTObserverArray.h"
-#include "nsVoidArray.h"
 #include "nsString.h"
 #include "nsIChannel.h"
 #include "nsIProgressEventSink.h"
 #include "nsIInterfaceRequestor.h"
 #include "nsIInterfaceRequestorUtils.h"
 #include "nsIChannelEventSink.h"
 #include "nsISecurityEventSink.h"
 #include "nsISupportsPriority.h"
 #include "nsCOMPtr.h"
 #include "pldhash.h"
 #include "nsAutoPtr.h"
 
 #include "mozilla/LinkedList.h"
 
-struct nsListenerInfo;
-
 /****************************************************************************
  * nsDocLoader implementation...
  ****************************************************************************/
 
 #define NS_THIS_DOCLOADER_IMPL_CID                    \
  { /* b4ec8387-98aa-4c08-93b6-6d23069c06f2 */         \
      0xb4ec8387,                                      \
      0x98aa,                                          \
@@ -93,16 +90,30 @@ public:
     // Remove aChild from our childlist.  This nulls out the child's mParent
     // pointer.
     nsresult RemoveChildLoader(nsDocLoader *aChild);
     // Add aChild to our child list.  This will set aChild's mParent pointer to
     // |this|.
     nsresult AddChildLoader(nsDocLoader* aChild);
     nsDocLoader* GetParent() const { return mParent; }
 
+    struct nsListenerInfo {
+      nsListenerInfo(nsIWeakReference *aListener, unsigned long aNotifyMask) 
+        : mWeakListener(aListener),
+          mNotifyMask(aNotifyMask)
+      {
+      }
+
+      // Weak pointer for the nsIWebProgressListener...
+      nsWeakPtr mWeakListener;
+
+      // Mask indicating which notifications the listener wants to receive.
+      unsigned long mNotifyMask;
+    };
+
 protected:
     virtual ~nsDocLoader();
 
     virtual nsresult SetDocLoaderParent(nsDocLoader * aLoader);
 
     bool IsBusy();
 
     void Destroy();
@@ -244,17 +255,18 @@ protected:
     // for owning pointers and raw COM interface pointers for weak
     // (ie, non owning) references. If you add any members to this
     // class, please make the ownership explicit (pinkerton, scc).
   
     nsCOMPtr<nsIRequest>       mDocumentRequest;       // [OWNER] ???compare with document
 
     nsDocLoader*               mParent;                // [WEAK]
 
-    nsVoidArray                mListenerInfoList;
+    typedef nsAutoTObserverArray<nsListenerInfo, 8> ListenerArray;
+    ListenerArray              mListenerInfoList;
 
     nsCOMPtr<nsILoadGroup>        mLoadGroup;
     // We hold weak refs to all our kids
     nsTObserverArray<nsDocLoader*> mChildList;
 
     // The following member variables are related to the new nsIWebProgress 
     // feedback interfaces that travis cooked up.
     int32_t mProgressStateFlags;
@@ -299,18 +311,16 @@ private:
     
     // DocLoaderIsEmpty should be called whenever the docloader may be empty.
     // This method is idempotent and does nothing if the docloader is not in
     // fact empty.  This method _does_ make sure that layout is flushed if our
     // loadgroup has no active requests before checking for "real" emptiness if
     // aFlushLayout is true.
     void DocLoaderIsEmpty(bool aFlushLayout);
 
-    nsListenerInfo *GetListenerInfo(nsIWebProgressListener* aListener);
-
     int64_t GetMaxTotalProgress();
 
     nsresult AddRequestInfo(nsIRequest* aRequest);
     void RemoveRequestInfo(nsIRequest* aRequest);
     nsRequestInfo *GetRequestInfo(nsIRequest* aRequest);
     void ClearRequestInfoHash();
     int64_t CalculateMaxProgress();
     static PLDHashOperator CalcMaxProgressCallback(PLDHashTable* table,