1423835 - Clean up EditorDOMPointBase

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

EditorDOMPointBase should have a method, SetToEndOf() which automatically initializes with mParent->Length().

And its Container() should be renamed to GetContainer() because it may return nullptr.

And GetChildAtOffset() should be renamed to GetChild() because different from RangeBoundary, EditorDOMPointBase may store only child node.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

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

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

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

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

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

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1423835 - part 1: Add EditorDOMPointBase::SetToEndOf() to initialize the instance at end of container node

Editor code sometimes sets a DOM point to end of a node.  In this case, we
need to write |Set(node, node->Length())|.  So, it should have
|void SetToEndOf(const nsINode* aContainer)| for making meaning of the code
clearer.

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1423835 - part 2: Rename EditorDOMPointBase::Container() to EditorDOMPointBase::GetContainer() and add some useful methods to access its container

EditorDOMPointBase::Container() may return nullptr.  So, it should be renamed
to GetContainer().

Then, the method name becomes longer and a lot of callers access the result
directly.  So, there should be following methods to make the callers shorter:

- GetContainerAsContent() to return the container as nsIContent*.
- GetContainerAsElement() to return the container as dom::Element*.
- GetContainerAsText() to return the container as dom::Text.
- GetContainerAsDOMNode() to return the container as nsIDOMNode*.
- CanContainerHaveChildren() to check if the container can have child nodes.
- IsInDataNode() to check if the point is in a data node including text node.
- IsInTextNode() to check if the point is in a text node.
- IsContainerHTMLElement() to check if the container is specific HTML element.
- IsContainerAnyOfHTMLElements() to check if the container is one of the
  specified HTML elements.

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Attachment: Bug 1423835 - part 3: Rename EditorDOMPointBase::GetChildAtOffset() to EditorDOMPointBase::GetChild()

Different from RangeBoundaryBase, EditorDOMPointBase can store only child node.
I.e., mOffset may be invalid until its Offset() is called.  So,
GetChildAtOffset() isn't good name.  It should be just GetChild().

Similarly, GetNextSiblingOfChildAtOffset() and
GetPreviousSiblingOfChildAtOffset() should be renamed to GetNextSiblingOfChild()
and GetPreviousSiblingOfChild().

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

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8935689 [details]
Bug 1423835 - part 1: Add EditorDOMPointBase::SetToEndOf() to initialize the instance at end of container node

https://reviewboard.mozilla.org/r/206590/#review212642

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8935690 [details]
Bug 1423835 - part 2: Rename EditorDOMPointBase::Container() to EditorDOMPointBase::GetContainer() and add some useful methods to access its container

https://reviewboard.mozilla.org/r/206592/#review212646

::: editor/libeditor/HTMLEditRules.cpp:6862
(Diff revision 1)
>    EditorRawDOMPoint pointToInsertBR;
>    if (doesCRCreateNewP &&
> -      atStartOfSelection.Container() == &aParentDivOrP) {
> +      atStartOfSelection.GetContainer() == &aParentDivOrP) {
>      // We are at the edges of the block, so, we don't need to create new <br>.
>      brNode = nullptr;
> -  } else if (EditorBase::IsTextNode(atStartOfSelection.Container())) {
> +  } else if (EditorBase::IsTextNode(atStartOfSelection.GetContainer())) {

atStartOfSelection.IsInTextNode?

::: editor/libeditor/HTMLEditRules.cpp:8074
(Diff revision 1)
> -      return CreateMozBR(*point.Container(), point.Offset());
> +      return CreateMozBR(*point.GetContainer(), point.Offset());
>      }
>    }
>  
>    // are we in a text node?
> -  if (EditorBase::IsTextNode(point.Container())) {
> +  if (EditorBase::IsTextNode(point.GetContainer())) {

point.IsInTextNode?

::: editor/libeditor/HTMLEditorDataTransfer.cpp:372
(Diff revision 1)
>  
>      // Remember if we are in a link.
> -    bool bStartedInLink = IsInLink(pointToInsert.Container()->AsDOMNode());
> +    bool bStartedInLink = IsInLink(pointToInsert.GetContainerAsDOMNode());
>  
>      // Are we in a text node? If so, split it.
> -    if (IsTextNode(pointToInsert.Container())) {
> +    if (IsTextNode(pointToInsert.GetContainer())) {

pointToInsert.IsInTextNode?

::: editor/libeditor/TextEditor.cpp:587
(Diff revision 1)
>  
>          // node might be anonymous DIV, so we find better text node
>          EditorRawDOMPoint insertionPoint =
>            FindBetterInsertionPoint(atStartOfSelection);
>  
> -        if (IsTextNode(insertionPoint.Container())) {
> +        if (IsTextNode(insertionPoint.GetContainer())) {

insertionPoint.IsInTextNode?

::: editor/libeditor/TextEditor.cpp:766
(Diff revision 1)
>        return NS_ERROR_FAILURE;
>      }
>      MOZ_ASSERT(pointToInsert.IsSetAndValid());
>  
>      // don't put text in places that can't have it
> -    if (!IsTextNode(pointToInsert.Container()) &&
> +    if (!IsTextNode(pointToInsert.GetContainer()) &&

pointToInsert.IsInTextNode?

::: editor/libeditor/WSRunObject.cpp:1329
(Diff revision 1)
>      // Nothing to delete
>      return NS_OK;
>    }
>  
> -  if (aStartPoint.Container() == aEndPoint.Container() &&
> -      aStartPoint.Container()->GetAsText()) {
> +  if (aStartPoint.GetContainer() == aEndPoint.GetContainer() &&
> +      aStartPoint.GetContainerAsText()) {

aStartPoint.IsInTextNode?

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8935691 [details]
Bug 1423835 - part 3: Rename EditorDOMPointBase::GetChildAtOffset() to EditorDOMPointBase::GetChild()

https://reviewboard.mozilla.org/r/206594/#review212648

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

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

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8935689 [details]
Bug 1423835 - part 1: Add EditorDOMPointBase::SetToEndOf() to initialize the instance at end of container node

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

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8935690 [details]
Bug 1423835 - part 2: Rename EditorDOMPointBase::Container() to EditorDOMPointBase::GetContainer() and add some useful methods to access its container

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

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

Comment on attachment 8935691 [details]
Bug 1423835 - part 3: Rename EditorDOMPointBase::GetChildAtOffset() to EditorDOMPointBase::GetChild()

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1427b2fca8d7
part 1: Add EditorDOMPointBase::SetToEndOf() to initialize the instance at end of container node r=m_kato
https://hg.mozilla.org/integration/autoland/rev/c41a81c80769
part 2: Rename EditorDOMPointBase::Container() to EditorDOMPointBase::GetContainer() and add some useful methods to access its container r=m_kato
https://hg.mozilla.org/integration/autoland/rev/92403ed2a488
part 3: Rename EditorDOMPointBase::GetChildAtOffset() to EditorDOMPointBase::GetChild() r=m_kato

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/1427b2fca8d7
https://hg.mozilla.org/mozilla-central/rev/c41a81c80769
https://hg.mozilla.org/mozilla-central/rev/92403ed2a488