Closed Bug 1393816 Opened 6 years ago Closed 6 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)

Product:
Core
Component:
DOM: Selection
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Bug 1393816 - part1: Cache a range until new range is created in Selection
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1393816 - part2: Selection::SetBaseAndExtent() should use mCachedRange if it's available
59 bytes, text/x-review-board-request
smaug
: review+
Details 
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 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 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.
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
You need to log in before you can comment on or make changes to this bug.