1413181 - Clean up and optimize EditorBase::SplitNodeDeep()

Status: RESOLVED FIXED

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

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d2eb0c54db5e13ac2a3d010c92d785c79900eba

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62e26ef982659adbc5db45d8907d33c0fe19686a

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8dc6f1cda4939967272198e0d61c9864ea5cbd9

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eeca55bacdc6ee2616adcefe4a6d566136baba4

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b1b7b1618100704b144bb01162bf5c4b4e14cd9

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eac56bce3784cf9b9642fe5d3932adf9222c346c

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d92d327cf1123fd730abef1d5fb6d3446a5af034

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e420f7b3f42e3ad9d4e0f2206f33832ff27f0e56

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1230987727a62db5cb7edeb8040c8dae018e3bbb

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f5a160461052fd19585191f47ad14b43f34f88

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2cb997b5759f53b4d40b8b850f6b6c4e1e3c741

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a37a855ec8bf0bc83c085ad9388bf2a521830067

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=087e212a97f26b4c7a4362c87d960cb1ca04a2ef

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee813cf46400d6e8c26030419781694a1deceda4

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=762ff632246ba3f90e8e70004fbc039f4d6123ab

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e861f5b1e897e953b15bc12ed0853b4e998e85d

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c64531894d3b8c617e2b21e2a0025abb1fd1db1

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4de4fb4debe3e36b97cdcc886bd908f7658e3cf9

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc15a9413a47e71cef346ab7f24395d92cda7a86

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5fa3d4fa339c110be684dd5e621a159f3e42046

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96f6affd888bd6d95e22e941a1aeb99da54f909

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70eecb87214fad9ee51493d116435b24a57b7e92

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint

EditorBaseSplitNodeImpl() should be clean up with EditorDOMPoint which should
be an argument to point the first point of right node (existing split node).

Review commit: https://reviewboard.mozilla.org/r/200948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200948/

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 2: SplitNodeTransaction should store start of existing right node with RangeBoundary

Make SplitNodeTransaction stores start of existing right node (which will be
split) instead of split point as a pair of the right node and offset in it.

Review commit: https://reviewboard.mozilla.org/r/200950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200950/

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 3: EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() should take EditorRawDOMPoint to specify the start of right node

EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() takes a set of
container and offset in it for specifying a point to split.

Instead, they should take EditorRawDOMPoint for specifying start of right node.

Review commit: https://reviewboard.mozilla.org/r/200952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200952/

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 4: Redesign nsIEditActionListener::DidSplitNode()

nsIEditActionListner::DidSplitNode() takes 4 arguments, the right node,
old offset in the old right node before splitting, the new left node and
nsresult.

Computing offset for this doesn't make sense because it's always same as
the length of the left node.  Additionally, nobody currently use nsersult.
So, we can get rid of it now.

Fortunately, nobody including comm-central and BlueGriffon implements
WillSplitNode().  So, we can get rid of it.  However, removing interface
method should be done in a follow up bug.  So, we can remove offset computation
in EditorBase::SplitNode() completely in the future.

Review commit: https://reviewboard.mozilla.org/r/200954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200954/

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 5: HTMLEditRules::SplitParagraph() should take EditorRawDOMPoint instead of a set of container node and offset

Although, we need to make WSRunObject::PrepareToSplitAcrossBlocks() in
another bug, we should do it now for making HTMLEditRules::ReturnInParagraph()
implementation simpler.

Review commit: https://reviewboard.mozilla.org/r/200956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200956/

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 6: Rename mozilla::EditorBase::EmptyContainers enum class to mozilla::SplitAtEdges for making its values clearer

EmptyContainers::yes and EmptyContainers::no are not so clear name what they
mean.

They means whether NodeSplitDeep() creates or won't create empty nodes when
split point is at start edge or end edge of an element.

This patch renames them to SplitAtEdges::eDoNotCreateEmptyContainer and
SplitAtEdges::eAllowToCreateEmptyContainer and make
HTMLEditor::InsertNodeAtPoint() use it instead of an bool argument.

Additionally, the argument of NodeSplitDeep() is now not optional because
this difference is really important when you read around the callers.

Review commit: https://reviewboard.mozilla.org/r/200958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200958/

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: 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

This doesn't change any meaning of the loop.

