1774894 - nsDisplayTableItem's mPartHasFixedBackground and UpdateForFrameBackground (and mDrawsBackground) seem to be obsolete/dead code

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Following up on a review comment from emilio in https://phabricator.services.mozilla.com/D149337 (which was a downstream refactoring that happened to adjusted a callsite in nsDisplayTableItem::UpdateForFrameBackground) :

1. It looks to me like nsDisplayTableItem::UpdateForFrameBackground is never called and can be removed.

2. That means the member-variable it sets, mPartHasFixedBackground, is always false and can be assumed to be such, and hence can be removed.

3. That means the nsDisplayTableItem's other member-variable, mDrawsBackground, is also unnecessary since it's only used once the following condition:

  if (mDrawsBackground && mPartHasFixedBackground) {

...which we know (per (2)) is just an if (... && false) check and as-such is dead code.

There may be further simplifications to be made as well.

As part of fixing this patch, we should probably double-check that it's OK/expected for this code to all be dead & removed... (Probably it was obsoleted by moving our table painting code to use display lists or something like that?)

Comment 1

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

(In reply to Daniel Holbert [:dholbert] from comment #0)

As part of fixing this patch, we should probably double-check that it's OK/expected for this code to all be dead & removed... (Probably it was obsoleted by moving our table painting code to use display lists or something like that?)

Yeah, looks like it.

UpdateForFrameBackground and mPartHasFixedBackground were added in bug 416735; and at that point, UpdateForFrameBackground did have several callsites, in nsTableFrame::DisplayGenericTablePart as well as in nsTableCellFrame::BuildDisplayList:
https://hg.mozilla.org/mozilla-central/rev/7c0e2f9dec6f56692f92daf3efcd5385487bb587#l4.106

And:

• DisplayGenericTablePart no longer exists (though we have a code-comment that references it)
• And nsTableCellFrame::BuildDisplayList does still exist, but it doesn't call UpdateForFrameBackground anymore. It looks like its callsites went away in https://hg.mozilla.org/integration/mozilla-inbound/rev/93acd240b578#l4.44 in bug 929484.

So this is likely-valid cleanup for left-over code that's cruft as-of bug 929484 & associated work.

Comment 2

[Daniel Holbert [:dholbert] (vacation until Jun 23)]

Attachment: Bug 1774894: Remove unused function nsDisplayTableItem::UpdateForFrameBackground and its associated dead-code. r?#layout-reviewers

This patch doesn't affect behavior; it's dead-code removal.

UpdateForFrameBackground is never called, and hence can be removed.

The member-var that it conditionally set, mPartHasFixedBackground,
can be assumed to be always false, and hence can be removed.

The same struct's other member-var, mDrawsBackground, is irrelevant since it's
only checked in combination with mPartHasFixedBackground, and hence can be
removed.

This all seems to be code that's obsoleted by our (long-ago) move to using
display lists to draw tables.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf46db9b7d3b
Remove unused function nsDisplayTableItem::UpdateForFrameBackground and its associated dead-code. r=emilio

Comment 4

[Cristian Tuns]

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