Closed Bug 1391228 Opened 5 years ago Closed 5 years ago
Check if devtools/shared/fronts/profiler
.js can be removed
59 bytes, text/x-review-board-request
I think that devtools/shared/fronts/profiler.js is no longer used in devtools. It is only imported in two tests: - devtools/server/tests/unit/test_profiler_events-01.js - devtools/server/tests/unit/test_profiler_events-02.js When looking at code coverage for this file: https://codecov.io/gh/marco-c/gecko-dev/src/master/devtools/shared/fronts/profiler.js we can see it is only imported twice, which most likely corresponds to the 2 tests mentioned above.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
While discussing this with Sole, she found https://bugzilla.mozilla.org/show_bug.cgi?id=1265727#c13 which seems to indicate that the file was already considered for removal when we moved fronts to shared. It was kept for potential b2g compatibility, but I think we can safely remove it now.
I have not looked through the code to verify the assumptions listed here, but I would say that anything kept around for legacy reasons like b2g would be a good candidate for removal, especially with our future plans to integrate perf.html with devtools.
Shouldn't we also be able to remove the spec and actor corresponding to this front? It appears we may not be able to from https://dxr.mozilla.org/mozilla-central/source/devtools/server/main.js#551-562, but I don't know why we are using the actor but not the front.
Maybe it can be removed from devtools/server/main after all, there aren't other references that I can see (except for some Servo actors - and I'm not sure if those are currently working and/or would break from the front removal): https://dxr.mozilla.org/mozilla-central/search?q=PerformanceActor&redirect=true. Victor, do you know if the PerformanceActor is only around for backwards compatibility or for some other reason?
(In reply to Brian Grinstead [:bgrins] from comment #7) > Shouldn't we also be able to remove the spec and actor corresponding to this > front? It appears we may not be able to from > https://dxr.mozilla.org/mozilla-central/source/devtools/server/main.js#551- > 562, but I don't know why we are using the actor but not the front. Thanks! Looks like you are right, we can probably get rid of the actor, specs and related tests. We have a bunch of tests in devtools based on `target.hasActor("profiler")`  but we can probably replace that with target.hasActor("performance")  https://dxr.mozilla.org/mozilla-central/search?q=hasActor(%22profiler%22)&redirect=false
Comment on attachment 8898832 [details] Bug 1391228 - remove unused devtools profiler actor, specs and front; Clearing the review request for now, waiting for Victor's feedback. Will attach a new patch after that.
Attachment #8898832 - Flags: review?(gtatum)
Updated patch removing actor, spec and front. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b0d0b6196efcff9996d299eb344dbc4d03a325c
(In reply to Brian Grinstead [:bgrins] from comment #8) > Maybe it can be removed from devtools/server/main after all, there aren't > other references that I can see (except for some Servo actors - and I'm not > sure if those are currently working and/or would break from the front > removal): > https://dxr.mozilla.org/mozilla-central/ > search?q=PerformanceActor&redirect=true. > Asked in #servo about their Profiler actors. Found a few dependencies additional to the Profiler actor in xpcshell tests: - devtools/server/tests/unit/test_profiler_activation-01.js - devtools/server/tests/unit/test_profiler_activation-02.js - devtools/server/tests/unit/test_profiler_bufferstatus.js - devtools/server/tests/unit/test_profiler_close.js - devtools/server/tests/unit/test_profiler_data.js - devtools/server/tests/unit/test_profiler_getbufferinfo.js - devtools/server/tests/unit/test_profiler_getfeatures.js - devtools/server/tests/unit/test_profiler_sharedlibraries.js Markus: looking at the tests, they seem to only be asserting the profiler actor itself (response type is correct etc...), so I don't think they provide valuable coverage to the underlying profiler. Can you have a look and let me know if you're fine with us removing them?
Try is green at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a725096c0eb37a3c4448e4649f0adb4a192fb497 Markus: clearing ni? as I flagged you for review (mostly to make sure deleting our tests is ok)
Comment on attachment 8898832 [details] Bug 1391228 - remove unused devtools profiler actor, specs and front; https://reviewboard.mozilla.org/r/170214/#review179652 Looks good to me. It's a little concerning that we don't have unit tests for the performance actor, but this patch doesn't really affect that one way or the other. The performance panel has full integration tests that actually run the profiler. The Gecko Profiler mainly uses these tests as sanity checks, so I don't think removing these will be an issue on that front.
Attachment #8898832 - Flags: review?(gtatum) → review+
Comment on attachment 8898832 [details] Bug 1391228 - remove unused devtools profiler actor, specs and front; https://reviewboard.mozilla.org/r/170214/#review179940 The last use of the profiler actor was here: https://github.com/devtools-html/Gecko-Profiler-Addon/blob/4e14f1447f58197c127aff476d82d95da4ca8500/lib/remote-profiler.js#L14 It was used in the pre-webextension Gecko profiler add-on in order to control the profiler in Firefox for Android. But this code is now gone, because there is no webextension API to connect to a remote debugging target, and as a result, the Gecko profiler add-on can't be used to profile Firefox for Android. Once the relevant webextensions APIs are added, we will probably need to add some new methods to the performance actor, in order to get the information that the Gecko Profiler needs.
Attachment #8898832 - Flags: review?(mstange) → review+
Thanks for the reviews! Clearing the ni? for Victor as I believe that the answer from Markup clarifies the initial question.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 5e0fffb16aff -d e066d2f8f62d: rebasing 417322:5e0fffb16aff "Bug 1391228 - remove unused devtools profiler actor, specs and front;r=gregtatum,mstange" (tip) local [dest] changed devtools/server/actors/profiler.js which other [source] deleted use (c)hanged version, (d)elete, or leave (u)nresolved? u local [dest] changed devtools/shared/fronts/profiler.js which other [source] deleted use (c)hanged version, (d)elete, or leave (u)nresolved? u merging devtools/client/framework/toolbox.js merging devtools/docs/backend/backward-compatibility.md merging devtools/server/actors/moz.build merging devtools/server/main.js merging devtools/shared/fronts/moz.build merging devtools/shared/specs/moz.build unresolved conflicts (see hg resolve, then hg rebase --continue)
Random errors on my try push. I don't believe they are related but started some retriggers.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/bf8dcf287da7 remove unused devtools profiler actor, specs and front;r=gregtatum,mstange
You need to log in before you can comment on or make changes to this bug.