1873530 - Unify continuation linking operations by removing SetPrevContinuation() and SetPrevInFlow()

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

While working on bug 1865012, I notice that SetNextInFlow() and SetPrevInFlow() are almost always called together when setting up a continuation link, but the callers don't call them in particular order. For example, we call SetPrevInFlow() first in [1], but call SetNextInFlow() first in [2]. It makes asserting invariant difficult when only one direction of the doubly link is setup. Ideally, setting up a frame continuation doubly link should be one operation just like SetNextSibling() [3]. (Yes, we don't have SetPrevSibling.)

[1] https://searchfox.org/mozilla-central/rev/43c40b22c776d098afb89d5237da3464a7545532/layout/generic/nsSplittableFrame.cpp#27-28
[2] https://searchfox.org/mozilla-central/rev/43c40b22c776d098afb89d5237da3464a7545532/layout/base/nsBidiPresUtils.cpp#681-687
[3] https://searchfox.org/mozilla-central/rev/43c40b22c776d098afb89d5237da3464a7545532/layout/generic/nsIFrame.h#1791-1801

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1873530 Part 1 - Change two if-statements to match coding style. r?dholbert

While I'm here, change aFrame to mNextContinuation so that it is clearer
that we are tweaking the bit on mNextContinuation.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1873530 Part 2 - Add a helper to update cached continuations in nsTextFrame. r?dholbert,jfkthame

SetPrevContinuation() and SetPrevInFlow() have the same piece of code. Let's
move it into a helper.

Depends on D197756

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1873530 Part 3 - Unify continuation linking operations by removing SetPrevContinuation() and SetPrevInFlow(). r?dholbert,jfkthame

SetNextContinuation() and SetPrevContinuation() are almost always called
together when setting up a continuation link, but the callers don't call them in
particular order. We should unify them as one method so that it's more
ergonomics and robust, especially when we do more complex work such as caching
continuations. Same reason for SetNextInFlow() and SetPrevInFlow().

We choose to merge the SetPrevContinuation() code into SetNextContinuation() for
the symmetry of SetNextSibling(). (Yes, we don't have SetPrevSibling().)

This patch doesn't change behavior.

Depends on D197965

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d26545584207
Part 1 - Change two if-statements to match coding style. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/7b396c4118af
Part 2 - Add a helper to update cached continuations in nsTextFrame. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/dfcfa98a7e21
Part 3 - Unify continuation linking operations by removing SetPrevContinuation() and SetPrevInFlow(). r=jfkthame

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d26545584207
https://hg.mozilla.org/mozilla-central/rev/7b396c4118af
https://hg.mozilla.org/mozilla-central/rev/dfcfa98a7e21