1654452 - Use UniquePtr to manage nsSharedPageData lifetime (and a few other nearby cleanups)

Status: RESOLVED FIXED

(Core :: Printing: Output, task, P3)

Description

[Daniel Holbert [:dholbert]]

Minor print code cleanup opportunity:

nsSharedPageData is currently allocated via 'new', and freed via an explicit 'delete' in its owner's destructor.

We should just store it in a UniquePtr to simplify this a bit and have somewhat easier-to-follow guarantees & lifetime management.

Comment 1

[Daniel Holbert [:dholbert]]

It might even make sense to just have this struct be a direct member variable on nsPageSequenceFrame, rather than being managed indirectly via a separate allocation.

I'm not making that change here, since that's a more substantial change & would require doing s/->/./ all over the place. I'm just opting for this more targeted simplification for now, to remove one explicit delete. (Also: we pass around a pointer to this object to various other frame classes, and they store that pointer indefinitely. That passing-around feels more hygenic/idiomatic if this struct has its own allocation; though maybe that's just a personal bias.)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1654452 part 1: Manage nsSharedPageData lifetime using UniquePtr instead of new/delete. r?TYlin

This makes its ownership a bit clearer - it's now encoded in the type system
that nsPageSequenceFrame is the owner of this object, and we can reason about
the implications of that for the other frames that retain a pointer to this
object. (Fortunately all those other frames will be destroyed before the
nsPageSequenceFrame that owns their nsSharedPageData, because they're
descendants of that frame.)

Comment 3

[Daniel Holbert [:dholbert]]

Incidentally, this is the last "trivially upgradeable-to-UniquePtr" instance of delete mFoo (for some member variable with an "m" prefix) in the layout/generic directory -- here are the other cases that still remain in that directory:

(1) nsIFrame.cpp has some "delete mFoo" invocations that are part of DR_FrameTypeInfo and friends (which are used for debugging/logging & which would require some special configuration when testing changes, to be sure that code is getting exercised; aside from this configuration, these ones are probably easy to clean up if someone wants to do so).

(2) nsLineBox.h and .cpp have some "delete mFoo" instances for pointers that are stored in union { ... } structures (which can't directly be replaced with UniquePtr, because unions don't invoke destructors for their "members".)

(3) nsTextRunTransformations.h has a pointer to a shared mFactory object that one instance strongly owns, with the ownership tracked only by a bool (set to true on the owner instance, and false on all other instances that share it). This is also non-trivial to replace with UniquePtr.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1654452 part 2: Use range-based for loops to iterate nsPageSequenceFrame children. r?TYLin

Depends on D84463

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1654452 part 3: Misc cleanup in nsPageSequenceFrame: reorder stray include, drop unused define, use 'auto', remove unnecessary 'rv' var. r?TYLin

Depends on D84534

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34fa316a6b63
part 1: Manage nsSharedPageData lifetime using UniquePtr instead of new/delete. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/29e8f72d4e96
part 2: Use range-based for loops to iterate nsPageSequenceFrame children. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/1c5c7e3874e6
part 3: Misc cleanup in nsPageSequenceFrame: reorder stray include, drop unused define, use 'auto', remove unnecessary 'rv' var. r=TYLin

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/34fa316a6b63
https://hg.mozilla.org/mozilla-central/rev/29e8f72d4e96
https://hg.mozilla.org/mozilla-central/rev/1c5c7e3874e6