Bug 1588688 - part 7-6: Clean up `HTMLEditor::DiscoverPartialListsAndTables()` r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 13 Feb 2020 08:42:22 +0000
changeset 514161 0809a778c694a05a11cfd999bb508e386cb04fee
parent 514160 4b0bed287f1a11fd828d76413a6fa4a82f67c46d
child 514162 7f6c84d555b3fd2cd56aa27117505853e65f57e0
push id37125
push usershindli@mozilla.com
push dateSat, 15 Feb 2020 09:56:17 +0000
treeherdermozilla-central@02b1aa498dd2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1588688
milestone75.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 1588688 - part 7-6: Clean up `HTMLEditor::DiscoverPartialListsAndTables()` r=m_kato `HTMLEditor::DiscoverPartialListsAndTables()` can be static and we can make its definition simpler. However, due to the odd logic, I'm still not sure what it does. I'll update it after cleaning up its result user, `HTMLEditor::ReplaceOrphanedStructure()`. Differential Revision: https://phabricator.services.mozilla.com/D61979
editor/libeditor/HTMLEditor.h
editor/libeditor/HTMLEditorDataTransfer.cpp
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -3958,24 +3958,28 @@ class HTMLEditor final : public TextEdit
    * CollectListAndTableRelatedElementsAt() collects list elements and
    * table related elements from aNode (meaning aNode may be in the first of
    * the result) to the root element.
    */
   static void CollectListAndTableRelatedElementsAt(
       nsINode& aNode,
       nsTArray<OwningNonNull<Element>>& aOutArrayOfListAndTableElements);
 
-  int32_t DiscoverPartialListsAndTables(
-      nsTArray<OwningNonNull<nsINode>>& aPasteNodes,
-      nsTArray<OwningNonNull<Element>>& aListsAndTables);
+  /**
+   * TODO: Document what this does.
+   */
+  static Element* DiscoverPartialListsAndTables(
+      const nsTArray<OwningNonNull<nsINode>>& aArrayOfNodes,
+      const nsTArray<OwningNonNull<Element>>&
+          aArrayOfListAndTableRelatedElements);
+
   enum class StartOrEnd { start, end };
