Closed Bug 1356036 Opened 4 years ago Closed 2 years ago

PerformanceWatcher-content.js and PerformanceWatcher.jsm are unreferenced

Categories

(Toolkit :: Performance Monitoring, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1406872

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file, 1 obsolete file)

resource://gre/modules/PerformanceWatcher-content.js and resource://gre/modules/PerformanceWatcher.jsm are referencing each other, but aren't referenced anywhere else. If we are not using them, we should stop shipping them.
Attached patch Patch (obsolete) — Splinter Review
The last reference to these files was removed in bug 1309946.
Attachment #8991843 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → florian
Status: REOPENED → ASSIGNED
Comment on attachment 8991843 [details] [diff] [review]
Patch

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

This needs to also remove toolkit/components/perfmonitoring/tests/browser/browser_webpagePerformanceAlerts.js , the reference for that in browser.ini, and the references in toolkit/components/perfmonitoring/PerformanceStats.jsm .
Attachment #8991843 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch Patch v2Splinter Review
(In reply to :Gijs (he/him) from comment #4)

> This needs to also remove
> toolkit/components/perfmonitoring/tests/browser/
> browser_webpagePerformanceAlerts.js , the reference for that in browser.ini,
> and the references in toolkit/components/perfmonitoring/PerformanceStats.jsm

Thanks for catching this. And then the CPUBurner part of head.js can also go.
Attachment #8992290 - Flags: review?(gijskruitbosch+bugs)
Attachment #8991843 - Attachment is obsolete: true
Comment on attachment 8992290 [details] [diff] [review]
Patch v2

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

rs=me if we're sure we don't want to use any of this for the "new" about:performance work. IIRC I was told it was still potentially relevant. Are we going to remove the JS counters that feed into this stuff, too?
Attachment #8992290 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #6)

> rs=me if we're sure we don't want to use any of this for the "new"
> about:performance work.

Thanks, I'll think about it some more before landing.

> IIRC I was told it was still potentially relevant.
> Are we going to remove the JS counters that feed into this stuff, too?

I don't know. I'll figure this out once both Tarek and Yoric are back from PTO.
What's the status of this bug?
Flags: needinfo?(florian)
(In reply to :Gijs (he/him) from comment #8)
> What's the status of this bug?

I think the current plan is to land it as part of bug 1406872.
Flags: needinfo?(florian)
See Also: → 1406872
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86cbcf2195f9
remove unreferenced PerformanceWatcher-content.js and PerformanceWatcher.jsm, r=Gijs.

The failure was because I removed 2 unreferenced files and the related exception, but there were 2 related files that were already reported as "indirectly whitelisted file", that became unreferenced and that the test started reporting.

So the test is working as expected, and I just didn't pay enough attention when landing here. This has now relanded as part of bug 1406872, so no need to keep this bug open.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago2 years ago
Flags: needinfo?(florian)
Resolution: --- → DUPLICATE
Duplicate of bug: 1406872
You need to log in before you can comment on or make changes to this bug.