Closed Bug 1452170 Opened 2 years ago Closed 2 years ago

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


(Core :: Layout, enhancement)

Not set



Tracking Status
firefox61 --- fixed


(Reporter: jwatt, Assigned: jwatt)



(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]

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:

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
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]

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

Please remove RestyleLogging.h from layout/base/ too.
Attachment #8966116 - Flags: review?(cam) → review+
Pushed by
Remove RestyleLogging.h, RestyleTrackerInlines.h and related RESTYLE_LOGGING cruft. r=heycam
Pushed by
follow-up - Fix Windows unified build bustage by adding missing AutoRestore.h includes. r=me on CLOSED TREE
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.