Closed
Bug 1393816
Opened 7 years ago
Closed 7 years ago
Selection should cache a range if a range isn't referred other than the Selection instance and Selection::Collapse() should reuse it
Categories
(Core :: DOM: Selection, enhancement)
Core
DOM: Selection
Tracking
()
RESOLVED
FIXED
mozilla57
| Tracking | Status | |
|---|---|---|
| firefox57 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files)
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).
|
Assignee | |
Updated•7 years ago
|
|
|
Assignee | |
Comment 1•7 years ago
|
||
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 4•7 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•7 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+
|
|
Assignee | |
Comment 6•7 years ago
|
||
| mozreview-review-reply | ||
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) |
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
|
|
Assignee | |
Comment 10•7 years ago
|
||
|
||
Comment 11•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/d311ad8f8c76
https://hg.mozilla.org/mozilla-central/rev/fbc63e299cd0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•