1732349 - Use of nsIFrame::GetLineNumber followed by GetLineIterator is inefficient

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

The pattern:

    lineNum = frame->GetLineNumber(lockScroll, &containingBlock);
    iter = containingBlock->GetLineIterator();

is inefficient, because GetLineNumber internally gets a line-iterator for the block, but then discards it and we have to create a new one in the caller.

For the (common) case where the container is an nsBlockFrame, this may be a bit expensive: creating the nsLineIterator involves two passes through the block's entire linked list of lines (first to count them, and then to copy the pointers into the iterator's array), as well as an array allocation.

To avoid duplicating all that, we can use the GetContainingBlockForLine method added in bug 1729412 to do:

    block = frame->GetContainingBlockForLine(lockScroll, frameOnLine);
    iter = containingBlock->GetLineIterator();
    lineNum = iter->FindLineContaining(frameOnLine);

so that only a single nsLineIterator is constructed.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1732349 - patch 1 - Avoid redundant creation of a line-iterator in nsIFrame::PeekOffsetForLineEdge. r=#layout-reviewers

Depends on D126470

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1732349 - patch 2 - Avoid redundant creation of a line-iterator in nsIFrame::GetFrameFromDirection. r=#layout-reviewers

Depends on D126530

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1732349 - patch 3 - Avoid getting line iterators that are not actually needed in nsIFrame::PeekOffsetForLine. r=#layout-reviewers

We only need the actual iterator once, to get the line number; subsequently, we can use
the new CanProvideLineIterator method to check if we've found a block frame without
paying the cost of actually initializing it.

This eliminates the last remaining caller of nsIFrame::GetLineNumber, so that method
(which is prone to inefficient usage) can be removed altogether.

Depends on D126531

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3509a2d8c0d4
patch 1 - Avoid redundant creation of a line-iterator in nsIFrame::PeekOffsetForLineEdge. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d4093c6aff04
patch 2 - Avoid redundant creation of a line-iterator in nsIFrame::GetFrameFromDirection. r=emilio
https://hg.mozilla.org/integration/autoland/rev/52c066d3d353
patch 3 - Avoid getting line iterators that are not actually needed in nsIFrame::PeekOffsetForLine. r=emilio

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/3509a2d8c0d4
https://hg.mozilla.org/mozilla-central/rev/d4093c6aff04
https://hg.mozilla.org/mozilla-central/rev/52c066d3d353