Closed Bug 1405794 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → ehsan
Blocks: 651120
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.
Attachment #8915257 - Flags: review?(masayuki) → review-
Attachment #8915257 - Attachment is obsolete: true
Attachment #8915773 - Flags: review?(masayuki) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62e1dd8deffb
Avoid using nsINode::GetChildAt() in TextEditor::SelectEntireDocument(); r=masayuki
https://hg.mozilla.org/mozilla-central/rev/62e1dd8deffb
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Flags: needinfo?(ehsan)
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?
Flags: needinfo?(ehsan) → needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(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.
(At least that was my assumption, I think...)
You need to log in before you can comment on or make changes to this bug.