Closed Bug 1533293 Opened 7 months ago Closed 7 months ago

Stop using Selection::Extend() as far as possible due to too slow

Categories

(Core :: Editor, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 3 open bugs)

Details

Attachments

(4 files)

No description provided.
Priority: -- → P2

The patches will mark a lot of editor methods as MOZ_CAN_RUN_SCRIPT. But this bug won't fix the case of nsFrameSelection::TakeFocus() because only it cares multiple selection, but the behavior of Selection::Extend() is really complicated in such case. So, we cannot touch it without cleaning up Selection::Extend() itself.

Blocks: 1498984

Selection::Extend() is too slow because:

  • it may create some nsRange instances.
  • it users nsContentUtils::ComparePoints() multiple times.

Therefore, we can improve the performance if we can stop using it in some
places. First, this patch creates Selection::SetStartAndEnd() and
Selection::SetStartAndEndInLimiter() for internal use. They remove
current ranges, reuse nsRange instance as far as possible and add new
range which is set by their arguments. Then, this patch makes
Selection::SelectAllChildren() stop using Selection::Extend(). At this
time, this fixes a web-compat issue. Selection::Expand() cannot cross the
selection limiter boundary when there is a limiter (e.g., when an editing host
has focus). But we can now fix this with using the new internal API.

Note that methods in editor shouldn't move selection to outside of active
editing host. Therefore, this patch adds Selection::SetStartAndEndInLimiter()
and Selection::SetBaseAndExtentInLimiter() for them.

EditorBase::SelectEntierDocument() uses Selection::Extend() but it's too
slow. It should use Selection::SetStartAndEndInLimiter() instead.

Additionally, TextEditor::SelectEntierDocument() shrink the result of
EditorBase::SelectEntierDocument() with Selection::Extend() if there is
a moz-<br> element. So, TextEditor::SelectEntinerDocument() should set
its expected selection with a call for saving the runtime cost.

Then, we don't need to make EditorBase::SelectEntierDocument() as non-pure
virtual method. So, this patch makes each its callers call
Selection->SelectAllChildren() directly.

Selection::Extend() is too slow but editor and ContentEventHandler use it in
some places. We should make them use Selection::SetStartAndEndInLimiter() or
Selection::SetBaseAndExtentInLimiter(). The former is usable only when caller
guarantees the start point is prior to the end point in the DOM tree.
Otherwise, we need to use the latter even though it's slower than the former.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/19cff61f4fed
part 1: Create Selection::SetStartAndEnd() to set new range as far as faster r=smaug
https://hg.mozilla.org/integration/autoland/rev/e536f6e123d8
part 2: Rewrite EditorBase::SelectEntireDocument() and its overrides r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d011dfe83683
part 3: Make editor and ContentEventHandler not use Selection::Extend() due to too slow r=m_kato
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1536595
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla67 → ---
Depends on: 1536852

Thank you for the backout. Accidentally the land is included into the 67 due to the time-zone difference. I already have a patch for fixing bug 1536595. But I still don't understand what happens about bug 1536852...

When adding an range via Selection::AddItem() and only when it's caused by
user's operation, the range may be split at non-selectable content. This is
done in nsRange::ExcludeNonSelectableNodes() but it modifies the given range
as the first range. Then, Selection::AddRangeInternal() calls
Selection::SelectFrames() with the range which may have been modified by
nsRange::ExcludeNonSelectableNodes()
.

Therefore, only the frames between first child of <body> element and
previous element of first non-selectable content are painted as selection
after user does "Select All".

Selection::Extend() calls Selection::SelectFrames() by itself for all
existing ranges
. Therefore, this patch creates new method and makes both
Selection::Extend() and Selection::SetStartAndEndInternal() call it only
when the result is not only one range.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/919df8d66c0b
part 1: Create Selection::SetStartAndEnd() to set new range as far as faster r=smaug
https://hg.mozilla.org/integration/autoland/rev/889784eb39fa
part 2: Rewrite EditorBase::SelectEntireDocument() and its overrides r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1b38317f4237
part 3: Make editor and ContentEventHandler not use Selection::Extend() due to too slow r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e75b6b732e7c
part 4: Make Selection::SetStartAndEndInternal() select frames by itself r=smaug
You need to log in before you can comment on or make changes to this bug.