Closed Bug 1481292 Opened 3 years ago Closed 3 years ago

Investigate if ContentEventHandler could stop using GetChildAt_Deprecated

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: smaug, Assigned: masayuki)

Details

Attachments

(2 files)

GetChildAt_Deprecated will be slow soon.
By which bug? And what should be used instead?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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?
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.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
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.
And RangeBounday does and will use the deprecated method, but usually it works without it.
ContentEventHandler::GetNodePositionHavingFlatText() is not used by anybody.
So, we should just get rid of it.
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 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
Attachment #8998147 - Flags: review+
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
Attachment #8998148 - Flags: review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/43373390fa8d
part 1: Get rid of ContentEventHandler::GetNodePositionHavingFlatText() because of unused r=smaug
Pushed by masayuki@d-toybox.com:
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
https://hg.mozilla.org/mozilla-central/rev/43373390fa8d
https://hg.mozilla.org/mozilla-central/rev/fcf0193311b0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.