Bug 1433345 - part 2: Make HTMLEditor store ComposerCommandsUpdater directly r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 26 Jan 2018 14:38:05 +0900
changeset 453670 510153b3ba8d15e49eeb8145d662249ca80aece5
parent 453669 d0cd3d4c1a0477b5990cfa9212c18df6bf0af762
child 453671 f2d590678c07c7e99ed2e7a424ad64796ddefadf
push id8799
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 16:46:23 +0000
treeherdermozilla-beta@15334014dc67 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1433345
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 1433345 - part 2: Make HTMLEditor store ComposerCommandsUpdater directly r=m_kato For making ComposerCommandsUpdater not derived from nsISelectionListener, HTMLEditor needs to store it directly. This patch also makes ComposerCommandsUpdater cycle collectable because it stores a strong pointer and HTMLEditor also needs to store it with RefPtr. Therefore, ComposerCommandsUpdater becomes unnecessary to use nsWeakPtr. MozReview-Commit-ID: 2WZnLdq15FK
editor/composer/ComposerCommandsUpdater.cpp
editor/composer/ComposerCommandsUpdater.h
editor/composer/nsEditingSession.cpp
editor/composer/nsEditingSession.h
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditor.h
--- a/editor/composer/ComposerCommandsUpdater.cpp
+++ b/editor/composer/ComposerCommandsUpdater.cpp
@@ -38,22 +38,33 @@ ComposerCommandsUpdater::ComposerCommand
 ComposerCommandsUpdater::~ComposerCommandsUpdater()
 {
   // cancel any outstanding update timer
   if (mUpdateTimer) {
     mUpdateTimer->Cancel();
   }
 }
 
