1776074 - Code-cleanup around nsPrintObject initialization code

Status: RESOLVED FIXED

(Core :: Printing: Output, task)

Description

[Daniel Holbert [:dholbert]]

I noticed some code-cleanup opportunities around nsPrintObject.

The main cleanup is: the "InitAs..." methods are effectively infallible, and should just be converted into constructors. I'll post a few patches as well to do supporting/neighboring cleanup as well.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 1: Remove unused 'aForPrintPreview' param from nsPrintObject::InitAsRootObject. r?#layout

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 2: Remove redundant assignment of mPrintObject->mFrameType. r?#layout

This patch doesn't change behavior.

The assignment here is workign with a freshly-constructed nsPrintObject,
initialized via InitAsRootObject. Its mFrameType will have already been
assigned to eDoc in the constructor. The only place in our codebase where we
reassign mFrameType is in InitAsNestedObject, which we're not calling here.

So, mFrameType will already be eDoc, and this assignment is unnecessary.

Depends on D150075

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 3: Merge nsPrintObject's Init methods into one method. r?#layout

This patch doesn't change behavior.

The two init functions already shared a lot of code. This patch merges them,
and uses the "aParent" arg as a signal for which variant we're handling (with
the "root" variant being signalled via a null value).

This is a valid conversion, as long as we can assume that the "nested" version
always got a non-null value for its aParent arg (which is then a signal that
we're using the nested version of the function). And indeed, we're safe to
make this assumption, since there's only one caller of the "nested" version
(nsPrintJob::BuildNestedPrintObjects), and it dereferences the pointer before
calling into this init function. So, the pointer must be non-null or else we
would have crashed.

(Spoiler alert: the next patch in this series will merge this Init() method
into the constructor. I'm doing this as a multi-step process with this
intermediate state, in order to hopefully make it easier to reason about the
conversion and confirm that it's valid.)

Depends on D150076

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 4: Use references instead of pointers for nsPrintObject::Init's Document and nsDocShell args (and in callers). r?#layout

I've added code-comments to justify why we know these args are non-null. The
args are null-checked by the caller, except in one case
(nsPrintJob::DoCommonPrint) aDoc is not null-checked by the caller. But
fortunately, it's null-checked higher up in the call-stack, so I simply
propagated the NotNull annotaiton up to that point for additional clarity and
grounding-in-reality.

Depends on D150077

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 5: Merge nsPrintObject's effectively-infallible Init function into constructor. r?#layout

This patch doesn't change behavior; it's just refactoring.

The pre-existing Init code is now clearly infallible, now that earlier patches
have removed all of Init's NS_ENSURE_STATE arg-null-checks (which were Init's
only failure-returning codepaths).

So: now that it's infallible, we can just merge the Init() code directly into
the constructor.

This lets us promote some member-variables to be 'const' as well, since they
will now be initialized in the init list and are never modified after that.

Depends on D150078

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 0: Mark nsPrintObject as 'final' and drop irrelevant comment about its destructor being non-virtual. r?#layout

It has no parent or child classes, so there's no reason that any of its methods
(including its destructor) would be virtual. So it's more-confusing than
helpful to have an explicit "non-virtual" code-comment.

The final keyword on the class hopefully makes this more-clear.

Comment 7

[Daniel Holbert [:dholbert]]

One final simplification that I haven't yet made but intend to make (tomorrow): we can get rid of nsPrintObject::mFrameType entirely and just use its mParent pointer, since that pointer's nullness carries the same information.

(mFrameType just indicates whether we're a root or not, and mParent tells us that just as clearly.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 6: Simplify nsPrintObject init list. r?#layout

Move two assignments to the declaration site, and remove an unnecessary
nullptr assignment (for a nsCOMPtr which is nullptr by default).

Depends on D150079

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1776074 part 7: Remove mFrameType member (and PrintObjectType in general) from nsPrintObject.h. r?#layout

This patch does not change behavior (other than a minor correctness fix in some
off-by-default logging; see below).

As shown in the removed line of the init list, this mFrameType member-var's
semantics are equivalent to a null-check of the mParent member-var. So: rather
than encoding that same information twice, this patch simplifies to just
directly null-check the mParent pointer at all the usage sites.

Before this patch, nsPrintJob.cpp had two different patterns for logging
mFrameType; sometimes with a global gFrameTypesStr array, and other times
with a local types array (with shorter 2-character strings). I've converted
both of these to helper-functions.

In the case of the types array, the old code used a 4-value array, which was
interesting since the enum type only had 2 possible values. This discrepancy is
just due to an oversight in bug 1769508, where we recently condensed the enum
from 4 values to 2; that bug technically should've condensed these arrays as
well (but didn't do so). This left these arrays' enum-to-string mapping being
wrong (since eIFrame changed its numeric value from 2 to 1 in bug 1769508), but
probably nobody has used this logging code in a while, so nobody
noticed. Anyway: in this patch, I'm restoring the mappings that we had before
that change (so we'll log "DC" for root print objects and "IF" for non-root
i.e. iframe-flavored print objects).

Depends on D150176

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebc0391f635d
part 0: Mark nsPrintObject as 'final' and drop irrelevant comment about its destructor being non-virtual. r=emilio
https://hg.mozilla.org/integration/autoland/rev/db7501e4488f
part 1: Remove unused 'aForPrintPreview' param from nsPrintObject::InitAsRootObject. r=emilio
https://hg.mozilla.org/integration/autoland/rev/37971cb5cdf3
part 2: Remove redundant assignment of mPrintObject->mFrameType. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9300e4384683
part 3: Merge nsPrintObject's Init methods into one method. r=emilio
https://hg.mozilla.org/integration/autoland/rev/26b0fba51cce
part 4: Use references instead of pointers for nsPrintObject::Init's Document and nsDocShell args (and in callers). r=emilio
https://hg.mozilla.org/integration/autoland/rev/f434006095ee
part 5: Merge nsPrintObject's effectively-infallible Init function into constructor. r=emilio
https://hg.mozilla.org/integration/autoland/rev/21c91e3a70c4
part 6: Simplify nsPrintObject init list. r=emilio
https://hg.mozilla.org/integration/autoland/rev/0b77e8d3a3c1
part 7: Remove mFrameType member (and PrintObjectType in general) from nsPrintObject.h. r=emilio

Comment 11

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/ebc0391f635d
https://hg.mozilla.org/mozilla-central/rev/db7501e4488f
https://hg.mozilla.org/mozilla-central/rev/37971cb5cdf3
https://hg.mozilla.org/mozilla-central/rev/9300e4384683
https://hg.mozilla.org/mozilla-central/rev/26b0fba51cce
https://hg.mozilla.org/mozilla-central/rev/f434006095ee
https://hg.mozilla.org/mozilla-central/rev/21c91e3a70c4
https://hg.mozilla.org/mozilla-central/rev/0b77e8d3a3c1