Bug 1483144 - Make HTMLEditor::GetSelectionContainer() protected r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 16 Aug 2018 15:12:51 +0000
changeset 432045 a28cf4300f126a9ae4960035c991d9280c9830d6
parent 432044 fd90e385d65bc4eeb960adaae509458504bbead8
child 432046 d301e82323de6816535b01ba5d2990e2ac57eb5a
push id34457
push userebalazs@mozilla.com
push dateFri, 17 Aug 2018 09:45:26 +0000
treeherdermozilla-central@d2ba1d6c76f2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1483144
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 1483144 - Make HTMLEditor::GetSelectionContainer() protected r=m_kato HTMLEditor::GetSelectionContainer() is a public method, but it's not used by outer classes. So, we can make it a protected member. Additionally, this patch cleans up the method. - Renames to GetSelectionContainerElement() for making clearer what will be returned. - Makes it const. - Makes it take Selection reference since most callers already have Selection. - Makes it use RangeBoundary to access start point and end point of range since nsRange::StartOffset() and nsRange::EndOffset() may be slow. - Makes it not use GetSelectedElement() since it requires unnecessary additional cost and the condition to call it means it uses only the first path in GetSelectedElement() which just returns start node of the range. - Makes it output warning when it returns nullptr since it reaches nullptr only when illegal cases, e.g., Selection is in orphan node. Differential Revision: https://phabricator.services.mozilla.com/D3461
editor/libeditor/HTMLAbsPositionEditor.cpp
editor/libeditor/HTMLAnonymousNodeEditor.cpp
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditor.h
--- a/editor/libeditor/HTMLAbsPositionEditor.cpp
+++ b/editor/libeditor/HTMLAbsPositionEditor.cpp
@@ -75,19 +75,27 @@ HTMLEditor::SetSelectionToAbsoluteOrStat
   }
 
   return rules->DidDoAction(selection, subActionInfo, rv);
 }
 
 already_AddRefed<Element>
 HTMLEditor::GetAbsolutelyPositionedSelectionContainer()
 {
+  RefPtr<Selection> selection = GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return nullptr;
+  }
+
+  RefPtr<Element> element = GetSelectionContainerElement(*selection);
+  if (NS_WARN_IF(!element)) {
+    return nullptr;
+  }
+
   nsAutoString positionStr;
-  RefPtr<Element> element = GetSelectionContainer();
-
   while (element && !element->IsHTMLElement(nsGkAtoms::html)) {
     nsresult rv =
       CSSEditUtils::GetComputedProperty(*element, *nsGkAtoms::position,
                                         positionStr);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return nullptr;
     }
     if (positionStr.EqualsLiteral("absolute")) {
--- a/editor/libeditor/HTMLAnonymousNodeEditor.cpp
+++ b/editor/libeditor/HTMLAnonymousNodeEditor.cpp
@@ -297,18 +297,20 @@ HTMLEditor::CheckSelectionStateForAnonym
       mIsInlineTableEditingEnabled, 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 = GetSelectionContainer();
-  NS_ENSURE_TRUE(focusElement, NS_OK);
+  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;
   }
 
   // what's its tag?
   nsAtom* focusTagAtom = focusElement->NodeInfo()->NameAtom();
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -10920,17 +10920,18 @@ HTMLEditRules::WillAbsolutePosition(bool
   if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) {
     return NS_ERROR_EDITOR_DESTROYED;
   }
   NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "WillInsert() failed");
 
   *aCancel = false;
   *aHandled = true;
 
-  RefPtr<Element> focusElement = HTMLEditorRef().GetSelectionContainer();
+  RefPtr<Element> focusElement =
+    HTMLEditorRef().GetSelectionContainerElement(SelectionRef());
   if (focusElement && HTMLEditUtils::IsImage(focusElement)) {
     mNewBlock = focusElement;
     return NS_OK;
   }
 
   rv = NormalizeSelection();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -4722,71 +4722,84 @@ HTMLEditor::GetElementOrigin(Element& aE
   nsPoint off = frame->GetOffsetTo(container);
   aX = nsPresContext::AppUnitsToIntCSSPixels(off.x);
   aY = nsPresContext::AppUnitsToIntCSSPixels(off.y);
 
   return NS_OK;
 }
 
 Element*