-  void ReplaceOrphanedStructure(
-      StartOrEnd aStartOrEnd, nsTArray<OwningNonNull<nsINode>>& aNodeArray,
-      nsTArray<OwningNonNull<Element>>& aListAndTableArray,
-      int32_t aHighWaterMark);
+  void ReplaceOrphanedStructure(StartOrEnd aStartOrEnd,
+                                nsTArray<OwningNonNull<nsINode>>& aNodeArray,
+                                Element& aListOrTableElement);
 
   /**
    * FindReplaceableTableElement() is a helper method of
    * ReplaceOrphanedStructure().  If aNodeMaybeInTableElement is a descendant
    * of aTableElement, returns aNodeMaybeInTableElement or its nearest ancestor
    * whose tag name is `<td>`, `<th>`, `<tr>`, `<thead>`, `<tfoot>`, `<tbody>`
    * or `<caption>`.
    *
--- a/editor/libeditor/HTMLEditorDataTransfer.cpp
+++ b/editor/libeditor/HTMLEditorDataTransfer.cpp
@@ -398,40 +398,37 @@ nsresult HTMLEditor::DoInsertHTMLWithCon
 
   // build up list of parents of first node in list that are either
   // lists or tables.  First examine front of paste node list.
   AutoTArray<OwningNonNull<Element>, 4>
       arrayOfListAndTableRelatedElementsAtStart;
   HTMLEditor::CollectListAndTableRelatedElementsAt(
       nodeList[0], arrayOfListAndTableRelatedElementsAtStart);
   if (!arrayOfListAndTableRelatedElementsAtStart.IsEmpty()) {
-    int32_t highWaterMark = DiscoverPartialListsAndTables(
+    Element* listOrTableElement = HTMLEditor::DiscoverPartialListsAndTables(
         nodeList, arrayOfListAndTableRelatedElementsAtStart);
     // if we have pieces of tables or lists to be inserted, let's force the
     // paste to deal with table elements right away, so that it doesn't orphan
     // some table or list contents outside the table or list.
-    if (highWaterMark >= 0) {
+    if (listOrTableElement) {
       ReplaceOrphanedStructure(StartOrEnd::start, nodeList,
-                               arrayOfListAndTableRelatedElementsAtStart,
-                               highWaterMark);
+                               *listOrTableElement);
     }
   }
 
   // Now go through the same process again for the end of the paste node list.
   AutoTArray<OwningNonNull<Element>, 4> arrayOfListAndTableRelatedElementsAtEnd;
   HTMLEditor::CollectListAndTableRelatedElementsAt(
       nodeList.LastElement(), arrayOfListAndTableRelatedElementsAtEnd);
   if (!arrayOfListAndTableRelatedElementsAtEnd.IsEmpty()) {
-    int32_t highWaterMark = DiscoverPartialListsAndTables(
+    Element* listOrTableElement = HTMLEditor::DiscoverPartialListsAndTables(
         nodeList, arrayOfListAndTableRelatedElementsAtEnd);
     // don't orphan partial list or table structure
-    if (highWaterMark >= 0) {
-      ReplaceOrphanedStructure(StartOrEnd::end, nodeList,
-                               arrayOfListAndTableRelatedElementsAtEnd,
-                               highWaterMark);
+    if (listOrTableElement) {
+      ReplaceOrphanedStructure(StartOrEnd::end, nodeList, *listOrTableElement);
     }
   }
 
   MOZ_ASSERT(pointToInsert.GetContainer()->GetChildAt_Deprecated(
                  pointToInsert.Offset()) == pointToInsert.GetChild());
 
   // Loop over the node list and paste the nodes:
   nsCOMPtr<nsINode> parentBlock =
@@ -2749,59 +2746,93 @@ void HTMLEditor::CollectListAndTableRela
   for (nsIContent* content = nsIContent::FromNode(&aNode); content;
        content = content->GetParentElement()) {
     if (HTMLEditUtils::IsList(content) || HTMLEditUtils::IsTable(content)) {
       aOutArrayOfListAndTableElements.AppendElement(*content->AsElement());
     }
   }
 }
 
-int32_t HTMLEditor::DiscoverPartialListsAndTables(
-    nsTArray<OwningNonNull<nsINode>>& aPasteNodes,
-    nsTArray<OwningNonNull<Element>>& aListsAndTables) {
-  int32_t ret = -1;
-  int32_t listAndTableParents = aListsAndTables.Length();
-
-  // Scan insertion list for table elements (other than table).
-  for (auto& curNode : aPasteNodes) {
-    if (HTMLEditUtils::IsTableElement(curNode) &&
-        !curNode->IsHTMLElement(nsGkAtoms::table)) {
-      nsCOMPtr<Element> table = curNode->GetParentElement();
-      while (table && !table->IsHTMLElement(nsGkAtoms::table)) {
-        table = table->GetParentElement();
-      }
-      if (table) {
-        int32_t idx = aListsAndTables.IndexOf(table);
-        if (idx == -1) {
-          return ret;
-        }
-        ret = idx;
-        if (ret == listAndTableParents - 1) {
-          return ret;
+// static
+Element* HTMLEditor::DiscoverPartialListsAndTables(
+    const nsTArray<OwningNonNull<nsINode>>& aArrayOfNodes,
+    const nsTArray<OwningNonNull<Element>>&
+        aArrayOfListAndTableRelatedElements) {
+  Element* lastFoundAncestorListOrTableElement = nullptr;
+  for (auto& node : aArrayOfNodes) {
+    if (HTMLEditUtils::IsTableElement(node) &&
+        !node->IsHTMLElement(nsGkAtoms::table)) {
+      Element* tableElement = nullptr;
+      for (Element* maybeTableElement = node->GetParentElement();
+           maybeTableElement;
+           maybeTableElement = maybeTableElement->GetParentElement()) {
+        if (maybeTableElement->IsHTMLElement(nsGkAtoms::table)) {
+          tableElement = maybeTableElement;
+          break;
         }
       }
-    }
-    if (HTMLEditUtils::IsListItem(curNode)) {
-      nsCOMPtr<Element> list = curNode->GetParentElement();
-      while (list && !HTMLEditUtils::IsList(list)) {
-        list = list->GetParentElement();
+      if (!tableElement) {
+        continue;
+      }
+      // If we find a `<table>` element which is an ancestor of a table
+      // related element and is not an acestor of first nor last of
+      // aArrayOfNodes, return the last found list or `<table>` element.
+      // XXX Is that really expected that this returns a list element in this
+      //     case?
+      if (!aArrayOfListAndTableRelatedElements.Contains(tableElement)) {
+        return lastFoundAncestorListOrTableElement;
       }
-      if (list) {
-        int32_t idx = aListsAndTables.IndexOf(list);
-        if (idx == -1) {
-          return ret;
-        }
-        ret = idx;
-        if (ret == listAndTableParents - 1) {
-          return ret;
-        }
+      // If we find a `<table>` element which is topmost list or `<table>`
+      // element at first or last of aArrayOfNodes, return it.
+      if (aArrayOfListAndTableRelatedElements.LastElement().get() ==
+          tableElement) {
+        return tableElement;
+      }
+      // Otherwise, store the `<table>` element which is an ancestor but
+      // not topmost ancestor of first or last of aArrayOfNodes.
+      lastFoundAncestorListOrTableElement = tableElement;
+      continue;
+    }
+
+    if (!HTMLEditUtils::IsListItem(node)) {
+      continue;
+    }
+    Element* listElement = nullptr;
+    for (Element* maybeListElement = node->GetParentElement(); maybeListElement;
+         maybeListElement = maybeListElement->GetParentElement()) {
+      if (HTMLEditUtils::IsList(maybeListElement)) {
+        listElement = maybeListElement;
+        break;
       }
     }
+    if (!listElement) {
+      continue;
+    }
+    // If we find a list element which is ancestor of a list item element and
+    // is not an acestor of first nor last of aArrayOfNodes, return the last
+    // found list or `<table>` element.
+    // XXX Is that really expected that this returns a `<table>` element in
+    //     this case?
+    if (!aArrayOfListAndTableRelatedElements.Contains(listElement)) {
+      return lastFoundAncestorListOrTableElement;
+    }
+    // If we find a list element which is topmost list or `<table>` element at
+    // first or last of aArrayOfNodes, return it.
+    if (aArrayOfListAndTableRelatedElements.LastElement().get() ==
+        listElement) {
+      return listElement;
+    }
+    // Otherwise, store the list element which is an ancestor but not topmost
+    // ancestor of first or last of aArrayOfNodes.
+    lastFoundAncestorListOrTableElement = listElement;
   }
-  return ret;
+
+  // If we find only non-topmost list or `<table>` element, returns the last
+  // found one (meaning bottommost one).  Otherwise, nullptr.
+  return lastFoundAncestorListOrTableElement;
 }
 
 // static
 Element* HTMLEditor::FindReplaceableTableElement(
     Element& aTableElement, nsINode& aNodeMaybeInTableElement) {
   MOZ_ASSERT(aTableElement.IsHTMLElement(nsGkAtoms::table));
   // Perhaps, this is designed for climbing up the DOM tree from
   // aNodeMaybeInTableElement to aTableElement and making sure that
@@ -2878,38 +2909,36 @@ bool HTMLEditor::IsReplaceableListElemen
     // XXX If we find another list element, why don't we keep searching
     //     from its parent?
   }
   return false;
 }
 
 void HTMLEditor::ReplaceOrphanedStructure(
     StartOrEnd aStartOrEnd, nsTArray<OwningNonNull<nsINode>>& aNodeArray,
-    nsTArray<OwningNonNull<Element>>& aListAndTableArray,
-    int32_t aHighWaterMark) {
+    Element& aListOrTableElement) {
   MOZ_ASSERT(!aNodeArray.IsEmpty());
 
   OwningNonNull<nsINode>& edgeNode =
       aStartOrEnd == StartOrEnd::end ? aNodeArray.LastElement() : aNodeArray[0];
-  OwningNonNull<Element> curNode = aListAndTableArray[aHighWaterMark];
 
   // Find substructure of list or table that must be included in paste.
   Element* replaceElement;
-  if (HTMLEditUtils::IsList(curNode)) {
-    if (!HTMLEditor::IsReplaceableListElement(curNode, edgeNode)) {
+  if (HTMLEditUtils::IsList(&aListOrTableElement)) {
+    if (!HTMLEditor::IsReplaceableListElement(aListOrTableElement, edgeNode)) {
       return;
     }
-    replaceElement = curNode;
-  } else if (curNode->IsHTMLElement(nsGkAtoms::table)) {
-    replaceElement = HTMLEditor::FindReplaceableTableElement(curNode, edgeNode);
+    replaceElement = &aListOrTableElement;
+  } else {
+    MOZ_ASSERT(aListOrTableElement.IsHTMLElement(nsGkAtoms::table));
+    replaceElement =
+        HTMLEditor::FindReplaceableTableElement(aListOrTableElement, edgeNode);
     if (!replaceElement) {
       return;
     }
-  } else {
-    return;
   }
 
   // If we found substructure, paste it instead of its descendants.
   // Postprocess list to remove any descendants of this node so that we don't
   // insert them twice.
   uint32_t removedCount = 0;
   uint32_t originalLength = aNodeArray.Length();
   for (uint32_t i = 0; i < originalLength; i++) {