Bug 1484110 - part 1: Create HTMLEditor::RefereshEditingUI() for internal use of nsIHTMLEditor::CheckSelectionStateForAnonymousButtons() r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 17 Aug 2018 17:56:28 +0900
changeset 490310 08d0e0c32a53307e3f75645206000244375c4314
parent 490309 5f3c40ffdfedafcf6476694d148a5c6c7601db73
child 490311 6a6a79e2c19b5b08ffcef13fc0741f98ddcecf19
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1484110
milestone63.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 1484110 - part 1: Create HTMLEditor::RefereshEditingUI() for internal use of nsIHTMLEditor::CheckSelectionStateForAnonymousButtons() r=m_kato HTMLEditor::CheckSelectionStateForAnonymousButtons() is called a lot internally. Especially, its virtual call cost may make damage to our performance since it's called from a selection listener. So, we should create non-virtual method, RefereshEditingUI() for internal use.
editor/libeditor/EditorBase.cpp
editor/libeditor/HTMLAbsPositionEditor.cpp
editor/libeditor/HTMLAnonymousNodeEditor.cpp
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditor.h
editor/nsIHTMLEditor.idl
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -4195,20 +4195,18 @@ EditorBase::EndUpdateViewBatch()
   // mutation events listeners because all the changes the user can apply
   // to a document may result in multiple events, some of them quite hard
   // to listen too (in particular when an ancestor of the selection is
   // changed but the selection itself is not changed).
   if (NS_WARN_IF(!selection)) {
     return;
   }
 
-  DebugOnly<nsresult> rv =
-    htmlEditor->CheckSelectionStateForAnonymousButtons(selection);
-  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
-    "CheckSelectionStateForAnonymousButtons() failed");
+  DebugOnly<nsresult> rv = htmlEditor->RefereshEditingUI(*selection);
+  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "RefereshEditingUI() failed");
 }
 
 TextComposition*
 EditorBase::GetComposition() const
 {
   return mComposition;
 }
 
--- a/editor/libeditor/HTMLAbsPositionEditor.cpp
+++ b/editor/libeditor/HTMLAbsPositionEditor.cpp
@@ -413,17 +413,21 @@ HTMLEditor::EndMoving()
   mMouseMotionListenerP = nullptr;
 
   mGrabberClicked = false;
   mIsMoving = false;
   RefPtr<Selection> selection = GetSelection();
   if (!selection) {
     return NS_ERROR_NOT_INITIALIZED;
   }
-  return CheckSelectionStateForAnonymousButtons(selection);
+  nsresult rv = RefereshEditingUI(*selection);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+  return NS_OK;
 }
 nsresult
 HTMLEditor::SetFinalPosition(int32_t aX,
                              int32_t aY)
 {
   nsresult rv = EndMoving();
   NS_ENSURE_SUCCESS(rv, rv);
 
--- a/editor/libeditor/HTMLAnonymousNodeEditor.cpp
+++ b/editor/libeditor/HTMLAnonymousNodeEditor.cpp
@@ -277,41 +277,46 @@ HTMLEditor::DeleteRefToAnonymousNode(Man
     // FIXME(emilio): This is the only caller to PresShell::ContentRemoved that
     // passes NAC into it. This is not great!
     aShell->ContentRemoved(aContent, nullptr);
   }
 
   // The ManualNACPtr destructor will invoke UnbindFromTree.
 }
 
-// The following method is mostly called by a selection listener. When a
-// selection change is notified, the method is called to check if resizing
-// handles, a grabber and/or inline table editing UI need to be displayed
-// or refreshed
 NS_IMETHODIMP
 HTMLEditor::CheckSelectionStateForAnonymousButtons(Selection* aSelection)
 {
   if (NS_WARN_IF(!aSelection)) {
     return NS_ERROR_INVALID_ARG;
   }
+  nsresult rv = RefereshEditingUI(*aSelection);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+  return NS_OK;
+}
 
