1474980 - Remove unused DebuggerServer.addActors method

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1474980 - Remove deprecated DebuggerServer.addActors().

Review commit: https://reviewboard.mozilla.org/r/256454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256454/

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1474980 - Stop exporting ActorPool/OriginalLocation from server/main.js.

Review commit: https://reviewboard.mozilla.org/r/256456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256456/

Comment 3

[Alexandre Poirot [:ochameau]]

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

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8991547 [details]
Bug 1474980 - Stop exporting ActorPool/OriginalLocation from server/main.js.

https://reviewboard.mozilla.org/r/256456/#review263470

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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?

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8991546 [details]
Bug 1474980 - Remove deprecated DebuggerServer.addActors().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256454/diff/1-2/

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8991547 [details]
Bug 1474980 - Stop exporting ActorPool/OriginalLocation from server/main.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256456/diff/1-2/

Comment 8

[Alexandre Poirot [:ochameau]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/421312ca4dd5
https://hg.mozilla.org/mozilla-central/rev/be3fe24cb43f