Selection should cache a range if a range isn't referred other than the Selection instance and Selection::Collapse() should reuse it

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Blocks 2 bugs)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments)

When setting value of <input type="text">, nsTextEditorState::SetValue() removes all ranges of normal selection first. Then, EditorBase::AfterEdit() will set selection to the end of the editor with Selection::Collapse().

In this case, when Selection::Collapse() is called, there are no ranges due to RemoveAllRanges() is called. Therefore, it cannot reuse existing instance.

From the point of view of caller of Selection::RemoveAllRanges(), they can guarantee that a range will be necessary immediately after that. So, adding Selection::RemoveAllRangesTemporarily() and cache a range if a range isn't referred other than the Selection instance, Selection::Collapse() can reuse the cache and reduce the allocation cost (and some other cost like adding mutation observer).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8901408 [details]
Bug 1393816 - part1: Cache a range until new range is created in Selection

https://reviewboard.mozilla.org/r/172884/#review178306

::: commit-message-b2cef:4
(Diff revision 1)
> +Bug 1393816 - part1: Cache a range until new range is created in Selection r?smaug
> +
> +When setting value of <input type="text">, nsTextEditorState removes all
> +rages of normal selection first.  Then, TextEditor sets the value.  Finally,

ranges

::: commit-message-b2cef:5
(Diff revision 1)
> +Bug 1393816 - part1: Cache a range until new range is created in Selection r?smaug
> +
> +When setting value of <input type="text">, nsTextEditorState removes all
> +rages of normal selection first.  Then, TextEditor sets the value.  Finally,
> +TextEditor collapse selection at the end of the editor.

collapses the selection

end of what? End of text I guess, not editor.

::: dom/base/Selection.h:221
(Diff revision 1)
>    void RemoveAllRanges(mozilla::ErrorResult& aRv);
>  
> +  /**
> +   * RemoveAllRangesTemporarily() is useful if the caller will add one or more
> +   * ranges later.  This tries to cache a removing range if it's possible.
> +   * If a range is not referred by other than this selection, the range can be

s/other/anything else/
Attachment #8901408 - Flags: review?(bugs) → review+

Comment 5

2 years ago
mozreview-review
Comment on attachment 8901409 [details]
Bug 1393816 - part2: Selection::SetBaseAndExtent() should use mCachedRange if it's available

https://reviewboard.mozilla.org/r/172886/#review178308

::: dom/base/Selection.h:488
(Diff revision 1)
>    RefPtr<nsRange> mAnchorFocusRange;
>    // mCachedRange is set by RemoveAllRangesTemporarily() and used by
> -  // Collapse().  If there is a range which will be released by Clear(),
> -  // RemoveAllRangesTemporarily() stores it with this.  If Collapse() is
> -  // called without existing ranges, it'll reuse this range for saving the
> -  // creating cost.
> +  // Collapse() and SetBaseAndExtent().  If there is a range which will be
> +  // released by Clear(), RemoveAllRangesTemporarily() stores it with this.
> +  // If Collapse() is called without existing ranges, it'll reuse this range
> +  // for saving the creating cost.

creation cost
Attachment #8901409 - Flags: review?(bugs) → review+
Comment on attachment 8901408 [details]
Bug 1393816 - part1: Cache a range until new range is created in Selection

https://reviewboard.mozilla.org/r/172884/#review178306

> collapses the selection
> 
> end of what? End of text I guess, not editor.

Yes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

2 years ago
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d311ad8f8c76
part1: Cache a range until new range is created in Selection r=smaug
https://hg.mozilla.org/integration/autoland/rev/fbc63e299cd0
part2: Selection::SetBaseAndExtent() should use mCachedRange if it's available r=smaug
https://hg.mozilla.org/mozilla-central/rev/d311ad8f8c76
https://hg.mozilla.org/mozilla-central/rev/fbc63e299cd0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.