From 016313d559f71ba0d269e54b6c6e60dbe1e63280 Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Fri, 22 May 2020 11:02:24 -0400 Subject: Bug 1316302 - Part 1: Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action When HTMLEditRules::WillDeleteSelection() tries to remove something from the end/start of a block to its last/first text node but it's contained by block elements, it tries to join the container and the block. However, JoinBlocks() always fails to join them since it's impossible operation. In this case, HTMLEditRules::WillDeleteSelection() should retry to remove something in the leaf, however, it's impossible for now because JoinBlocks() and its helper methods don't return if it handles the action actually. This patch renames |JoinBlocks()| to |TryToJoinBlocks()| for representing what it is. And this patch adds |bool* aHandled| to the helper methods. Then, *aHandled and *aCancel are now always returns the result of each method. Therefore, for merging the result of multiple helper methods, callers need to receive the result with temporary variables and merge them by themselves. Note that when they modify DOM node actually or the action should do nothing (for example, selection is across tables), aHandled is set to true. Tag #1563 --- editor/libeditor/HTMLEditRules.cpp | 142 +++++++++++++++++++++++-------------- 1 file changed, 88 insertions(+), 54 deletions(-) (limited to 'editor/libeditor/HTMLEditRules.cpp') diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 545e22f70..13411b4f6 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -1844,10 +1844,10 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, // origCollapsed is used later to determine whether we should join blocks. We // don't really care about bCollapsed because it will be modified by - // ExtendSelectionForDelete later. JoinBlocks should happen if the original - // selection is collapsed and the cursor is at the end of a block element, in - // which case ExtendSelectionForDelete would always make the selection not - // collapsed. + // ExtendSelectionForDelete later. TryToJoinBlocks() should happen if the + // original selection is collapsed and the cursor is at the end of a block + // element, in which case ExtendSelectionForDelete would always make the + // selection not collapsed. bool bCollapsed = aSelection->Collapsed(); bool join = false; bool origCollapsed = bCollapsed; @@ -2196,9 +2196,13 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, address_of(selPointNode), &selPointOffset); NS_ENSURE_STATE(leftNode && leftNode->IsContent() && rightNode && rightNode->IsContent()); + bool handled = false, canceled = false; + rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), + &canceled, &handled); + // TODO: If it does nothing and previous or next node is a text node, + // we should modify it. *aHandled = true; - rv = JoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), - aCancel); + *aCancel |= canceled; NS_ENSURE_SUCCESS(rv, rv); } aSelection->Collapse(selPointNode, selPointOffset); @@ -2247,9 +2251,14 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater, address_of(selPointNode), &selPointOffset); NS_ENSURE_STATE(leftNode->IsContent() && rightNode->IsContent()); + bool handled = false, canceled = false; + rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), + &canceled, &handled); + // This should claim that trying to join the block means that + // this handles the action because the caller shouldn't do anything + // anymore in this case. *aHandled = true; - rv = JoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), - aCancel); + *aCancel |= canceled; NS_ENSURE_SUCCESS(rv, rv); } aSelection->Collapse(selPointNode, selPointOffset); @@ -2421,7 +2430,10 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, } if (join) { - rv = JoinBlocks(*leftParent, *rightParent, aCancel); + bool handled = false, canceled = false; + rv = TryToJoinBlocks(*leftParent, *rightParent, &canceled, &handled); + MOZ_ASSERT(*aHandled); + *aCancel |= canceled; NS_ENSURE_SUCCESS(rv, rv); } } @@ -2571,21 +2583,17 @@ HTMLEditRules::GetGoodSelPointForNode(nsINode& aNode, return ret; } - -/** - * This method is used to join two block elements. The right element is always - * joined to the left element. If the elements are the same type and not - * nested within each other, JoinNodesSmart is called (example, joining two - * list items together into one). If the elements are not the same type, or - * one is a descendant of the other, we instead destroy the right block placing - * its children into leftblock. DTD containment rules are followed throughout. - */ nsresult -HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, - nsIContent& aRightNode, - bool* aCanceled) +HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, + nsIContent& aRightNode, + bool* aCanceled, + bool* aHandled) { MOZ_ASSERT(aCanceled); + MOZ_ASSERT(aHandled); + + *aCanceled = false; + *aHandled = false; NS_ENSURE_STATE(mHTMLEditor); RefPtr htmlEditor(mHTMLEditor); @@ -2601,6 +2609,7 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, HTMLEditUtils::IsTableElement(rightBlock)) { // Do not try to merge table elements *aCanceled = true; + *aHandled = true; return NS_OK; } @@ -2617,6 +2626,7 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, // Bail if both blocks the same if (leftBlock == rightBlock) { *aCanceled = true; + *aHandled = true; return NS_OK; } @@ -2624,6 +2634,7 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, if (HTMLEditUtils::IsList(leftBlock) && HTMLEditUtils::IsListItem(rightBlock) && rightBlock->GetParentNode() == leftBlock) { + *aHandled = true; return NS_OK; } @@ -2694,11 +2705,15 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, rv = htmlEditor->MoveNode(child, leftList, -1); NS_ENSURE_SUCCESS(rv, rv); } + // XXX Should this set to true only when above for loop moves the node? + *aHandled = true; } else { - MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset); + bool handled = false; + MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled); + *aHandled |= handled; } - if (brNode) { - htmlEditor->DeleteNode(brNode); + if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) { + *aHandled = true; } // Offset below is where you find yourself in leftBlock when you traverse // upwards from rightBlock @@ -2730,7 +2745,9 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, nsCOMPtr brNode = CheckForInvisibleBR(*leftBlock, BRLocation::beforeBlock, leftOffset); if (mergeLists) { - MoveContents(*rightList, *leftList, &leftOffset); + bool handled = false; + MoveContents(*rightList, *leftList, &leftOffset, &handled); + *aHandled |= handled; } else { // Left block is a parent of right block, and the parent of the previous // visible content. Right block is a child and contains the contents we @@ -2786,12 +2803,14 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, NS_ENSURE_TRUE(previousContentParent, NS_ERROR_NULL_POINTER); + bool handled = false; rv = MoveBlock(*previousContentParent->AsElement(), *rightBlock, - previousContentOffset, rightOffset); + previousContentOffset, rightOffset, &handled); + *aHandled |= handled; NS_ENSURE_SUCCESS(rv, rv); } - if (brNode) { - htmlEditor->DeleteNode(brNode); + if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) { + *aHandled = true; } } else { // Normal case. Blocks are siblings, or at least close enough. An example @@ -2815,32 +2834,37 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode, ConvertListType(rightBlock, getter_AddRefs(newBlock), existingList, nsGkAtoms::li); } + *aHandled = true; } else { // Nodes are dissimilar types. - rv = MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset); + bool handled = false; + rv = + MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled); + *aHandled |= handled; NS_ENSURE_SUCCESS(rv, rv); } if (brNode) { rv = htmlEditor->DeleteNode(brNode); + // XXX In other top level if/else-if blocks, the result of DeleteNode() + // is ignored. Why does only this result is respected? NS_ENSURE_SUCCESS(rv, rv); + *aHandled = true; } } return NS_OK; } - -/** - * Moves the content from aRightBlock starting from aRightOffset into - * aLeftBlock at aLeftOffset. Note that the "block" might merely be inline - * nodes between
s, or between blocks, etc. DTD containment rules are - * followed throughout. - */ nsresult HTMLEditRules::MoveBlock(Element& aLeftBlock, Element& aRightBlock, int32_t aLeftOffset, - int32_t aRightOffset) + int32_t aRightOffset, + bool* aHandled) { + MOZ_ASSERT(aHandled); + + *aHandled = false; + nsTArray> arrayOfNodes; // GetNodesFromPoint is the workhorse that figures out what we wnat to move. nsresult rv = GetNodesFromPoint(EditorDOMPoint(&aRightBlock, aRightOffset), @@ -2851,15 +2875,20 @@ HTMLEditRules::MoveBlock(Element& aLeftBlock, // get the node to act on if (IsBlockNode(arrayOfNodes[i])) { // For block nodes, move their contents only, then delete block. + bool handled = false; rv = MoveContents(*arrayOfNodes[i]->AsElement(), aLeftBlock, - &aLeftOffset); + &aLeftOffset, &handled); + *aHandled |= handled; NS_ENSURE_SUCCESS(rv, rv); NS_ENSURE_STATE(mHTMLEditor); rv = mHTMLEditor->DeleteNode(arrayOfNodes[i]); + *aHandled = true; } else { // Otherwise move the content as is, checking against the DTD. + bool handled = false; rv = MoveNodeSmart(*arrayOfNodes[i]->AsContent(), aLeftBlock, - &aLeftOffset); + &aLeftOffset, &handled); + *aHandled |= handled; } } @@ -2868,17 +2897,16 @@ HTMLEditRules::MoveBlock(Element& aLeftBlock, return NS_OK; } -/** - * This method is used to move node aNode to (aDestElement, aInOutDestOffset). - * DTD containment rules are followed throughout. aInOutDestOffset is updated - * to point _after_ inserted content. - */ nsresult HTMLEditRules::MoveNodeSmart(nsIContent& aNode, Element& aDestElement, - int32_t* aInOutDestOffset) + int32_t* aInOutDestOffset, + bool* aHandled) { MOZ_ASSERT(aInOutDestOffset); + MOZ_ASSERT(aHandled); + + *aHandled = false; NS_ENSURE_STATE(mHTMLEditor); RefPtr htmlEditor(mHTMLEditor); @@ -2892,37 +2920,43 @@ HTMLEditRules::MoveNodeSmart(nsIContent& aNode, if (*aInOutDestOffset != -1) { (*aInOutDestOffset)++; } + // XXX Should we check if the node is actually moved in this case? + *aHandled = true; } else { // If it can't, move its children (if any), and then delete it. if (aNode.IsElement()) { - nsresult rv = - MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset); + bool handled = false; + nsresult rv = MoveContents(*aNode.AsElement(), aDestElement, + aInOutDestOffset, &handled); + *aHandled |= handled; NS_ENSURE_SUCCESS(rv, rv); } nsresult rv = htmlEditor->DeleteNode(&aNode); NS_ENSURE_SUCCESS(rv, rv); + *aHandled = true; } return NS_OK; } -/** - * Moves the _contents_ of aElement to (aDestElement, aInOutDestOffset). DTD - * containment rules are followed throughout. aInOutDestOffset is updated to - * point _after_ inserted content. - */ nsresult HTMLEditRules::MoveContents(Element& aElement, Element& aDestElement, - int32_t* aInOutDestOffset) + int32_t* aInOutDestOffset, + bool* aHandled) { MOZ_ASSERT(aInOutDestOffset); + MOZ_ASSERT(aHandled); + + *aHandled = false; NS_ENSURE_TRUE(&aElement != &aDestElement, NS_ERROR_ILLEGAL_VALUE); while (aElement.GetFirstChild()) { + bool handled = false; nsresult rv = MoveNodeSmart(*aElement.GetFirstChild(), aDestElement, - aInOutDestOffset); + aInOutDestOffset, &handled); + *aHandled |= handled; NS_ENSURE_SUCCESS(rv, rv); } return NS_OK; -- cgit v1.2.3 From 016c55f4094d99cc4aaa2f8ac0f2c22adbf96a5b Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Fri, 22 May 2020 11:03:43 -0400 Subject: Bug 1316302 - Part 2: WillDeleteSelection() should retry to handle it when selection is collapsed and JoinBlocks() doesn't handle nor cancel the action When selection is collapsed and JoinBlocks() doesn't handle nor cancel the action, WillDeleteSelection() should move selection to the start/end of leftmost/rightmost editable leaf node and retry to handle the action again. For avoiding infinite loop, it checks if selected node is changed actually before calling itself again. Tag #1563 --- editor/libeditor/HTMLEditRules.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'editor/libeditor/HTMLEditRules.cpp') diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 13411b4f6..156bbc179 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -2199,12 +2199,24 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, bool handled = false, canceled = false; rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), &canceled, &handled); - // TODO: If it does nothing and previous or next node is a text node, - // we should modify it. - *aHandled = true; + *aHandled |= handled; *aCancel |= canceled; NS_ENSURE_SUCCESS(rv, rv); } + + // If TryToJoinBlocks() didn't handle it and it's not canceled, + // user may want to modify the start leaf node or the last leaf node + // of the block. + if (!*aHandled && !*aCancel && leafNode != startNode) { + int32_t offset = + aAction == nsIEditor::ePrevious ? + static_cast(leafNode->Length()) : 0; + aSelection->Collapse(leafNode, offset); + return WillDeleteSelection(aSelection, aAction, aStripWrappers, + aCancel, aHandled); + } + + // Otherwise, we must have deleted the selection as user expected. aSelection->Collapse(selPointNode, selPointOffset); return NS_OK; } -- cgit v1.2.3 From d4cd571021b3651f4f2c6f3a2846e48ad726bcae Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Fri, 22 May 2020 11:48:40 -0400 Subject: Bug 1316302 - Part 3: Create EditActionResult class for making the methods which return nsresult, handled and canceled with out params In a lot of places, edit action handlers and their helper methods return nsresult and aHandled and aCanceled with out params. However, the out params cause the code complicated since: * it's not unclear if the method will overwrite aHandled and aCanceled value. * callers need to create temporary variable event if some of them are not necessary. This patch rewrites the helper methods of HTMLEditRules::WillDeleteSelection() with it. Tag #1563 --- editor/libeditor/HTMLEditRules.cpp | 274 +++++++++++++++++++++---------------- 1 file changed, 155 insertions(+), 119 deletions(-) (limited to 'editor/libeditor/HTMLEditRules.cpp') diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 156bbc179..fb732f05a 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -2196,12 +2196,13 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, address_of(selPointNode), &selPointOffset); NS_ENSURE_STATE(leftNode && leftNode->IsContent() && rightNode && rightNode->IsContent()); - bool handled = false, canceled = false; - rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), - &canceled, &handled); - *aHandled |= handled; - *aCancel |= canceled; - NS_ENSURE_SUCCESS(rv, rv); + EditActionResult ret = + TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent()); + *aHandled |= ret.Handled(); + *aCancel |= ret.Canceled(); + if (NS_WARN_IF(ret.Failed())) { + return ret.Rv(); + } } // If TryToJoinBlocks() didn't handle it and it's not canceled, @@ -2263,15 +2264,16 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater, address_of(selPointNode), &selPointOffset); NS_ENSURE_STATE(leftNode->IsContent() && rightNode->IsContent()); - bool handled = false, canceled = false; - rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(), - &canceled, &handled); + EditActionResult ret = + TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent()); // This should claim that trying to join the block means that // this handles the action because the caller shouldn't do anything // anymore in this case. *aHandled = true; - *aCancel |= canceled; - NS_ENSURE_SUCCESS(rv, rv); + *aCancel |= ret.Canceled(); + if (NS_WARN_IF(ret.Failed())) { + return ret.Rv(); + } } aSelection->Collapse(selPointNode, selPointOffset); return NS_OK; @@ -2442,11 +2444,12 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection, } if (join) { - bool handled = false, canceled = false; - rv = TryToJoinBlocks(*leftParent, *rightParent, &canceled, &handled); + EditActionResult ret = TryToJoinBlocks(*leftParent, *rightParent); MOZ_ASSERT(*aHandled); - *aCancel |= canceled; - NS_ENSURE_SUCCESS(rv, rv); + *aCancel |= ret.Canceled(); + if (NS_WARN_IF(ret.Failed())) { + return ret.Rv(); + } } } } @@ -2595,59 +2598,58 @@ HTMLEditRules::GetGoodSelPointForNode(nsINode& aNode, return ret; } -nsresult +EditActionResult HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, - nsIContent& aRightNode, - bool* aCanceled, - bool* aHandled) + nsIContent& aRightNode) { - MOZ_ASSERT(aCanceled); - MOZ_ASSERT(aHandled); - - *aCanceled = false; - *aHandled = false; + if (NS_WARN_IF(!mHTMLEditor)) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } - NS_ENSURE_STATE(mHTMLEditor); RefPtr htmlEditor(mHTMLEditor); nsCOMPtr leftBlock = htmlEditor->GetBlock(aLeftNode); nsCOMPtr rightBlock = htmlEditor->GetBlock(aRightNode); // Sanity checks - NS_ENSURE_TRUE(leftBlock && rightBlock, NS_ERROR_NULL_POINTER); - NS_ENSURE_STATE(leftBlock != rightBlock); + if (NS_WARN_IF(!leftBlock) || NS_WARN_IF(!rightBlock)) { + return EditActionIgnored(NS_ERROR_NULL_POINTER); + } + if (NS_WARN_IF(leftBlock == rightBlock)) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } if (HTMLEditUtils::IsTableElement(leftBlock) || HTMLEditUtils::IsTableElement(rightBlock)) { // Do not try to merge table elements - *aCanceled = true; - *aHandled = true; - return NS_OK; + return EditActionCanceled(); } // Make sure we don't try to move things into HR's, which look like blocks // but aren't containers if (leftBlock->IsHTMLElement(nsGkAtoms::hr)) { leftBlock = htmlEditor->GetBlockNodeParent(leftBlock); + if (NS_WARN_IF(!leftBlock)) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } } if (rightBlock->IsHTMLElement(nsGkAtoms::hr)) { rightBlock = htmlEditor->GetBlockNodeParent(rightBlock); + if (NS_WARN_IF(!rightBlock)) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } } - NS_ENSURE_STATE(leftBlock && rightBlock); // Bail if both blocks the same if (leftBlock == rightBlock) { - *aCanceled = true; - *aHandled = true; - return NS_OK; + return EditActionIgnored(); } // Joining a list item to its parent is a NOP. if (HTMLEditUtils::IsList(leftBlock) && HTMLEditUtils::IsListItem(rightBlock) && rightBlock->GetParentNode() == leftBlock) { - *aHandled = true; - return NS_OK; + return EditActionHandled(); } // Special rule here: if we are trying to join list items, and they are in @@ -2681,6 +2683,7 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, // offset below is where you find yourself in rightBlock when you traverse // upwards from leftBlock + EditActionResult ret(NS_OK); if (EditorUtils::IsDescendantOf(leftBlock, rightBlock, &rightOffset)) { // Tricky case. Left block is inside right block. Do ws adjustment. This // just destroys non-visible ws at boundaries we will be joining. @@ -2688,7 +2691,9 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, nsresult rv = WSRunObject::ScrubBlockBoundary(htmlEditor, WSRunObject::kBlockEnd, leftBlock); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } { // We can't just track rightBlock because it's an Element. @@ -2698,11 +2703,16 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, rv = WSRunObject::ScrubBlockBoundary(htmlEditor, WSRunObject::kAfterBlock, rightBlock, rightOffset); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } + if (trackingRightBlock->IsElement()) { rightBlock = trackingRightBlock->AsElement(); } else { - NS_ENSURE_STATE(trackingRightBlock->GetParentElement()); + if (NS_WARN_IF(!trackingRightBlock->GetParentElement())) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } rightBlock = trackingRightBlock->GetParentElement(); } } @@ -2715,17 +2725,22 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, for (nsCOMPtr child = rightList->GetChildAt(offset); child; child = rightList->GetChildAt(rightOffset)) { rv = htmlEditor->MoveNode(child, leftList, -1); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } } // XXX Should this set to true only when above for loop moves the node? - *aHandled = true; + ret.MarkAsHandled(); } else { - bool handled = false; - MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled); - *aHandled |= handled; + // XXX Why do we ignore the result of MoveBlock()? + EditActionResult retMoveBlock = + MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset); + if (retMoveBlock.Handled()) { + ret.MarkAsHandled(); + } } if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) { - *aHandled = true; + ret.MarkAsHandled(); } // Offset below is where you find yourself in leftBlock when you traverse // upwards from rightBlock @@ -2735,7 +2750,10 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, nsresult rv = WSRunObject::ScrubBlockBoundary(htmlEditor, WSRunObject::kBlockStart, rightBlock); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } + { // We can't just track leftBlock because it's an Element, so track // something else. @@ -2745,11 +2763,16 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, rv = WSRunObject::ScrubBlockBoundary(htmlEditor, WSRunObject::kBeforeBlock, leftBlock, leftOffset); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } + if (trackingLeftBlock->IsElement()) { leftBlock = trackingLeftBlock->AsElement(); } else { - NS_ENSURE_STATE(trackingLeftBlock->GetParentElement()); + if (NS_WARN_IF(!trackingLeftBlock->GetParentElement())) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } leftBlock = trackingLeftBlock->GetParentElement(); } } @@ -2757,9 +2780,12 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, nsCOMPtr brNode = CheckForInvisibleBR(*leftBlock, BRLocation::beforeBlock, leftOffset); if (mergeLists) { - bool handled = false; - MoveContents(*rightList, *leftList, &leftOffset, &handled); - *aHandled |= handled; + // XXX Why do we ignore the result of MoveContents()? + EditActionResult retMoveContents = + MoveContents(*rightList, *leftList, &leftOffset); + if (retMoveContents.Handled()) { + ret.MarkAsHandled(); + } } else { // Left block is a parent of right block, and the parent of the previous // visible content. Right block is a child and contains the contents we @@ -2804,7 +2830,9 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, &previousContentOffset, nullptr, nullptr, nullptr, getter_AddRefs(splittedPreviousContent)); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } if (splittedPreviousContent) { previousContentParent = splittedPreviousContent->GetParentNode(); @@ -2813,16 +2841,18 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, } } - NS_ENSURE_TRUE(previousContentParent, NS_ERROR_NULL_POINTER); + if (NS_WARN_IF(!previousContentParent)) { + return EditActionIgnored(NS_ERROR_NULL_POINTER); + } - bool handled = false; - rv = MoveBlock(*previousContentParent->AsElement(), *rightBlock, - previousContentOffset, rightOffset, &handled); - *aHandled |= handled; - NS_ENSURE_SUCCESS(rv, rv); + ret |= MoveBlock(*previousContentParent->AsElement(), *rightBlock, + previousContentOffset, rightOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; + } } if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) { - *aHandled = true; + ret.MarkAsHandled(); } } else { // Normal case. Blocks are siblings, or at least close enough. An example @@ -2833,7 +2863,9 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, // Adjust whitespace at block boundaries nsresult rv = WSRunObject::PrepareToJoinBlocks(htmlEditor, leftBlock, rightBlock); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } // Do br adjustment. nsCOMPtr brNode = CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd); @@ -2846,81 +2878,83 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, ConvertListType(rightBlock, getter_AddRefs(newBlock), existingList, nsGkAtoms::li); } - *aHandled = true; + ret.MarkAsHandled(); } else { // Nodes are dissimilar types. - bool handled = false; - rv = - MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled); - *aHandled |= handled; - NS_ENSURE_SUCCESS(rv, rv); + ret |= MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; + } } if (brNode) { rv = htmlEditor->DeleteNode(brNode); // XXX In other top level if/else-if blocks, the result of DeleteNode() // is ignored. Why does only this result is respected? - NS_ENSURE_SUCCESS(rv, rv); - *aHandled = true; + if (NS_WARN_IF(NS_FAILED(rv))) { + return ret.SetResult(rv); + } + ret.MarkAsHandled(); } } - return NS_OK; + return ret; } -nsresult +EditActionResult HTMLEditRules::MoveBlock(Element& aLeftBlock, Element& aRightBlock, int32_t aLeftOffset, - int32_t aRightOffset, - bool* aHandled) + int32_t aRightOffset) { - MOZ_ASSERT(aHandled); - - *aHandled = false; - nsTArray> arrayOfNodes; // GetNodesFromPoint is the workhorse that figures out what we wnat to move. nsresult rv = GetNodesFromPoint(EditorDOMPoint(&aRightBlock, aRightOffset), EditAction::makeList, arrayOfNodes, TouchContent::yes); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } + + EditActionResult ret(NS_OK); for (uint32_t i = 0; i < arrayOfNodes.Length(); i++) { // get the node to act on if (IsBlockNode(arrayOfNodes[i])) { // For block nodes, move their contents only, then delete block. - bool handled = false; - rv = MoveContents(*arrayOfNodes[i]->AsElement(), aLeftBlock, - &aLeftOffset, &handled); - *aHandled |= handled; - NS_ENSURE_SUCCESS(rv, rv); - NS_ENSURE_STATE(mHTMLEditor); + ret |= + MoveContents(*arrayOfNodes[i]->AsElement(), aLeftBlock, &aLeftOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; + } + if (NS_WARN_IF(!mHTMLEditor)) { + return ret.SetResult(NS_ERROR_UNEXPECTED); + } rv = mHTMLEditor->DeleteNode(arrayOfNodes[i]); - *aHandled = true; + ret.MarkAsHandled(); } else { // Otherwise move the content as is, checking against the DTD. - bool handled = false; - rv = MoveNodeSmart(*arrayOfNodes[i]->AsContent(), aLeftBlock, - &aLeftOffset, &handled); - *aHandled |= handled; + ret |= + MoveNodeSmart(*arrayOfNodes[i]->AsContent(), aLeftBlock, &aLeftOffset); } } // XXX We're only checking return value of the last iteration - NS_ENSURE_SUCCESS(rv, rv); - return NS_OK; + if (NS_WARN_IF(ret.Failed())) { + return ret; + } + + return ret; } -nsresult +EditActionResult HTMLEditRules::MoveNodeSmart(nsIContent& aNode, Element& aDestElement, - int32_t* aInOutDestOffset, - bool* aHandled) + int32_t* aInOutDestOffset) { MOZ_ASSERT(aInOutDestOffset); - MOZ_ASSERT(aHandled); - *aHandled = false; + if (NS_WARN_IF(!mHTMLEditor)) { + return EditActionIgnored(NS_ERROR_UNEXPECTED); + } - NS_ENSURE_STATE(mHTMLEditor); RefPtr htmlEditor(mHTMLEditor); // Check if this node can go into the destination node @@ -2928,50 +2962,52 @@ HTMLEditRules::MoveNodeSmart(nsIContent& aNode, // If it can, move it there nsresult rv = htmlEditor->MoveNode(&aNode, &aDestElement, *aInOutDestOffset); - NS_ENSURE_SUCCESS(rv, rv); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } if (*aInOutDestOffset != -1) { (*aInOutDestOffset)++; } // XXX Should we check if the node is actually moved in this case? - *aHandled = true; + return EditActionHandled(); } else { // If it can't, move its children (if any), and then delete it. + EditActionResult ret(NS_OK); if (aNode.IsElement()) { - bool handled = false; - nsresult rv = MoveContents(*aNode.AsElement(), aDestElement, - aInOutDestOffset, &handled); - *aHandled |= handled; - NS_ENSURE_SUCCESS(rv, rv); + ret = MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; + } } nsresult rv = htmlEditor->DeleteNode(&aNode); - NS_ENSURE_SUCCESS(rv, rv); - *aHandled = true; + if (NS_WARN_IF(NS_FAILED(rv))) { + return ret.SetResult(rv); + } + return ret.MarkAsHandled(); } - return NS_OK; } -nsresult +EditActionResult HTMLEditRules::MoveContents(Element& aElement, Element& aDestElement, - int32_t* aInOutDestOffset, - bool* aHandled) + int32_t* aInOutDestOffset) { MOZ_ASSERT(aInOutDestOffset); - MOZ_ASSERT(aHandled); - *aHandled = false; - - NS_ENSURE_TRUE(&aElement != &aDestElement, NS_ERROR_ILLEGAL_VALUE); + if (NS_WARN_IF(&aElement == &aDestElement)) { + return EditActionIgnored(NS_ERROR_ILLEGAL_VALUE); + } + EditActionResult ret(NS_OK); while (aElement.GetFirstChild()) { - bool handled = false; - nsresult rv = MoveNodeSmart(*aElement.GetFirstChild(), aDestElement, - aInOutDestOffset, &handled); - *aHandled |= handled; - NS_ENSURE_SUCCESS(rv, rv); + ret |= + MoveNodeSmart(*aElement.GetFirstChild(), aDestElement, aInOutDestOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; + } } - return NS_OK; + return ret; } -- cgit v1.2.3 From d0126b96cd6527e6dc89530b333fb0c196aba30d Mon Sep 17 00:00:00 2001 From: Gaming4JC Date: Fri, 22 May 2020 11:55:44 -0400 Subject: Bug 1316302 - Part 4: Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style for making scope of EditActionResult variable smaller For now, let's make the scope of EditActionResult variable in them smaller without big change. Tag #1563 --- editor/libeditor/HTMLEditRules.cpp | 112 +++++++++++++++++++------------------ 1 file changed, 59 insertions(+), 53 deletions(-) (limited to 'editor/libeditor/HTMLEditRules.cpp') diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index fb732f05a..d3cbb8775 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -2683,7 +2683,6 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, // offset below is where you find yourself in rightBlock when you traverse // upwards from leftBlock - EditActionResult ret(NS_OK); if (EditorUtils::IsDescendantOf(leftBlock, rightBlock, &rightOffset)) { // Tricky case. Left block is inside right block. Do ws adjustment. This // just destroys non-visible ws at boundaries we will be joining. @@ -2719,6 +2718,7 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, // Do br adjustment. nsCOMPtr brNode = CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd); + EditActionResult ret(NS_OK); if (mergeLists) { // The idea here is to take all children in rightList that are past // offset, and pull them into leftlist. @@ -2742,9 +2742,12 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) { ret.MarkAsHandled(); } + return ret; + } + // Offset below is where you find yourself in leftBlock when you traverse // upwards from rightBlock - } else if (EditorUtils::IsDescendantOf(rightBlock, leftBlock, &leftOffset)) { + if (EditorUtils::IsDescendantOf(rightBlock, leftBlock, &leftOffset)) { // Tricky case. Right block is inside left block. Do ws adjustment. This // just destroys non-visible ws at boundaries we will be joining. nsresult rv = WSRunObject::ScrubBlockBoundary(htmlEditor, @@ -2779,6 +2782,7 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, // Do br adjustment. nsCOMPtr brNode = CheckForInvisibleBR(*leftBlock, BRLocation::beforeBlock, leftOffset); + EditActionResult ret(NS_OK); if (mergeLists) { // XXX Why do we ignore the result of MoveContents()? EditActionResult retMoveContents = @@ -2854,47 +2858,49 @@ HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode, if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) { ret.MarkAsHandled(); } - } else { - // Normal case. Blocks are siblings, or at least close enough. An example - // of the latter is

paragraph

  • one
  • two
  • three
. The - // first li and the p are not true siblings, but we still want to join them - // if you backspace from li into p. + return ret; + } - // Adjust whitespace at block boundaries - nsresult rv = - WSRunObject::PrepareToJoinBlocks(htmlEditor, leftBlock, rightBlock); - if (NS_WARN_IF(NS_FAILED(rv))) { - return EditActionIgnored(rv); - } - // Do br adjustment. - nsCOMPtr brNode = - CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd); - if (mergeLists || leftBlock->NodeInfo()->NameAtom() == - rightBlock->NodeInfo()->NameAtom()) { - // Nodes are same type. merge them. - EditorDOMPoint pt = JoinNodesSmart(*leftBlock, *rightBlock); - if (pt.node && mergeLists) { - nsCOMPtr newBlock; - ConvertListType(rightBlock, getter_AddRefs(newBlock), - existingList, nsGkAtoms::li); - } - ret.MarkAsHandled(); - } else { - // Nodes are dissimilar types. - ret |= MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset); - if (NS_WARN_IF(ret.Failed())) { - return ret; - } + // Normal case. Blocks are siblings, or at least close enough. An example + // of the latter is

paragraph

  • one
  • two
  • three
. The + // first li and the p are not true siblings, but we still want to join them + // if you backspace from li into p. + + // Adjust whitespace at block boundaries + nsresult rv = + WSRunObject::PrepareToJoinBlocks(htmlEditor, leftBlock, rightBlock); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionIgnored(rv); + } + // Do br adjustment. + nsCOMPtr brNode = + CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd); + EditActionResult ret(NS_OK); + if (mergeLists || leftBlock->NodeInfo()->NameAtom() == + rightBlock->NodeInfo()->NameAtom()) { + // Nodes are same type. merge them. + EditorDOMPoint pt = JoinNodesSmart(*leftBlock, *rightBlock); + if (pt.node && mergeLists) { + nsCOMPtr newBlock; + ConvertListType(rightBlock, getter_AddRefs(newBlock), + existingList, nsGkAtoms::li); + } + ret.MarkAsHandled(); + } else { + // Nodes are dissimilar types. + ret |= MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; } - if (brNode) { - rv = htmlEditor->DeleteNode(brNode); - // XXX In other top level if/else-if blocks, the result of DeleteNode() - // is ignored. Why does only this result is respected? - if (NS_WARN_IF(NS_FAILED(rv))) { - return ret.SetResult(rv); - } - ret.MarkAsHandled(); + } + if (brNode) { + rv = htmlEditor->DeleteNode(brNode); + // XXX In other top level if blocks, the result of DeleteNode() + // is ignored. Why does only this result is respected? + if (NS_WARN_IF(NS_FAILED(rv))) { + return ret.SetResult(rv); } + ret.MarkAsHandled(); } return ret; } @@ -2970,22 +2976,22 @@ HTMLEditRules::MoveNodeSmart(nsIContent& aNode, } // XXX Should we check if the node is actually moved in this case? return EditActionHandled(); - } else { - // If it can't, move its children (if any), and then delete it. - EditActionResult ret(NS_OK); - if (aNode.IsElement()) { - ret = MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset); - if (NS_WARN_IF(ret.Failed())) { - return ret; - } - } + } - nsresult rv = htmlEditor->DeleteNode(&aNode); - if (NS_WARN_IF(NS_FAILED(rv))) { - return ret.SetResult(rv); + // If it can't, move its children (if any), and then delete it. + EditActionResult ret(NS_OK); + if (aNode.IsElement()) { + ret = MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset); + if (NS_WARN_IF(ret.Failed())) { + return ret; } - return ret.MarkAsHandled(); } + + nsresult rv = htmlEditor->DeleteNode(&aNode); + if (NS_WARN_IF(NS_FAILED(rv))) { + return ret.SetResult(rv); + } + return ret.MarkAsHandled(); } EditActionResult -- cgit v1.2.3