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

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P2)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1533293 - part 1: Create Selection::SetStartAndEnd() to set new range as far as faster

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.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1533293 - part 2: Rewrite EditorBase::SelectEntireDocument() and its overrides

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.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1533293 - part 3: Make editor and ContentEventHandler not use Selection::Extend() due to too slow

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/19cff61f4fed
https://hg.mozilla.org/mozilla-central/rev/e536f6e123d8
https://hg.mozilla.org/mozilla-central/rev/d011dfe83683

Comment 7

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1533293) for causing Bug 1536595 a=backout
Backout: https://hg.mozilla.org/mozilla-central/rev/25398e555020fef80c7b2a06a0d4c667e861cd6f
This was a request from pascalc on IRC as part of https://bugzilla.mozilla.org/show_bug.cgi?id=1536595#c3

This was also backed out from beta: https://hg.mozilla.org/releases/mozilla-beta/rev/bfc174c4deb853203c5cf7d1c94571f9878d6d21

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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...

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1533293 - part 4: Make Selection::SetStartAndEndInternal() select frames by itself

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/919df8d66c0b
https://hg.mozilla.org/mozilla-central/rev/889784eb39fa
https://hg.mozilla.org/mozilla-central/rev/1b38317f4237
https://hg.mozilla.org/mozilla-central/rev/e75b6b732e7c