1657024 - Simplify "get-next-page" logic in nsPageFrame.cpp

Status: RESOLVED FIXED

(Core :: Printing: Output, task)

Description

[Daniel Holbert [:dholbert]]

nsPageFrame.cpp has a function called "GetNextPage", which essentially just takes a nsPageContentFrame and returns the next continuation.

Right now it does this by doing unnecessarily-complex pointer-chasing (it returns the nsPageContentFrame's parent's next-sibling's first-child; i.e. it's "cousin", looked up by following each direct relationship).

We can simplify this to just use GetNextContinuation, since the series of nsPageContentFrames are all connected in a next-continuation relationship anyway. This will help avoid the need for further complexity in bug 1652278 (where we'll be removing the guarantee that nsPageFrame instances are siblings of each other).

Comment 1

[Daniel Holbert [:dholbert]]

One minor drawback of switching to GetNextContinuation() here is that it's a virtual function-call.

However:

• it saves us a lot of pointer-chasing (the "some ancestor's next-sibling's descendant" lookup)
• If we static_cast to the concrete frame type, then the compiler will probably devirtualize it and be able to inline it.[1]

[1] (Justification: here's a simplified scenario on godbolt Compiler Explorer, compiled under clang: https://godbolt.org/z/45xEj5
The "static_cast" sample in that snippet (the last one) gets inlined successfully, when you look at its assembly. The assembly doesn't seem to be doing dynamic dispatch.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1657024: Simplify code that's effectively walking the nsPageContentFrame continuation chain. r?TYLin

Before this patch, we used a clumsy "GetNextPage" helper-function (which,
despite its name, actually gets the next page content frame). It worked by
pointer-chasing to get the parent's next-sibling's first-child. This is
unnecessary; we can just directly query for the nsPageContentFrame's
next-continuation, since these frames are linked in a straightforward
continuation chain.

Note that GetNextContinuation() is a virtual function-call, but it may still be
faster than the pointer-chasing that we were doing with the GetNextPage()
API. Moreover, can help the compiler devirtualize it by static_cast'ing to the
concrete type before calling the method. This is safe since nsPageFrame always
contains a single nsPageContentFrame (which we already validate -- at least the
frame-type-part -- via assertions).

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37030207d352
Simplify code that's effectively walking the nsPageContentFrame continuation chain. r=TYLin

Comment 4

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/37030207d352