-NS_IMPL_ISUPPORTS(ComposerCommandsUpdater,
-                  nsISelectionListener,
-                  nsIDocumentStateListener,
-                  nsITransactionListener,
-                  nsITimerCallback,
-                  nsINamed)
+NS_IMPL_CYCLE_COLLECTING_ADDREF(ComposerCommandsUpdater)
+NS_IMPL_CYCLE_COLLECTING_RELEASE(ComposerCommandsUpdater)
+
+NS_INTERFACE_MAP_BEGIN(ComposerCommandsUpdater)
+  NS_INTERFACE_MAP_ENTRY(nsISelectionListener)
+  NS_INTERFACE_MAP_ENTRY(nsIDocumentStateListener)
+  NS_INTERFACE_MAP_ENTRY(nsITransactionListener)
+  NS_INTERFACE_MAP_ENTRY(nsITimerCallback)
+  NS_INTERFACE_MAP_ENTRY(nsINamed)
+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDocumentStateListener)
+  NS_INTERFACE_MAP_ENTRIES_CYCLE_COLLECTION(ComposerCommandsUpdater)
+NS_INTERFACE_MAP_END
+
+NS_IMPL_CYCLE_COLLECTION(ComposerCommandsUpdater,
+                         mUpdateTimer,
+                         mDOMWindow,
+                         mDocShell)
 
 #if 0
 #pragma mark -
 #endif
 
 NS_IMETHODIMP
 ComposerCommandsUpdater::NotifyDocumentCreated()
 {
@@ -220,19 +231,21 @@ ComposerCommandsUpdater::DidMerge(nsITra
 
 #if 0
 #pragma mark -
 #endif
 
 nsresult
 ComposerCommandsUpdater::Init(nsPIDOMWindowOuter* aDOMWindow)
 {
-  NS_ENSURE_ARG(aDOMWindow);
-  mDOMWindow = do_GetWeakReference(aDOMWindow);
-  mDocShell = do_GetWeakReference(aDOMWindow->GetDocShell());
+  if (NS_WARN_IF(!aDOMWindow)) {
+    return NS_ERROR_INVALID_ARG;
+  }
+  mDOMWindow = aDOMWindow;
+  mDocShell = aDOMWindow->GetDocShell();
   return NS_OK;
 }
 
 nsresult
 ComposerCommandsUpdater::PrimeUpdateTimer()
 {
   if (!mUpdateTimer) {
     mUpdateTimer = NS_NewTimer();;
@@ -339,33 +352,36 @@ ComposerCommandsUpdater::UpdateOneComman
   commandUpdater->CommandStatusChanged(aCommand);
 
   return NS_OK;
 }
 
 bool
 ComposerCommandsUpdater::SelectionIsCollapsed()
 {
-  nsCOMPtr<nsPIDOMWindowOuter> domWindow = do_QueryReferent(mDOMWindow);
-  NS_ENSURE_TRUE(domWindow, true);
+  if (NS_WARN_IF(!mDOMWindow)) {
+    return true;
+  }
 
-  nsCOMPtr<nsISelection> domSelection = domWindow->GetSelection();
+  nsCOMPtr<nsISelection> domSelection = mDOMWindow->GetSelection();
   if (NS_WARN_IF(!domSelection)) {
     return false;
   }
 
   return domSelection->AsSelection()->IsCollapsed();
 }
 
 already_AddRefed<nsPICommandUpdater>
 ComposerCommandsUpdater::GetCommandUpdater()
 {
-  nsCOMPtr<nsIDocShell> docShell = do_QueryReferent(mDocShell);
-  NS_ENSURE_TRUE(docShell, nullptr);
-  nsCOMPtr<nsICommandManager> manager = docShell->GetCommandManager();
+  if (NS_WARN_IF(!mDocShell)) {
+    return nullptr;
+  }
+
+  nsCOMPtr<nsICommandManager> manager = mDocShell->GetCommandManager();
   nsCOMPtr<nsPICommandUpdater> updater = do_QueryInterface(manager);
   return updater.forget();
 }
 
 NS_IMETHODIMP
 ComposerCommandsUpdater::GetName(nsACString& aName)
 {
   aName.AssignLiteral("ComposerCommandsUpdater");
--- a/editor/composer/ComposerCommandsUpdater.h
+++ b/editor/composer/ComposerCommandsUpdater.h
@@ -3,43 +3,47 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_ComposerCommandsUpdater_h
 #define mozilla_ComposerCommandsUpdater_h
 
 #include "nsCOMPtr.h"                   // for already_AddRefed, nsCOMPtr
+#include "nsCycleCollectionParticipant.h"
 #include "nsIDocumentStateListener.h"
 #include "nsINamed.h"
 #include "nsISelectionListener.h"
 #include "nsISupportsImpl.h"            // for NS_DECL_ISUPPORTS
 #include "nsITimer.h"                   // for NS_DECL_NSITIMERCALLBACK, etc
 #include "nsITransactionListener.h"     // for nsITransactionListener
 #include "nsIWeakReferenceUtils.h"      // for nsWeakPtr
 #include "nscore.h"                     // for NS_IMETHOD, nsresult, etc
 
-class nsPIDOMWindowOuter;
+class nsIDocShell;
 class nsITransaction;
 class nsITransactionManager;
+class nsPIDOMWindowOuter;
 class nsPICommandUpdater;
 
 namespace mozilla {
 
 class ComposerCommandsUpdater final : public nsISelectionListener
                                     , public nsIDocumentStateListener
                                     , public nsITransactionListener
                                     , public nsITimerCallback
                                     , public nsINamed
 {
 public:
   ComposerCommandsUpdater();
 
   // nsISupports
-  NS_DECL_ISUPPORTS
+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
+  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(ComposerCommandsUpdater,
+                                           nsIDocumentStateListener)
 
   // nsISelectionListener
   NS_DECL_NSISELECTIONLISTENER
 
   // nsIDocumentStateListener
   NS_DECL_NSIDOCUMENTSTATELISTENER
 
   // nsITimerCallback
@@ -69,19 +73,19 @@ protected:
   nsresult UpdateCommandGroup(const nsAString& aCommandGroup);
 
   already_AddRefed<nsPICommandUpdater> GetCommandUpdater();
 
   nsresult PrimeUpdateTimer();
   void TimerCallback();
 
   nsCOMPtr<nsITimer> mUpdateTimer;
+  nsCOMPtr<nsPIDOMWindowOuter> mDOMWindow;
+  nsCOMPtr<nsIDocShell> mDocShell;
 
-  nsWeakPtr mDOMWindow;
-  nsWeakPtr mDocShell;
   int8_t mDirtyState;
   int8_t mSelectionCollapsed;
   bool mFirstDoOfFirstUndo;
 };
 
 } // namespace mozilla
 
 #endif // #ifndef mozilla_ComposerCommandsUpdater_h
--- a/editor/composer/nsEditingSession.cpp
+++ b/editor/composer/nsEditingSession.cpp
@@ -373,26 +373,26 @@ nsEditingSession::SetupEditorOnWindow(mo
     needHTMLController = true;
   }
 
   if (mInteractive) {
     mEditorFlags |= nsIPlaintextEditor::eEditorAllowInteraction;
   }
 
   // make the UI state maintainer
-  mStateMaintainer = new ComposerCommandsUpdater();
+  mComposerCommandsUpdater = new ComposerCommandsUpdater();
 
   // now init the state maintainer
   // This allows notification of error state
   //  even if we don't create an editor
-  rv = mStateMaintainer->Init(window);
+  rv = mComposerCommandsUpdater->Init(window);
   NS_ENSURE_SUCCESS(rv, rv);
 
   if (mEditorStatus != eEditorCreationInProgress) {
-    mStateMaintainer->NotifyDocumentCreated();
+    mComposerCommandsUpdater->NotifyDocumentCreated();
     return NS_ERROR_FAILURE;
   }
 
   // Create editor and do other things
   //  only if we haven't found some error above,
   nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
   NS_ENSURE_TRUE(docShell, NS_ERROR_FAILURE);
   nsCOMPtr<nsIPresShell> presShell = docShell->GetPresShell();
@@ -450,36 +450,39 @@ nsEditingSession::SetupEditorOnWindow(mo
 
   nsCOMPtr<nsIDOMDocument> domDoc;
   rv = contentViewer->GetDOMDocument(getter_AddRefs(domDoc));
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_TRUE(domDoc, NS_ERROR_FAILURE);
 
   // Set up as a doc state listener
   // Important! We must have this to broadcast the "obs_documentCreated" message
-  rv = htmlEditor->AddDocumentStateListener(mStateMaintainer);
+  rv = htmlEditor->AddDocumentStateListener(mComposerCommandsUpdater);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = htmlEditor->Init(domDoc, nullptr /* root content */,
                         nullptr, mEditorFlags, EmptyString());
   NS_ENSURE_SUCCESS(rv, rv);
 
   RefPtr<Selection> selection = htmlEditor->GetSelection();
   if (NS_WARN_IF(!selection)) {
     return NS_ERROR_FAILURE;
   }
 
-  rv = selection->AddSelectionListener(mStateMaintainer);
-  NS_ENSURE_SUCCESS(rv, rv);
+  rv = selection->AddSelectionListener(mComposerCommandsUpdater);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+  htmlEditor->SetComposerCommandsUpdater(mComposerCommandsUpdater);
 
   // and as a transaction listener
   nsCOMPtr<nsITransactionManager> txnMgr;
   htmlEditor->GetTransactionManager(getter_AddRefs(txnMgr));
   if (txnMgr) {
-    txnMgr->AddListener(mStateMaintainer);
+    txnMgr->AddListener(mComposerCommandsUpdater);
   }
 
   // Set context on all controllers to be the editor
   rv = SetEditorOnControllers(aWindow, htmlEditor);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Everything went fine!
   mEditorStatus = eEditorOK;
@@ -488,32 +491,33 @@ nsEditingSession::SetupEditorOnWindow(mo
   return htmlEditor->PostCreate();
 }
 
 // Removes all listeners and controllers from aWindow and aEditor.
 void
 nsEditingSession::RemoveListenersAndControllers(nsPIDOMWindowOuter* aWindow,
                                                 HTMLEditor* aHTMLEditor)
 {
-  if (!mStateMaintainer || !aHTMLEditor) {
+  if (!mComposerCommandsUpdater || !aHTMLEditor) {
     return;
   }
 
   // Remove all the listeners
   RefPtr<Selection> selection = aHTMLEditor->GetSelection();
   if (selection) {
-    selection->RemoveSelectionListener(mStateMaintainer);
+    selection->RemoveSelectionListener(mComposerCommandsUpdater);
   }
+  aHTMLEditor->SetComposerCommandsUpdater(nullptr);
 
-  aHTMLEditor->RemoveDocumentStateListener(mStateMaintainer);
+  aHTMLEditor->RemoveDocumentStateListener(mComposerCommandsUpdater);
 
   nsCOMPtr<nsITransactionManager> txnMgr;
   aHTMLEditor->GetTransactionManager(getter_AddRefs(txnMgr));
   if (txnMgr) {
-    txnMgr->RemoveListener(mStateMaintainer);
+    txnMgr->RemoveListener(mComposerCommandsUpdater);
   }
 
   // Remove editor controllers from the window now that we're not
   // editing in that window any more.
   RemoveEditorControllers(aWindow);
 }
 
 /*---------------------------------------------------------------------------
@@ -552,17 +556,17 @@ nsEditingSession::TearDownEditorOnWindow
   nsCOMPtr<nsIDocShell> docShell = window->GetDocShell();
   NS_ENSURE_STATE(docShell);
 
   RefPtr<HTMLEditor> htmlEditor = docShell->GetHTMLEditor();
   if (stopEditing) {
     htmlDoc->TearingDownEditor();
   }
 
-  if (mStateMaintainer && htmlEditor) {
+  if (mComposerCommandsUpdater && htmlEditor) {
     // Null out the editor on the controllers first to prevent their weak
     // references from pointing to a destroyed editor.
     SetEditorOnControllers(aWindow, nullptr);
   }
 
   // Null out the editor on the docShell to trigger PreDestroy which
   // needs to happen before document state listeners are removed below.
   docShell->SetEditor(nullptr);
@@ -1278,17 +1282,18 @@ nsEditingSession::RestoreAnimationMode(n
   presContext->SetImageAnimationMode(mImageAnimationMode);
 }
 
 nsresult
 nsEditingSession::DetachFromWindow(mozIDOMWindowProxy* aWindow)
 {
   NS_ENSURE_TRUE(mDoneSetup, NS_OK);
 
-  NS_ASSERTION(mStateMaintainer, "mStateMaintainer should exist.");
+  NS_ASSERTION(mComposerCommandsUpdater,
+               "mComposerCommandsUpdater should exist.");
 
   // Kill any existing reload timer
   if (mLoadBlankDocTimer) {
     mLoadBlankDocTimer->Cancel();
     mLoadBlankDocTimer = nullptr;
   }
 
   auto* window = nsPIDOMWindowOuter::From(aWindow);
@@ -1308,17 +1313,18 @@ nsEditingSession::DetachFromWindow(mozID
 }
 
 nsresult
 nsEditingSession::ReattachToWindow(mozIDOMWindowProxy* aWindow)
 {
   NS_ENSURE_TRUE(mDoneSetup, NS_OK);
   NS_ENSURE_TRUE(aWindow, NS_ERROR_FAILURE);
 
-  NS_ASSERTION(mStateMaintainer, "mStateMaintainer should exist.");
+  NS_ASSERTION(mComposerCommandsUpdater,
+               "mComposerCommandsUpdater should exist.");
 
   // Imitate nsEditorDocShell::MakeEditable() to reattach the
   // old editor ot the window.
   nsresult rv;
 
   auto* window = nsPIDOMWindowOuter::From(aWindow);
   nsIDocShell *docShell = window->GetDocShell();
   NS_ENSURE_TRUE(docShell, NS_ERROR_FAILURE);
@@ -1345,18 +1351,18 @@ nsEditingSession::ReattachToWindow(mozID
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = SetupEditorCommandController("@mozilla.org/editor/editordocstatecontroller;1",
                                     aWindow,
                                     static_cast<nsIEditingSession*>(this),
                                     &mDocStateControllerId);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  if (mStateMaintainer) {
-    mStateMaintainer->Init(window);
+  if (mComposerCommandsUpdater) {
+    mComposerCommandsUpdater->Init(window);
   }
 
   // Get editor
   RefPtr<HTMLEditor> htmlEditor = GetHTMLEditorForWindow(aWindow);
   if (NS_WARN_IF(!htmlEditor)) {
     return NS_ERROR_FAILURE;
   }
 
--- a/editor/composer/nsEditingSession.h
+++ b/editor/composer/nsEditingSession.h
@@ -125,17 +125,17 @@ protected:
 
   bool            mProgressListenerRegistered;
 
   // The image animation mode before it was turned off.
   uint16_t        mImageAnimationMode;
 
   // THE REMAINING MEMBER VARIABLES WILL BECOME A SET WHEN WE EDIT
   // MORE THAN ONE EDITOR PER EDITING SESSION
-  RefPtr<mozilla::ComposerCommandsUpdater> mStateMaintainer;
+  RefPtr<mozilla::ComposerCommandsUpdater> mComposerCommandsUpdater;
 
   // Save the editor type so we can create the editor after loading uri
   nsCString       mEditorType;
   uint32_t        mEditorFlags;
   uint32_t        mEditorStatus;
   uint32_t        mBaseCommandControllerId;
   uint32_t        mDocStateControllerId;
   uint32_t        mHTMLCommandControllerId;
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -1,15 +1,16 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "mozilla/HTMLEditor.h"
 
+#include "mozilla/ComposerCommandsUpdater.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/EditAction.h"
 #include "mozilla/EditorDOMPoint.h"
 #include "mozilla/EventStates.h"
 #include "mozilla/mozInlineSpellChecker.h"
 #include "mozilla/TextEvents.h"
 
 #include "nsCRT.h"
@@ -185,25 +186,27 @@ HTMLEditor::HideAnonymousEditingUIs()
     HideResizers();
   }
 }
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(HTMLEditor)
 
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(HTMLEditor, TextEditor)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mTypeInState)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mComposerCommandsUpdater)
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mStyleSheets)
 
   tmp->HideAnonymousEditingUIs();
 
   NS_IMPL_CYCLE_COLLECTION_UNLINK(mLinkHandler)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(HTMLEditor, TextEditor)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTypeInState)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mComposerCommandsUpdater)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mStyleSheets)
 
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTopLeftHandle)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTopHandle)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTopRightHandle)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLeftHandle)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRightHandle)
   NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mBottomLeftHandle)
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -2,16 +2,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_HTMLEditor_h
 #define mozilla_HTMLEditor_h
 
 #include "mozilla/Attributes.h"
+#include "mozilla/ComposerCommandsUpdater.h"
 #include "mozilla/CSSEditUtils.h"
 #include "mozilla/ManualNAC.h"
 #include "mozilla/StyleSheet.h"
 #include "mozilla/TextEditor.h"
 #include "mozilla/TextEditRules.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/dom/Element.h"
 #include "mozilla/dom/File.h"
@@ -249,16 +250,30 @@ public:
                                           nsAtom* aAttr,
                                           const nsAString& aValue,
                                           bool* aFirst,
                                           bool* aAny,
                                           bool* aAll,
                                           nsAString& outValue);
   nsresult RemoveInlineProperty(nsAtom* aProperty,
                                 nsAtom* aAttribute);
+
+  /**
+   * SetComposerCommandsUpdater() sets or unsets mComposerCommandsUpdater.
+   * This will crash in debug build if the editor already has an instance
+   * but called with another instance.
+   */
+  void SetComposerCommandsUpdater(
+         ComposerCommandsUpdater* aComposerCommandsUpdater)
+  {
+    MOZ_ASSERT(!aComposerCommandsUpdater || !mComposerCommandsUpdater ||
+               aComposerCommandsUpdater == mComposerCommandsUpdater);
+    mComposerCommandsUpdater = aComposerCommandsUpdater;
+  }
+
 protected:
   virtual ~HTMLEditor();
 
   using EditorBase::IsBlockNode;
   virtual bool IsBlockNode(nsINode *aNode) override;
 
 public:
   // XXX Why don't we move following methods above for grouping by the origins?
@@ -968,16 +983,17 @@ protected:
    *                    insure we reset the caret in a table-editing method.
    */
   void SetSelectionAfterTableEdit(nsIDOMElement* aTable,
                                   int32_t aRow, int32_t aCol,
                                   int32_t aDirection, bool aSelected);
 
 protected:
   RefPtr<TypeInState> mTypeInState;
+  RefPtr<ComposerCommandsUpdater> mComposerCommandsUpdater;
 
   bool mCRInParagraphCreatesParagraph;
 
   bool mCSSAware;
   UniquePtr<CSSEditUtils> mCSSEditUtils;
 
   // Used by GetFirstSelectedCell and GetNextSelectedCell
   int32_t  mSelectedCellIndex;