1405794 - Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement)

Comment 1

[(no longer active)]

Attachment: Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8915257 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()

I don't really like this patch because as far as possible, editor methods should share the code deciding where is actual insertion point when there are some selected ranges. So, please add new method like GetEndChildNode() or something and use it here.

And if you use |if (NS_WARN_IF(something)) {\n  return something;\n}| instead of NS_ESNRUE_*() in new code, I'd be happy.

Comment 3

[(no longer active)]

Attachment: Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62e1dd8deffb
Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument(); r=masayuki

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62e1dd8deffb

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8915773 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()

I must be missing something. Why didn't this change the behavior in case selOffset pointed to the end of the last child.
GetChildAtEndOffset returns null, if range end point doesn't have next sibling.

Comment 7

[(no longer active)]

Here is the old GetEndNodeAndOffset (not GetChildAtEndOffset) which was being called here: https://github.com/mozilla/gecko-dev/commit/0111d1b30db037d300b454417bf4e3a7466bfd80#diff-b230691f6cf4408326794f09f385f18dL4091

This calls EditorBase::GetEndPoint(): https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/editor/libeditor/EditorBase.cpp#3893

The new code calls EditorBase::GetEndChildNode(): https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/editor/libeditor/EditorBase.cpp#3911

As far as I can tell, the logic in these two functions mirrors each other.  What difference am I missing?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8915773 [details] [diff] [review]
Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()


>   // Don't select the trailing BR node if we have one
>-  int32_t selOffset;
>-  nsCOMPtr<nsINode> selNode;
>-  rv = GetEndNodeAndOffset(aSelection, getter_AddRefs(selNode), &selOffset);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
>-  nsINode* childNode = selNode->GetChildAt(selOffset - 1);
If selOffset points to the end of the last child node, 
GetChildAt(selOffset - 1) will return the last child node.

>+  nsCOMPtr<nsIContent> childNode;
>+  rv = GetEndChildNode(aSelection, getter_AddRefs(childNode));
This returns null if the range's end point is after the last child

>+  if (childNode) {
>+    childNode = childNode->GetPreviousSibling();
>+  }
...and then this doesn't end up working, since childNode was null from the beginning.

Comment 9

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8915773 [details] [diff] [review]
> Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument()
> 
> 
> >   // Don't select the trailing BR node if we have one
> >-  int32_t selOffset;
> >-  nsCOMPtr<nsINode> selNode;
> >-  rv = GetEndNodeAndOffset(aSelection, getter_AddRefs(selNode), &selOffset);
> >-  NS_ENSURE_SUCCESS(rv, rv);
> >-
> >-  nsINode* childNode = selNode->GetChildAt(selOffset - 1);
> If selOffset points to the end of the last child node, 
> GetChildAt(selOffset - 1) will return the last child node.

I don't see how this could happen.  If this were the case, then GetEndNodeAndOffset would have returned NS_ERROR_FAILURE from here <https://github.com/mozilla/gecko-dev/commit/0111d1b30db037d300b454417bf4e3a7466bfd80#diff-b230691f6cf4408326794f09f385f18dL4104> which would cause us to bail out early from the NS_ENSURE_SUCCESS above.

Comment 10

[(no longer active)]

(At least that was my assumption, I think...)