1356036 - PerformanceWatcher-content.js and PerformanceWatcher.jsm are unreferenced

Status: RESOLVED DUPLICATE of bug 1406872

(Toolkit :: Performance Monitoring, enhancement)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

about:performance is being redesigned; mass closing the bugs related to parts of the current about:performance page that we are not keeping.

Our goals with the redesign are to reduce the overhead caused by having the page opened, increase the reliability of the displayed information, and make the offered information actionable for most users. The back-end work is being tracked in bug 1419681.

Comment 2

[Florian Quèze [:florian]]

Err, we are redesigning about:performance, but code that was already dead is still dead; we still want to remove it.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

The last reference to these files was removed in bug 1309946.

Comment 4

[:Gijs (he/him)]

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 .

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2

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

Comment 6

[:Gijs (he/him)]

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?

Comment 7

[Florian Quèze [:florian]]

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

Comment 8

[:Gijs (he/him)]

What's the status of this bug?

Comment 9

[Florian Quèze [: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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86cbcf2195f9
remove unreferenced PerformanceWatcher-content.js and PerformanceWatcher.jsm, r=Gijs.

Comment 11

[Noemi Erli[:noemi_erli]]

Backed out changeset 86cbcf2195f9 (Bug 1356036) for browser-chrome failures in browser_all_files_referenced.js CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=86cbcf2195f9d41aae9560c4028785d95003a86d&searchStr=browser-chrome

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=220827578&repo=mozilla-inbound&lineNumber=2711

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b52489bddb94bdb2d6fe88ed639d5c35cb2f5c7e

Comment 12

[Florian Quèze [:florian]]

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.