-HTMLEditor::GetSelectionContainer()
+HTMLEditor::GetSelectionContainerElement(Selection& aSelection) const
 {
-  // If we don't get the selection, just skip this
-  NS_ENSURE_TRUE(GetSelection(), nullptr);
-
-  OwningNonNull<Selection> selection = *GetSelection();
-
-  nsCOMPtr<nsINode> focusNode;
-
-  if (selection->IsCollapsed()) {
-    focusNode = selection->GetFocusNode();
+  nsINode* focusNode = nullptr;
+  if (aSelection.IsCollapsed()) {
+    focusNode = aSelection.GetFocusNode();
+    if (NS_WARN_IF(!focusNode)) {
+      return nullptr;
+    }
   } else {
-    int32_t rangeCount = selection->RangeCount();
+    uint32_t rangeCount = aSelection.RangeCount();
+    MOZ_ASSERT(rangeCount, "If 0, Selection::IsCollapsed() should return true");
 
     if (rangeCount == 1) {
-      RefPtr<nsRange> range = selection->GetRangeAt(0);
-
-      nsCOMPtr<nsINode> startContainer = range->GetStartContainer();
-      int32_t startOffset = range->StartOffset();
-      nsCOMPtr<nsINode> endContainer = range->GetEndContainer();
-      int32_t endOffset = range->EndOffset();
-
-      if (startContainer == endContainer && startOffset + 1 == endOffset) {
-        MOZ_ASSERT(!focusNode, "How did it get set already?");
-        IgnoredErrorResult error;
-        focusNode = GetSelectedElement(*selection, nullptr, error);
-        NS_WARNING_ASSERTION(!error.Failed(), "Failed to get selected element");
-      }
-      if (!focusNode) {
+      nsRange* range = aSelection.GetRangeAt(0);
+
+      const RangeBoundary& startRef = range->StartRef();
+      const RangeBoundary& endRef = range->EndRef();
+
+      // This method called GetSelectedElement() to retrieve proper container
+      // when only one node is selected.  However, it simply returns start
+      // node of Selection with additional cost.  So, we do not need to call
+      // it anymore.
+      if (startRef.Container()->IsElement() &&
+          startRef.Container() == endRef.Container() &&
+          startRef.GetChildAtOffset() &&
+          startRef.GetChildAtOffset()->GetNextSibling() ==
+            endRef.GetChildAtOffset()) {
+        focusNode = startRef.GetChildAtOffset();
+        MOZ_ASSERT(focusNode, "Start container must not be nullptr");
+      } else {
         focusNode = range->GetCommonAncestor();
+        if (NS_WARN_IF(!focusNode)) {
+          return nullptr;
+        }
       }
     } else {
-      for (int32_t i = 0; i < rangeCount; i++) {
-        RefPtr<nsRange> range = selection->GetRangeAt(i);
-
-        nsCOMPtr<nsINode> startContainer = range->GetStartContainer();
+      for (uint32_t i = 0; i < rangeCount; i++) {
+        nsRange* range = aSelection.GetRangeAt(i);
+        nsINode* startContainer = range->GetStartContainer();
         if (!focusNode) {
           focusNode = startContainer;
         } else if (focusNode != startContainer) {
+          // XXX Looks odd to use parent of startContainer because previous
+          //     range may not be in the parent node of current startContainer.
           focusNode = startContainer->GetParentNode();
+          // XXX Looks odd to break the for-loop here because we refer only
+          //     first range and another range which starts from different
+          //     container, and the latter range is preferred. Why?
           break;
         }
       }
+      if (NS_WARN_IF(!focusNode)) {
+        return nullptr;
+      }
     }
   }
 
-  if (focusNode && focusNode->GetAsText()) {
+  if (focusNode->GetAsText()) {
     focusNode = focusNode->GetParentNode();
-  }
-
-  if (focusNode && focusNode->IsElement()) {
-    return focusNode->AsElement();
-  }
-
-  return nullptr;
+    if (NS_WARN_IF(!focusNode)) {
+      return nullptr;
+    }
+  }
+
+  if (NS_WARN_IF(!focusNode->IsElement())) {
+    return nullptr;
+  }
+  return focusNode->AsElement();
 }
 
 NS_IMETHODIMP
 HTMLEditor::IsAnonymousElement(Element* aElement,
                                bool* aReturn)
 {
   NS_ENSURE_TRUE(aElement, NS_ERROR_NULL_POINTER);
   *aReturn = aElement->IsRootOfNativeAnonymousSubtree();
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -119,21 +119,16 @@ public:
   HTMLEditor();
 
   nsHTMLDocument* GetHTMLDocument() const;
 
   virtual void PreDestroy(bool aDestroyingFrames) override;
 
   bool GetReturnInParagraphCreatesNewParagraph();
 
-  /**
-   * Returns the deepest container of the selection
-   */
-  Element* GetSelectionContainer();
-
   // TextEditor overrides
   virtual nsresult Init(nsIDocument& aDoc, Element* aRoot,
                         nsISelectionController* aSelCon, uint32_t aFlags,
                         const nsAString& aValue) override;
   NS_IMETHOD BeginningOfDocument() override;
   NS_IMETHOD SetFlags(uint32_t aFlags) override;
 
   NS_IMETHOD CanPaste(int32_t aSelectionType, bool* aCanPaste) override;
@@ -440,16 +435,27 @@ protected: // May be called by friends.
   /**
    * GetBlock() returns aNode itself, or parent or nearest ancestor of aNode
    * if there is a block parent.  If aAncestorLimiter is not nullptr,
    * this stops looking for the result.
    */
   static Element* GetBlock(nsINode& aNode,
                            nsINode* aAncestorLimiter = nullptr);
 
+  /**
+   * Returns container element of ranges in Selection.  If Selection is
+   * collapsed, returns focus container node (or its parent element).
+   * If Selection selects only one element node, returns the element node.
+   * If Selection is only one range, returns common ancestor of the range.
+   * XXX If there are two or more Selection ranges, this returns parent node
+   *     of start container of a range which starts with different node from
+   *     start container of the first range.
+   */
+  Element* GetSelectionContainerElement(Selection& aSelection) const;
+
   void IsNextCharInNodeWhitespace(nsIContent* aContent,
                                   int32_t aOffset,
                                   bool* outIsSpace,
                                   bool* outIsNBSP,
                                   nsIContent** outNode = nullptr,
                                   int32_t* outOffset = 0);
   void IsPrevCharInNodeWhitespace(nsIContent* aContent,
                                   int32_t aOffset,