Closed Bug 1452170 Opened 2 years ago Closed 2 years ago

Remove RestyleLogging.h, RestyleTrackerInlines.h and related RESTYLE_LOGGING cruft

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jwatt, Assigned: jwatt)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Attachment #8965773 - Flags: review?(dholbert)
Comment on attachment 8965773 [details] [diff] [review]
patch

Is this just a "remove dead code" bug?  These functions aren't exactly dead -- they have callers in the tree (behind an #ifdef guard) here:
https://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleLogging.h

You should probably tag heycam to confirm that this really is dead code (since he's got hg blame on this code), and you should probably remove the callers before or at the same time as removing these APIs.
Attachment #8965773 - Flags: review?(dholbert) → review-
Looks like I should morph this into "Remove dead RESTYLE_LOGGING code".
Flags: needinfo?(emilio)
RestyleTrackerInlines.h isn't included by any file. RESTYLE_LOGGING isn't defined by anything.
(In reply to Jonathan Watt [:jwatt] from comment #4)
> RESTYLE_LOGGING isn't defined by anything.

Sure, but I assume the idea is/was that a developer would define it locally if they wanted extra logging.

> RestyleTrackerInlines.h isn't included by any file.

Still: probably better to remove the (#include-severed) callers from RestyleLogging.h at the same time.

(Hypothetically, if we want to keep those callers around, then the right solution here would be to sprinkle around some includes for RestyleTrackerInlines.h, rather than removing it.)
Right, sorry I wasn't clear. Those two points were meant as a summary (along with comment 3) for Emilio to consider whether he thinks this logging should be resurected or removed.
We have restyle logging with RUST_LOG=style, using the standard log mechanism. So I don't think it's worth resurrecting this code IMO.
Flags: needinfo?(emilio)
Yes, RestyleLogging.h can be removed.

(One slightly nice thing about the old logging code, compared to the RUST_LOG=style one, is that it indented the logging based on the depth of the restyle, which made it a bit easier to visually scan to see where restyles where being cut off and so on.  That's probably too tricky to do with the standard Rust logging mechanism, not to mention how it would work given work stealing / parallelism.  In any case, that wouldn't involve RestyleLogging.h.)
To be clear, it would be all the RESTYLE_LOGGING code I'd remove, not just RestyleLogging.h

https://searchfox.org/mozilla-central/search?q=RESTYLE_LOGGING
Yeah, all that can go.
Attached patch patchSplinter Review
Attachment #8965773 - Attachment is obsolete: true
Attachment #8966116 - Flags: review?(cam)
Summary: Remove RestyleTrackerInlines.h → Remove RestyleLogging.h, RestyleTrackerInlines.h and related RESTYLE_LOGGING cruft
Comment on attachment 8966116 [details] [diff] [review]
patch

Review of attachment 8966116 [details] [diff] [review]:
-----------------------------------------------------------------

Please remove RestyleLogging.h from layout/base/moz.build too.
Attachment #8966116 - Flags: review?(cam) → review+
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f892607541eb
Remove RestyleLogging.h, RestyleTrackerInlines.h and related RESTYLE_LOGGING cruft. r=heycam
Pushed by jwatt@jwatt.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3a7a21dedd
follow-up - Fix Windows unified build bustage by adding missing AutoRestore.h includes. r=me on CLOSED TREE
https://hg.mozilla.org/mozilla-central/rev/f892607541eb
https://hg.mozilla.org/mozilla-central/rev/5c3a7a21dedd
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.