It is a bug if the loop meets orphan node before meeting the most ancestor
node to be split which is given as aNode.  So, we can check it before trying
to split it.

Review commit: https://reviewboard.mozilla.org/r/200960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200960/

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 8: Merge two if blocks in the loop of EditorBase::SplitNodeDeep()

Now, we can merge two if blocks in the loop of EditorBase::SplitNodeDeep() and
get rid of |didSplit| variable.

Review commit: https://reviewboard.mozilla.org/r/200962/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200962/

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop

Make the loop in EditorBase::SplitNodeDeep() use EditorDOMPoint for making the
code simpler.

Review commit: https://reviewboard.mozilla.org/r/200964/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200964/

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 10: Redesign EditorBase::SplitNodeDeep()

First of all, this patches fixes a bug of EditorBase::CreateNode().  It takes
|EditorRawDOMPoint&| but it should be |const EditorRawDOMPoint&| for making
callers can specify temporary instances.

Next, this patch creates |SplitNodeResult| stack class for result of
EditorBase::SplitNodeDeep().  SplitNodeDeep() needs to return previous node
and next node of split point.  They are called as left node and right node,
but these calls are really different term usage.  Therefore, this patch names:

aOutLeftNode -> SplitNodeResult::GetPreviousNode()
aOutRightNode -> SplitNodeResult::GetNextNode()

and also declares SplitNodeResult::GetLeftNode() and
SplitNodeResult::GetRightNode() which are same meaning as left/right node of
other splitting methods.

Additionally, of course, this patch makes SplitNodeDeep() use
|const EditorRawDOMPoint&| to receive the start point of right node.

Review commit: https://reviewboard.mozilla.org/r/200966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200966/

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 11: Create AutoEditorDOMPointOffsetInvalidator stack class for automatically invalidate offset of EditorDOMPoint

In the following patch, we need to invalidate offset a lot of places after
splitting nodes.  Therefore, there should be a helper stack class before that.

Review commit: https://reviewboard.mozilla.org/r/200968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200968/

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 12: Redesign and rename HTMLEditRules::SplitAsNeeded()

Now, we can make HTMLEditRules::SplitAsNeeded() use |SplitNodeResult| as its
result and |const EditorRawDOMPoint&| as specifying start of the deepest right
node.  Then, the implementation becomes simpler.

And I think that we should rename it to MaybeSplitAncestorsForInsert().

Additionally, this patch makes it stop calling
EditorBase::IsDescendantOfEditorRoot() and HTMLEditor::GetActiveEditingHost()
because they are really expensive.  Instead, it should check if the given start
point of the deepest right node is in active editing host before the loop and
if the loop reaches editing host.

Review commit: https://reviewboard.mozilla.org/r/200970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200970/

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Attachment: Bug 1413181 - part 13: HTMLEditRules::MaybeSplitAncestorsForInsert() should be able to return a DOM point in text node

HTMLEditRules::MaybeSplitAncestorsForInsert() may be called with a point in a
text and it needs to return given split point as is.  Additionally, the given
point may be in a text node.  So, it may not be represented with an
nsCOMPtr<nsIContent>.

Therefore, we need to add new member, EditorDOMPoint, to SplitNodeResult and
when MaybeSplitAncestorsForInsert() needs to return the given point as is,
it should use it.

Note that if the methods which return SplitNodeResult split some nodes actually,
the left node and/or the right node may be removed from the DOM tree.  In this
case, EditorDOMPoint cannot store such orphan node.  Therefore, we cannot
make mNextNode nor mPreviousNode EditorDOMPoint.

Review commit: https://reviewboard.mozilla.org/r/201046/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201046/

Comment 38

[Makoto Kato [:m_kato]]

Comment on attachment 8929922 [details]
Bug 1413181 - part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint

https://reviewboard.mozilla.org/r/200948/#review206652

Comment 39

[Makoto Kato [:m_kato]]

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();

Comment 40

[Makoto Kato [:m_kato]]

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.

Comment 41

[Makoto Kato [:m_kato]]

Comment on attachment 8929925 [details]
Bug 1413181 - part 4: Redesign nsIEditActionListener::DidSplitNode()

https://reviewboard.mozilla.org/r/200954/#review206670

Comment 42

[Makoto Kato [:m_kato]]

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

Comment 43

[Makoto Kato [:m_kato]]

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

Comment 44

