Closed Bug 1413181 Opened 2 years ago Closed 2 years ago

Clean up and optimize EditorBase::SplitNodeDeep()

Categories

(Core :: DOM: Editor, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(13 files)

59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
59 bytes, text/x-review-board-request
m_kato
: review+
Details
Priority: -- → P3
Hmm, our splitting code is really buggy.

The point which split related methods takes as argument means the point of the last child/character of newly created left node. E.g., specifying, 0, the only the first child of existing right node is moved to the new left node.  However, a lot of callers may set (aNode - aNode-Length()) as the point.  This is bugs of them.  However, EditorBase::SplitNodeImpl() ignores odd offset with this hack:

https://searchfox.org/mozilla-central/rev/7fb4cc447c06f14fe3b5c6b0c9d103a860937250/editor/libeditor/EditorBase.cpp#2939,2941-2942,2964-2966,2973-2974

>     for (int32_t i = aOffset - 1; i >= 0; i--) {
>       nsCOMPtr<nsIContent> childNode = childNodes->Item(i);
>       if (childNode) {
>         aExistingRightNode.RemoveChild(*childNode, rv);
>         if (!rv.Failed()) {
>           nsCOMPtr<nsIContent> firstChild = aNewLeftNode.GetFirstChild();
>           aNewLeftNode.InsertBefore(*childNode, firstChild, rv);
>         }
>       }
>       if (rv.Failed()) {
>         break;
>       }

This is same as:

>     for (int32_t i = aOffset - 1; i >= 0; i--) {
>       nsCOMPtr<nsIContent> childNode = childNodes->Item(i);
>       if (!childNode) {
>         continue;
>       }
>       aExistingRightNode.RemoveChild(*childNode, rv);
>       if (!rv.Failed()) {
>         break;
>       }
>       nsCOMPtr<nsIContent> firstChild = aNewLeftNode.GetFirstChild();
>       aNewLeftNode.InsertBefore(*childNode, firstChild, rv);
>       if (rv.Failed()) {
>         break;
>       }

So, just ignoring invalid offset at moving children...
Er, I misunderstood. According to the code moving children, given offset is start of the right node. Okay, this is odd case:

>   // Split the children between the two nodes.  At this point,
>   // aExistingRightNode has all the children.  Move all the children whose
>   // index is < aOffset to aNewLeftNode.
>   if (aOffset < 0) {
>     // This means move no children
>     return NS_OK;
>   }
Comment on attachment 8929922 [details]
Bug 1413181 - part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint

https://reviewboard.mozilla.org/r/200948/#review206652
Attachment #8929922 - Flags: review?(m_kato) → review+
Comment on attachment 8929923 [details]
Bug 1413181 - part 2: SplitNodeTransaction should store start of existing right node with RangeBoundary

https://reviewboard.mozilla.org/r/200950/#review206654

::: editor/libeditor/SplitNodeTransaction.cpp:66
(Diff revision 1)
>    }
>    if (NS_WARN_IF(!clone)) {
>      return NS_ERROR_UNEXPECTED;
>    }
>    mNewLeftNode = dont_AddRef(clone.forget().take()->AsContent());
> -  mEditorBase->MarkNodeDirty(mExistingRightNode->AsDOMNode());
> +  mEditorBase->MarkNodeDirty(mStartOfRightNode.Container()->AsDOMNode());

I will file a bug to create nsINode version of MarkNodeDirty since all caller can has nsINode.

::: editor/libeditor/SplitNodeTransaction.cpp:135
(Diff revision 1)
>    }
>  
> -  ErrorResult rv;
>    // First, massage the existing node so it is in its post-split state
> -  if (mExistingRightNode->GetAsText()) {
> -    rv = mExistingRightNode->GetAsText()->DeleteData(0, mOffset);
> +  Text* rightNodeAsText = mStartOfRightNode.Container()->GetAsText();
> +  if (rightNodeAsText) {

rightNodeAtText is used on true block only, so I think that the following is better.


if (mStartOfRightNode.Container()->IsNodeOfType(nsINode::eTEXT)) {
  Text* rightNodeAsText =     mStartOfRightNode.Container()->GetAsText();
Attachment #8929923 - Flags: review?(m_kato) → review+
Comment on attachment 8929924 [details]
Bug 1413181 - part 3: EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() should take EditorRawDOMPoint to specify the start of right node

https://reviewboard.mozilla.org/r/200952/#review206656

::: editor/libeditor/EditorBase.cpp:1541
(Diff revision 1)
> -                      int32_t aOffset,
> +                      ErrorResult& aError)
> -                      ErrorResult& aResult)
>  {
> +  if (NS_WARN_IF(!aStartOfRightNode.IsSet()) ||
> +      NS_WARN_IF(!aStartOfRightNode.Container()->IsContent())) {
> +    return nullptr;

Why doesn't aError.Throw call?  We should set error status.
Attachment #8929924 - Flags: review?(m_kato) → review+
Comment on attachment 8929925 [details]
Bug 1413181 - part 4: Redesign nsIEditActionListener::DidSplitNode()

https://reviewboard.mozilla.org/r/200954/#review206670
Attachment #8929925 - Flags: review?(m_kato) → review+
Comment on attachment 8929926 [details]
Bug 1413181 - part 5: HTMLEditRules::SplitParagraph() should take EditorRawDOMPoint instead of a set of container node and offset

https://reviewboard.mozilla.org/r/200956/#review206674
Attachment #8929926 - Flags: review?(m_kato) → review+
Comment on attachment 8929927 [details]
Bug 1413181 - part 6: Rename mozilla::EditorBase::EmptyContainers enum class to mozilla::SplitAtEdges for making its values clearer

https://reviewboard.mozilla.org/r/200958/#review206692
Attachment #8929927 - Flags: review?(m_kato) → review+
Comment on attachment 8929928 [details]
Bug 1413181 - part 7: EditorBase::SplitNodeDeep() should stop splitting orphan node if it meets an orphan node before meeting the most ancestor node to be split

https://reviewboard.mozilla.org/r/200960/#review206700
Attachment #8929928 - Flags: review?(m_kato) → review+
Comment on attachment 8929929 [details]
Bug 1413181 - part 8: Merge two if blocks in the loop of EditorBase::SplitNodeDeep()

https://reviewboard.mozilla.org/r/200962/#review206706
Attachment #8929929 - Flags: review?(m_kato) → review+
Comment on attachment 8929930 [details]
Bug 1413181 - part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop

https://reviewboard.mozilla.org/r/200964/#review206714
Attachment #8929930 - Flags: review?(m_kato) → review+
Comment on attachment 8929931 [details]
Bug 1413181 - part 10: Redesign EditorBase::SplitNodeDeep()

https://reviewboard.mozilla.org/r/200966/#review206734
Attachment #8929931 - Flags: review?(m_kato) → review+
Comment on attachment 8929932 [details]
Bug 1413181 - part 11: Create AutoEditorDOMPointOffsetInvalidator stack class for automatically invalidate offset of EditorDOMPoint

https://reviewboard.mozilla.org/r/200968/#review206738
Attachment #8929932 - Flags: review?(m_kato) → review+
Comment on attachment 8929933 [details]
Bug 1413181 - part 12: Redesign and rename HTMLEditRules::SplitAsNeeded()

https://reviewboard.mozilla.org/r/200970/#review206772
Attachment #8929933 - Flags: review?(m_kato) → review+
Comment on attachment 8929934 [details]
Bug 1413181 - part 13: HTMLEditRules::MaybeSplitAncestorsForInsert() should be able to return a DOM point in text node

https://reviewboard.mozilla.org/r/201046/#review206774
Attachment #8929934 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b1aa0ef250e8
part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ad1488c29218
part 2: SplitNodeTransaction should store start of existing right node with RangeBoundary r=m_kato
https://hg.mozilla.org/integration/autoland/rev/254338ffb5fa
part 3: EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() should take EditorRawDOMPoint to specify the start of right node r=m_kato
https://hg.mozilla.org/integration/autoland/rev/704c808bbfb0
part 4: Redesign nsIEditActionListener::DidSplitNode() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7cf991358f09
part 5: HTMLEditRules::SplitParagraph() should take EditorRawDOMPoint instead of a set of container node and offset r=m_kato
https://hg.mozilla.org/integration/autoland/rev/efc91fe72a26
part 6: Rename mozilla::EditorBase::EmptyContainers enum class to mozilla::SplitAtEdges for making its values clearer r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2d0bb2bb96b9
part 7: EditorBase::SplitNodeDeep() should stop splitting orphan node if it meets an orphan node before meeting the most ancestor node to be split r=m_kato
https://hg.mozilla.org/integration/autoland/rev/4f14d1405ed6
part 8: Merge two if blocks in the loop of EditorBase::SplitNodeDeep() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d0d42ddd114b
part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ad09d8d3d8f0
part 10: Redesign EditorBase::SplitNodeDeep() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1053929e8e53
part 11: Create AutoEditorDOMPointOffsetInvalidator stack class for automatically invalidate offset of EditorDOMPoint r=m_kato
https://hg.mozilla.org/integration/autoland/rev/27cb72cdc916
part 12: Redesign and rename HTMLEditRules::SplitAsNeeded() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/62cd3bf2db56
part 13: HTMLEditRules::MaybeSplitAncestorsForInsert() should be able to return a DOM point in text node r=m_kato
Depends on: 1421504
Depends on: 1423767
You need to log in before you can comment on or make changes to this bug.