+nsresult
+HTMLEditor::RefereshEditingUI(Selection& aSelection)
+{
   // early way out if all contextual UI extensions are disabled
-  if (NS_WARN_IF(!IsObjectResizerEnabled() &&
-                 !IsAbsolutePositionEditorEnabled() &&
-                 !IsInlineTableEditorEnabled())) {
+  if (!IsObjectResizerEnabled() &&
+      !IsAbsolutePositionEditorEnabled() &&
+      !IsInlineTableEditorEnabled()) {
     return NS_OK;
   }
 
   // Don't change selection state if we're moving.
   if (mIsMoving) {
     return NS_OK;
   }
 
   // let's get the containing element of the selection
-  RefPtr<Element> focusElement = GetSelectionContainerElement(*aSelection);
+  RefPtr<Element> focusElement = GetSelectionContainerElement(aSelection);
   if (NS_WARN_IF(!focusElement)) {
     return NS_OK;
   }
 
   // If we're not in a document, don't try to add resizers
   if (!focusElement->IsInUncomposedDoc()) {
     return NS_OK;
   }
@@ -326,27 +331,30 @@ HTMLEditor::CheckSelectionStateForAnonym
     absPosElement = GetAbsolutelyPositionedSelectionContainer();
   }
 
   RefPtr<Element> cellElement;
   if (IsObjectResizerEnabled() || IsInlineTableEditorEnabled()) {
     // Resizing or Inline Table Editing is enabled, we need to check if the
     // selection is contained in a table cell
     cellElement =
-      GetElementOrParentByTagNameAtSelection(*aSelection, *nsGkAtoms::td);
+      GetElementOrParentByTagNameAtSelection(aSelection, *nsGkAtoms::td);
   }
 
   if (IsObjectResizerEnabled() && cellElement) {
     // we are here because Resizing is enabled AND selection is contained in
     // a cell
 
     // get the enclosing table
     if (nsGkAtoms::img != focusTagAtom) {
       // the element container of the selection is not an image, so we'll show
       // the resizers around the table
+      // XXX There may be a bug.  cellElement may be not in <table> in invalid
+      //     tree.  So, perhaps, GetEnclosingTable() returns nullptr, we should
+      //     not set focusTagAtom to nsGkAtoms::table.
       focusElement = GetEnclosingTable(cellElement);
       focusTagAtom = nsGkAtoms::table;
     }
   }
 
   // we allow resizers only around images, tables, and absolutely positioned
   // elements. If we don't have image/table, let's look at the latter case.
   if (nsGkAtoms::img != focusTagAtom && nsGkAtoms::table != focusTagAtom) {
@@ -364,25 +372,34 @@ HTMLEditor::CheckSelectionStateForAnonym
   if (IsAbsolutePositionEditorEnabled() && mAbsolutelyPositionedObject &&
       absPosElement != mAbsolutelyPositionedObject) {
     HideGrabber();
     NS_ASSERTION(!mAbsolutelyPositionedObject, "HideGrabber failed");
   }
 
   if (IsObjectResizerEnabled() && mResizedObject &&
       mResizedObject != focusElement) {
+    // Perhaps, even if HideResizers() failed, we should try to hide inline
+    // table editing UI.  However, it returns error only when we cannot do
+    // anything.  So, it's okay for now.
     nsresult rv = HideResizers();
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
     NS_ASSERTION(!mResizedObject, "HideResizers failed");
   }
 
   if (mIsInlineTableEditingEnabled && mInlineEditedCell &&
       mInlineEditedCell != cellElement) {
+    // XXX HideInlineTableEditingUI() won't return error.  Should be change it
+    //     void later.
     nsresult rv = HideInlineTableEditingUI();
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
     NS_ASSERTION(!mInlineEditedCell, "HideInlineTableEditingUI failed");
   }
 
   // now, let's display all contextual UI for good
   nsIContent* hostContent = GetActiveEditingHost();
 
   if (IsObjectResizerEnabled() && focusElement &&
       IsModifiableNode(*focusElement) && focusElement != hostContent) {
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -429,28 +429,29 @@ HTMLEditor::NotifySelectionChanged(nsIDo
     return NS_ERROR_INVALID_ARG;
   }
 
   if (mTypeInState) {
     RefPtr<TypeInState> typeInState = mTypeInState;
     typeInState->OnSelectionChange(*aSelection);
 
     // We used a class which derived from nsISelectionListener to call
-    // HTMLEditor::CheckSelectionStateForAnonymousButtons().  The lifetime of
-    // the class was exactly same as mTypeInState.  So, call it only when
-    // mTypeInState is not nullptr.
+    // HTMLEditor::RefereshEditingUI().  The lifetime of the class was
+    // exactly same as mTypeInState.  So, call it only when mTypeInState
+    // is not nullptr.
     if ((aReason & (nsISelectionListener::MOUSEDOWN_REASON |
                     nsISelectionListener::KEYPRESS_REASON |
                     nsISelectionListener::SELECTALL_REASON)) && aSelection) {
       // the selection changed and we need to check if we have to
       // hide and/or redisplay resizing handles
       // FYI: This is an XPCOM method.  So, the caller, Selection, guarantees
       //      the lifetime of this instance.  So, don't need to grab this with
       //      local variable.
-      CheckSelectionStateForAnonymousButtons(aSelection);
+      DebugOnly<nsresult> rv = RefereshEditingUI(*aSelection);
+      NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "RefereshEditingUI() failed");
     }
   }
 
   if (mComposerCommandsUpdater) {
     RefPtr<ComposerCommandsUpdater> updater = mComposerCommandsUpdater;
     updater->OnSelectionChange();
   }
 
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -1505,16 +1505,23 @@ protected: // Shouldn't be used by frien
   void RemoveListenerAndDeleteRef(const nsAString& aEvent,
                                   nsIDOMEventListener* aListener,
                                   bool aUseCapture,
                                   ManualNACPtr aElement,
                                   nsIPresShell* aShell);
   void DeleteRefToAnonymousNode(ManualNACPtr aContent,
                                 nsIPresShell* aShell);
 
+  /**
+   * RefereshEditingUI() may refresh editing UIs for current Selection, focus,
+   * etc.  If this shows or hides some UIs, it causes reflow.  So, this is
+   * not safe method.
+   */
+  nsresult RefereshEditingUI(Selection& aSelection);
+
   nsresult ShowResizersInner(Element& aResizedElement);
 
   /**
    * Returns the offset of an element's frame to its absolute containing block.
    */
   nsresult GetElementOrigin(Element& aElement,
                             int32_t& aX, int32_t& aY);
   nsresult GetPositionAndDimensions(Element& aElement,
--- a/editor/nsIHTMLEditor.idl
+++ b/editor/nsIHTMLEditor.idl
@@ -392,19 +392,24 @@ interface nsIHTMLEditor : nsISupports
    * A boolean which is true is the HTMLEditor has been instantiated
    * with CSS knowledge and if the CSS pref is currently checked
    *
    * @return    true if CSS handled and enabled
    */
   attribute boolean isCSSEnabled;
 
   /**
-   * Checks if the anonymous nodes created by the HTML editor have to be
-   * refreshed or hidden depending on a possible new state of the selection
-   * @param aSelection [IN] a selection
+   * checkSelectionStateForAnonymousButtons() may refresh editing UI such as
+   * resizers, inline-table-editing UI, absolute positioning UI for current
+   * Selection and focus state.  When this method shows or hides UI, the
+   * editor (and/or its document/window) could be broken by mutation observers.
+   * FYI: Current user in script is only BlueGriffon.
+   *
+   * @param aSelection  Selection instance for the normal selection of the
+   *                    document.
    */
   void checkSelectionStateForAnonymousButtons(in Selection aSelection);
 
   boolean isAnonymousElement(in Element aElement);
 
   /**
    * A boolean indicating if a return key pressed in a paragraph creates
    * another paragraph or just inserts a <br> at the caret