[Makoto Kato [:m_kato]]

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

Comment 45

[Makoto Kato [:m_kato]]

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

Comment 46

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44218a93b8d7db4be9295a9a511f3be1d75931d4

Comment 47

[Makoto Kato [:m_kato]]

Comment on attachment 8929930 [details]
Bug 1413181 - part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop

https://reviewboard.mozilla.org/r/200964/#review206714

Comment 48

[Makoto Kato [:m_kato]]

Comment on attachment 8929931 [details]
Bug 1413181 - part 10: Redesign EditorBase::SplitNodeDeep()

https://reviewboard.mozilla.org/r/200966/#review206734

Comment 49

[Makoto Kato [:m_kato]]

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

Comment 50

[Makoto Kato [:m_kato]]

Comment on attachment 8929933 [details]
Bug 1413181 - part 12: Redesign and rename HTMLEditRules::SplitAsNeeded()

https://reviewboard.mozilla.org/r/200970/#review206772

Comment 51

[Makoto Kato [:m_kato]]

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

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929922 [details]
Bug 1413181 - part 1: Redesign EditorBase::SplitNodeImpl() with EditorDOMPoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200948/diff/1-2/

Comment 53

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929923 [details]
Bug 1413181 - part 2: SplitNodeTransaction should store start of existing right node with RangeBoundary

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200950/diff/1-2/

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929924 [details]
Bug 1413181 - part 3: EditorBase::CreateTxnForSplitNode() and EditorBase::SplitNode() should take EditorRawDOMPoint to specify the start of right node

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200952/diff/1-2/

Comment 55

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929925 [details]
Bug 1413181 - part 4: Redesign nsIEditActionListener::DidSplitNode()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200954/diff/1-2/

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929926 [details]
Bug 1413181 - part 5: HTMLEditRules::SplitParagraph() should take EditorRawDOMPoint instead of a set of container node and offset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200956/diff/1-2/

Comment 57

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929927 [details]
Bug 1413181 - part 6: Rename mozilla::EditorBase::EmptyContainers enum class to mozilla::SplitAtEdges for making its values clearer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200958/diff/1-2/

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200960/diff/1-2/

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929929 [details]
Bug 1413181 - part 8: Merge two if blocks in the loop of EditorBase::SplitNodeDeep()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200962/diff/1-2/

Comment 60

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929930 [details]
Bug 1413181 - part 9: Make EditorBase::SplitNodeDeep() use EditorDOMPoint in the loop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200964/diff/1-2/

Comment 61

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929931 [details]
Bug 1413181 - part 10: Redesign EditorBase::SplitNodeDeep()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200966/diff/1-2/

Comment 62

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929932 [details]
Bug 1413181 - part 11: Create AutoEditorDOMPointOffsetInvalidator stack class for automatically invalidate offset of EditorDOMPoint

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200968/diff/1-2/

Comment 63

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929933 [details]
Bug 1413181 - part 12: Redesign and rename HTMLEditRules::SplitAsNeeded()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200970/diff/1-2/

Comment 64

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away: 6/25-6/28)]

Comment on attachment 8929934 [details]
Bug 1413181 - part 13: HTMLEditRules::MaybeSplitAncestorsForInsert() should be able to return a DOM point in text node

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201046/diff/1-2/

Comment 65

[Pulsebot]

Pushed by [e-mail]:
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

Comment 66

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b1aa0ef250e8
https://hg.mozilla.org/mozilla-central/rev/ad1488c29218
https://hg.mozilla.org/mozilla-central/rev/254338ffb5fa
https://hg.mozilla.org/mozilla-central/rev/704c808bbfb0
https://hg.mozilla.org/mozilla-central/rev/7cf991358f09
https://hg.mozilla.org/mozilla-central/rev/efc91fe72a26
https://hg.mozilla.org/mozilla-central/rev/2d0bb2bb96b9
https://hg.mozilla.org/mozilla-central/rev/4f14d1405ed6
https://hg.mozilla.org/mozilla-central/rev/d0d42ddd114b
https://hg.mozilla.org/mozilla-central/rev/ad09d8d3d8f0
https://hg.mozilla.org/mozilla-central/rev/1053929e8e53
https://hg.mozilla.org/mozilla-central/rev/27cb72cdc916
https://hg.mozilla.org/mozilla-central/rev/62cd3bf2db56