Use UniquePtr to manage nsSharedPageData lifetime (and a few other nearby cleanups)
Categories
(Core :: Printing: Output, task, P3)
Tracking
()
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.
Assignee | ||
Comment 1•5 years ago
|
||
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.)
Assignee | ||
Comment 2•5 years ago
|
||
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.)
Assignee | ||
Comment 3•5 years ago
•
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
Depends on D84463
Assignee | ||
Comment 5•5 years ago
|
||
Depends on D84534
Assignee | ||
Updated•5 years ago
|
Comment 7•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/34fa316a6b63
https://hg.mozilla.org/mozilla-central/rev/29e8f72d4e96
https://hg.mozilla.org/mozilla-central/rev/1c5c7e3874e6
Description
•