1481292 - Investigate if ContentEventHandler could stop using GetChildAt_Deprecated

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

GetChildAt_Deprecated will be slow soon.

Comment 1

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

By which bug? And what should be used instead?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Bug 651120.

Basically one should try to use nsINode::GetFirstChild / GetNextSibling / GetPreviousSibling when possible, and or if there is RangeBoundary around, it has its methods.

Comment 3

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

Thank you.

> Basically one should try to use nsINode::GetFirstChild / GetNextSibling / GetPreviousSibling when possible

It may be difficult to decide how many times to call GetFirstChild and GetNextSibling is faster than calling GetChildAt_Deprecated...

> or if there is RangeBoundary around, it has its methods
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/base/RangeBoundary.h#211

Anyway, RangeBoundary (and EditoDOMPoint which is another implementation of RangeBoundary) uses GetChildAt_Deprecated. Is it okay or will it updated with new API or something?

Comment 4

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

EditorDOMPoint's reference is here:
https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/editor/libeditor/EditorDOMPoint.h#788

I guess that editor accesses this path a lot...

Comment 5

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

GetChildAt_Deprecated is used in 4 places:

1. https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/events/ContentEventHandler.cpp#1590
2. https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/events/ContentEventHandler.cpp#1597
Looks like ContentEventHandler::GetNodePositionHavingFlatText() is dead code now. So, perhaps, we can remove all overloads of ContentEventHandler::GetNodePositionHavingFlatText().

3. https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/events/ContentEventHandler.cpp#1697
Must be replaced with RangeBoundary.

4. https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/events/ContentEventHandler.cpp#3039
Same.

I'll create a patch.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Great, thanks.

If the methods in nsINode or RangeBoundary can't be used, there is also the option to use nsINode::ChildNodes(), though that does use more memory. But at least the index is cached there.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

And RangeBounday does and will use the deprecated method, but usually it works without it.

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=387d10b9b27b5f826b63a037e5e537978513c4a2

Comment 9

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

Attachment: Bug 1481292 - part 1: Get rid of ContentEventHandler::GetNodePositionHavingFlatText() because of unused

ContentEventHandler::GetNodePositionHavingFlatText() is not used by anybody.
So, we should just get rid of it.

Comment 10

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

Attachment: Bug 1481292 - part 2: Make ContentEventHandler::GetLastFrameInRangeForTextRect() and ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode() use RangeBoundary when they treat start or end of range

ContentEventHandler::GetLastFrameInRangeForTextRect() and
ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode() use
nsINode::GetChildAt_Deprecated() directly. They use this method when they
treat start or end of a range. So, we can make them use RangeBoundary instead.

Comment 11

[Phabricator Automation]

Comment on attachment 8998147 [details]
Bug 1481292 - part 1: Get rid of ContentEventHandler::GetNodePositionHavingFlatText() because of unused

Olli Pettay [:smaug] has approved the revision.

https://phabricator.services.mozilla.com/D2842

Comment 12

[Phabricator Automation]

Comment on attachment 8998148 [details]
Bug 1481292 - part 2: Make ContentEventHandler::GetLastFrameInRangeForTextRect() and ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode() use RangeBoundary when they treat start or end of range

Olli Pettay [:smaug] has approved the revision.

https://phabricator.services.mozilla.com/D2843

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43373390fa8d
part 1: Get rid of ContentEventHandler::GetNodePositionHavingFlatText() because of unused r=smaug

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcf0193311b0
part 2: Make ContentEventHandler::GetLastFrameInRangeForTextRect() and ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode() use RangeBoundary when they treat start or end of range r=smaug

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/43373390fa8d
https://hg.mozilla.org/mozilla-central/rev/fcf0193311b0