Closed Bug 1654452 Opened 2 months ago Closed 2 months ago

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

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox80 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(3 files)

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.

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.)

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.)

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.

Attachment #9165275 - Attachment description: Bug 1654452: Manage nsSharedPageData lifetime using UniquePtr instead of new/delete. r?TYlin → Bug 1654452 part 1: Manage nsSharedPageData lifetime using UniquePtr instead of new/delete. r?TYlin
Summary: Use UniquePtr to manage nsSharedPageData lifetime → Use UniquePtr to manage nsSharedPageData lifetime (and a few other nearby cleanups)
Pushed by dholbert@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.