Closed Bug 1474980 Opened 2 years ago Closed 2 years ago

Remove unused DebuggerServer.addActors method

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files)

Bug 1473578 already remove some actor definition helpers that were only used by tests.
But there is also DebuggerServer.addActors():
  https://searchfox.org/mozilla-central/search?q=.addActors(&case=true&regexp=false&path=
that is used only by tests, whereas it justify keeping a bunch of very old and cryptic code in server/main.js because all of this rely on loadSubScript and mixes the scope of main.js with all the test actors.

We could remove all of that:
https://searchfox.org/mozilla-central/source/devtools/server/main.js#35-42
https://searchfox.org/mozilla-central/source/devtools/server/main.js#1350-1357
Assignee: nobody → poirot.alex
Blocks: 1473828
Blocks: dbg-server
Some useful notes about this:
* this may have removed the last cases where prefix was different from the attribute name used on RootActor.listTab/BrowsingContext.form.
* this removes yet another set of things that were put on DebuggerServer, but we may still have some other cases like this.
* I tested xpcshell debugger to ensure it still works.
* devtools/client/debugger/test/mochitest/browser_dbg_target-scoped-actor-0*.js only works with e10s disabled, so I imagine we no longer run these tests... But I verified that they still pass locally. DebuggerServer.addActors/registerModule don't work across processes.
* DebuggerServer.addGlobalActor/addTargetScopedActors is now only called by registerModule except for actors registered by constructor instead of modules (actor-registry-utils and some tests).
Severity: normal → enhancement
Priority: -- → P3
Comment on attachment 8991547 [details]
Bug 1474980 - Stop exporting ActorPool/OriginalLocation from server/main.js.

https://reviewboard.mozilla.org/r/256456/#review263470
Attachment #8991547 - Flags: review?(jryans) → review+
Comment on attachment 8991546 [details]
Bug 1474980 - Remove deprecated DebuggerServer.addActors().

https://reviewboard.mozilla.org/r/256454/#review263468

I assume mobile is okay too?

https://searchfox.org/mozilla-central/source/mobile/android/modules/dbg-browser-actors.js

Thanks for working on this! :)

::: devtools/client/debugger/test/mochitest/browser_dbg_globalactor.js:21
(Diff revision 1)
>    DebuggerServer.registerAllActors();
>  
> -  DebuggerServer.addActors(ACTORS_URL);
> +  DebuggerServer.registerModule(ACTORS_URL, {
> +    prefix: "testOne",
> +    constructor: "TestActor1",
> +    type: { global: true, target: true },

Does this test use it as a target-scoped actor?
Attachment #8991546 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> Comment on attachment 8991546 [details]
> Bug 1474980 - Remove deprecated DebuggerServer.addActors().
> 
> https://reviewboard.mozilla.org/r/256454/#review263468
> 
> I assume mobile is okay too?
> 
> https://searchfox.org/mozilla-central/source/mobile/android/modules/dbg-
> browser-actors.js

Yes, it is already loaded as module per a recent cleanup:
  https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/RemoteDebugger.js#208
But I built Fennec to verify it still works.

> ::: devtools/client/debugger/test/mochitest/browser_dbg_globalactor.js:21
> (Diff revision 1)
> >    DebuggerServer.registerAllActors();
> >  
> > -  DebuggerServer.addActors(ACTORS_URL);
> > +  DebuggerServer.registerModule(ACTORS_URL, {
> > +    prefix: "testOne",
> > +    constructor: "TestActor1",
> > +    type: { global: true, target: true },
> 
> Does this test use it as a target-scoped actor?

No, fixed!
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/421312ca4dd5
Remove deprecated DebuggerServer.addActors(). r=jryans
https://hg.mozilla.org/integration/autoland/rev/be3fe24cb43f
Stop exporting ActorPool/OriginalLocation from server/main.js. r=jryans
https://hg.mozilla.org/mozilla-central/rev/421312ca4dd5
https://hg.mozilla.org/mozilla-central/rev/be3fe24cb43f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1478944
You need to log in before you can comment on or make changes to this bug.