1424094 - Get rid of the second argument of GetLastIBSplitSibling

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

We can do this now after bug 1419762 and bug 1419964. Let's also be consistent about what continuation we append to.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1424094: Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split.

The LastContinuation stuff behavior was what we did before the other patches, so
let's do that all the time to avoid surprises, though unfortunately I don't have
test-cases.

This effectively restores exactly the same behavior we had before all my frame
constructor patches, just with less code.

Review commit: https://reviewboard.mozilla.org/r/206448/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206448/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935573 [details]
Bug 1424094: Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206448/diff/1-2/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935573 [details]
Bug 1424094: Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206448/diff/2-3/

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 8935573 [details]
Bug 1424094: Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split.

https://reviewboard.mozilla.org/r/206448/#review212276

r=mats, feel free to resolve the nits as you please

::: layout/base/nsCSSFrameConstructor.cpp:6494
(Diff revision 3)
> +AppendParent(nsContainerFrame* aParentFrame)
> +{

nit: AppendParent sounds like it's appending something.  Maybe ContinuationToAppendTo, or ContinuationForAppend is better?
Prefixing the calls with :: wouldn't hurt either, to make it clear it's not a method call on 'this'.
(nsCSSFrameConstructor is quite big unfortunately so prefixing static helper function calls with :: helps when reading the code IMO)

::: layout/base/nsCSSFrameConstructor.cpp:6955
(Diff revision 3)
> -      // Get continuation that parents the last child.
> -      aInsertion->mParentFrame =
> -        nsLayoutUtils::LastContinuationWithChild(aInsertion->mParentFrame);
> -      // Deal with fieldsets
>        aInsertion->mParentFrame =
> -        ::GetAdjustedParentFrame(aInsertion->mParentFrame, aChild);
> +        GetAdjustedParentFrame(aInsertion->mParentFrame, aChild);

nit: fwiw, I'd prefer to keep ::

::: layout/base/nsCSSFrameConstructor.cpp:7561
(Diff revision 3)
>    if (GetDisplayContentsStyleFor(insertion.mContainer) ||
>        nsLayoutUtils::GetAfterFrame(insertion.mContainer)) {

Fwiw, it looks like everything from "parentFrame = AppendParent(parentFrame)" (inclusive) to this point could go in an else-branch of this if-statement, since 'parentFrame' isn't used by it.

Not intended as an optimization (since taking the then-branch should be rare), but perhaps it would make the code simpler/clearer?

::: layout/base/nsCSSFrameConstructor.cpp:7569
(Diff revision 3)
>    if (nextSibling) {
>      parentFrame = nextSibling->GetParent()->GetContentInsertionFrame();

nit: please move this inside the preceding if-block

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935573 [details]
Bug 1424094: Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split.

https://reviewboard.mozilla.org/r/206448/#review212276

> nit: AppendParent sounds like it's appending something.  Maybe ContinuationToAppendTo, or ContinuationForAppend is better?
> Prefixing the calls with :: wouldn't hurt either, to make it clear it's not a method call on 'this'.
> (nsCSSFrameConstructor is quite big unfortunately so prefixing static helper function calls with :: helps when reading the code IMO)

Yup, agree, I'll go with `::ContinuationToAppendTo` :)

> nit: fwiw, I'd prefer to keep ::

Fair, will keep those.

> Fwiw, it looks like everything from "parentFrame = AppendParent(parentFrame)" (inclusive) to this point could go in an else-branch of this if-statement, since 'parentFrame' isn't used by it.
> 
> Not intended as an optimization (since taking the then-branch should be rare), but perhaps it would make the code simpler/clearer?

Agreed it's nicer, so will do.

> nit: please move this inside the preceding if-block

Done.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935573 [details]
Bug 1424094: Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206448/diff/3-4/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cce54e34c28b
Remove second argument for GetLastIBSplitSibling, and always append to the last continuation of an IB split. r=mats

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/cce54e34c28b

Comment 10

[Boris Zbarsky [:bzbarsky]]

So I'm confused about one thing: if the display:contents/::after condition tests true but nextSibling comes back null, why do we not need to ContinuationToAppendTo()?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Blerg, I should not address review comments in the morning... :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Bug 1424680 tracks fixing the issue I raise in comment 10.