Bug 1482019 - part 4: Reduce the indent level of the last block in HTMLEditor::GetSelectedNode() r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 10 Aug 2018 17:46:33 +0900
changeset 486973 66e9a30a6b1a192e6afdb9bae386243cec9a2127
parent 486972 f6053ad1d49aa8924c4ede25f0247c9c2a86d607
child 486974 57e568a7f9334e4b0610199a3fec7fce79e786bb
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1482019
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 1482019 - part 4: Reduce the indent level of the last block in HTMLEditor::GetSelectedNode() r=m_kato The first check of the last block of HTMLEditor::GetSelectedNode() can use early-return style. Additionally, |current| is not necessary since Selection is not changed since the method retrieved first range. So, we can get rid of |current| and the |nullptr| case of it.
editor/libeditor/HTMLEditor.cpp
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -2655,27 +2655,27 @@ HTMLEditor::GetSelectedNode(Selection& a
                             const nsAtom* aTagName,
                             ErrorResult& aRv)
 {
   MOZ_ASSERT(!aRv.Failed());
 
   bool isLinkTag = aTagName && IsLinkTag(*aTagName);
   bool isNamedAnchorTag = aTagName && IsNamedAnchorTag(*aTagName);
 
-  RefPtr<nsRange> range = aSelection.GetRangeAt(0);
-  if (NS_WARN_IF(!range)) {
+  RefPtr<nsRange> firstRange = aSelection.GetRangeAt(0);
+  if (NS_WARN_IF(!firstRange)) {
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
 
-  nsCOMPtr<nsINode> startContainer = range->GetStartContainer();
-  nsIContent* startNode = range->GetChildAtStartOffset();
-
-  nsCOMPtr<nsINode> endContainer = range->GetEndContainer();
-  nsIContent* endNode = range->GetChildAtEndOffset();
+  nsCOMPtr<nsINode> startContainer = firstRange->GetStartContainer();
+  nsIContent* startNode = firstRange->GetChildAtStartOffset();
+
+  nsCOMPtr<nsINode> endContainer = firstRange->GetEndContainer();
+  nsIContent* endNode = firstRange->GetChildAtEndOffset();
 
   // Optimization for a single selected element
   if (startContainer && startContainer == endContainer &&
       startNode && endNode && startNode->GetNextSibling() == endNode) {
     nsCOMPtr<nsINode> selectedNode = startNode;
     if (selectedNode) {
       // Test for appropriate node type requested
       if (!aTagName || aTagName == selectedNode->NodeInfo()->NameAtom() ||
@@ -2720,76 +2720,84 @@ HTMLEditor::GetSelectedNode(Selection& a
             focus.GetChildAtOffset() ==
               anchor.GetChildAtOffset()->GetNextSibling()) {
           selectedElement = Element::FromNodeOrNull(anchor.GetChildAtOffset());
         }
       }
     }
   }
 
-  if (!aSelection.IsCollapsed()) {
-    RefPtr<nsRange> currange = aSelection.GetRangeAt(0);
-    if (currange) {
-      nsresult rv;
-      nsCOMPtr<nsIContentIterator> iter =
-        do_CreateInstance("@mozilla.org/content/post-content-iterator;1",
-                          &rv);
-      if (NS_WARN_IF(NS_FAILED(rv))) {
-        aRv.Throw(rv);
-        return nullptr;
+  if (aSelection.IsCollapsed()) {
+    return selectedElement.forget();
+  }
+
+  nsresult rv;
+  nsCOMPtr<nsIContentIterator> iter =
+    do_CreateInstance("@mozilla.org/content/post-content-iterator;1",
+                      &rv);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    aRv.Throw(rv);
+    return nullptr;
+  }
+
+  bool found = !!selectedElement;
+  const nsAtom* tagNameLookingFor = aTagName;
+  iter->Init(firstRange);
+  // loop through the content iterator for each content node
+  while (!iter->IsDone()) {
+    // Update selectedElement with new node.  If it's not an element node,
+    // clear it.
+    // XXX This is really odd since this means that the result depends on
+    //     what is the last node.  If the last node is an element node,
+    //     it may be returned even if it does not match with aTagName.
+    //     On the other hand, if last node is not an element, i.e., we have
+    //     not found proper element node, we return nullptr as this method
+    //     name explains.
+    selectedElement = Element::FromNodeOrNull(iter->GetCurrentNode());
+    if (selectedElement) {
+      // If we already found a node, then we have another element,
+      // thus there's not just one element selected.
+      // XXX Really odd.  The new element node may be different name element.
+      //     So, this means that we return any next element node if we find
+      //     proper element as first element in the range.
+      if (found) {
+        break;
       }
 
-      bool found = !!selectedElement;
-      const nsAtom* tagNameLookingFor = aTagName;
-      iter->Init(currange);
-      // loop through the content iterator for each content node
-      while (!iter->IsDone()) {
-        // Query interface to cast nsIContent to Element
-        //  then get tagType to compare to  aTagName
-        // Clone node of each desired type and append it to the aDomFrag
-        selectedElement = Element::FromNodeOrNull(iter->GetCurrentNode());
-        if (selectedElement) {
-          // If we already found a node, then we have another element,
-          //  thus there's not just one element selected
-          if (found) {
-            break;
-          }
-
-          if (!tagNameLookingFor) {
-            // Get name of first selected element
-            tagNameLookingFor = selectedElement->NodeInfo()->NameAtom();
-          }
-
-          // The "A" tag is a pain,
-          //  used for both link(href is set) and "Named Anchor"
-          if ((isLinkTag &&
-               HTMLEditUtils::IsLink(selectedElement)) ||
-              (isNamedAnchorTag &&
-               HTMLEditUtils::IsNamedAnchor(selectedElement))) {
-            found = true;
-          }
-          // All other tag names are handled here.
-          else if (tagNameLookingFor ==
-                     selectedElement->NodeInfo()->NameAtom()) {
-            found = true;
-          }
-
-          if (!found) {
-            // Check if node we have is really part of the selection???
-            break;
-          }
-        }
-        iter->Next();
+      if (!tagNameLookingFor) {
+        // Get name of first selected element
+        // XXX Looks like that this is necessary only for making the following
+        //     handler work as expected...  Why don't you check this below??
+        tagNameLookingFor = selectedElement->NodeInfo()->NameAtom();
+      }
+
+      // The "A" tag is a pain,
+      //  used for both link(href is set) and "Named Anchor"
+      if ((isLinkTag &&
+           HTMLEditUtils::IsLink(selectedElement)) ||
+          (isNamedAnchorTag &&
+           HTMLEditUtils::IsNamedAnchor(selectedElement))) {
+        found = true;
       }
-    } else {
-      // Should never get here?
-      NS_WARNING("isCollapsed was FALSE, but no elements found in selection\n");
+      // All other tag names are handled here.
+      else if (tagNameLookingFor ==
+                 selectedElement->NodeInfo()->NameAtom()) {
+        found = true;
+      }
+
+      if (!found) {
+        // Check if node we have is really part of the selection???
+        // XXX This is odd.  This means that we return element node whose
+        //     tag name does not match with aTagName if we find such element
+        //     node first.
+        break;
+      }
     }
-  }
-
+    iter->Next();
+  }
   return selectedElement.forget();
 }
 
 already_AddRefed<Element>
 HTMLEditor::GetSelectedElement(const nsAString& aTagName)
 {
   nsCOMPtr<nsISupports> domElement;
   GetSelectedElement(aTagName, getter_AddRefs(domElement));