1449622 - Remove ActorRegistry actor

Status: NEW

(DevTools :: Framework, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

Actor registry was added to support adding actors at runtime.
It was only used by bootstrapless/addon-sdk addons.
DevTools is not using this, so it should be safe removing this.
  https://searchfox.org/mozilla-central/source/devtools/server/actors/actor-registry.js#32

This is the only add-on using "setupInChild":
  https://searchfox.org/mozilla-central/source/devtools/server/main.js#937

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1449622 - Remove unused ActorRegistryActor.

This actor was only used by bootstrapless addons to register new actors.

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

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1449622 - Remove unused DebuggerServer.setupInChild method.

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8963227 [details]
Bug 1449622 - Remove unused DebuggerServer.setupInChild method.

https://reviewboard.mozilla.org/r/232118/#review237608


Code analysis found 11 defects in this patch:
 - 11 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/startup/frame.js:23
(Diff revision 1)
>    // Encapsulate in its own scope to allows loading this frame script more than once.
>    (function() {
>      const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
>  
>      const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>      const { dumpn } = DevToolsUtils;

Error: 'dumpn' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:65
(Diff revision 1)
>    };
>    Services.obs.addObserver(onParent, "test:setupParent");
>  
> -  // Instanciate e10s machinery and call setupInChild
> +  // Instanciate e10s machinery and call setupInParent from the content process.
>    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
> -    DebuggerServer.setupInChild({
> +		Services.ppmm.loadFrameScript("data:text/javascript,new " + function FrameScriptScope() {

Error: Line 51 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:65
(Diff revision 1)
>    };
>    Services.obs.addObserver(onParent, "test:setupParent");
>  
> -  // Instanciate e10s machinery and call setupInChild
> +  // Instanciate e10s machinery and call setupInParent from the content process.
>    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
> -    DebuggerServer.setupInChild({
> +		Services.ppmm.loadFrameScript("data:text/javascript,new " + function FrameScriptScope() {

Error: Unexpected tab character. [eslint: no-tabs]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:65
(Diff revision 1)
>    };
>    Services.obs.addObserver(onParent, "test:setupParent");
>  
> -  // Instanciate e10s machinery and call setupInChild
> +  // Instanciate e10s machinery and call setupInParent from the content process.
>    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
> -    DebuggerServer.setupInChild({
> +		Services.ppmm.loadFrameScript("data:text/javascript,new " + function FrameScriptScope() {

Error: Expected indentation of 4 spaces but found 2 tabs. [eslint: indent-legacy]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:67
(Diff revision 1)
>  
> -  // Instanciate e10s machinery and call setupInChild
> +  // Instanciate e10s machinery and call setupInParent from the content process.
>    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
> -    DebuggerServer.setupInChild({
> -      module: "chrome://mochitests/content/chrome/devtools/server/tests/mochitest/setup-in-child.js",
> -      setupChild: "setupChild",
> +		Services.ppmm.loadFrameScript("data:text/javascript,new " + function FrameScriptScope() {
> +      /* eslint-disable no-shadow */
> +      const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});

Error: Expected indentation of 2 spaces but found 6. [eslint: indent-legacy]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:68
(Diff revision 1)
> -  // Instanciate e10s machinery and call setupInChild
> +  // Instanciate e10s machinery and call setupInParent from the content process.
>    DebuggerServer.connectToFrame(conn, iframe).then(actor => {
> -    DebuggerServer.setupInChild({
> -      module: "chrome://mochitests/content/chrome/devtools/server/tests/mochitest/setup-in-child.js",
> -      setupChild: "setupChild",
> -      args: [1, "two", {three: true}]
> +		Services.ppmm.loadFrameScript("data:text/javascript,new " + function FrameScriptScope() {
> +      /* eslint-disable no-shadow */
> +      const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
> +      const { DebuggerServer } = require("devtools/server/main");

Error: Expected indentation of 2 spaces but found 6. [eslint: indent-legacy]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:71
(Diff revision 1)
> -      module: "chrome://mochitests/content/chrome/devtools/server/tests/mochitest/setup-in-child.js",
> -      setupChild: "setupChild",
> -      args: [1, "two", {three: true}]
> +      /* eslint-disable no-shadow */
> +      const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
> +      const { DebuggerServer } = require("devtools/server/main");
> +      /* eslint-enable no-shadow */
> +
> +      /* Hack! Fetch DebuggerServerConnection objects directly within DebuggerServer guts. */

Error: Line 57 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:72
(Diff revision 1)
> -      setupChild: "setupChild",
> -      args: [1, "two", {three: true}]
> +      const { require } = ChromeUtils.import("resource://devtools/shared/Loader.jsm", {});
> +      const { DebuggerServer } = require("devtools/server/main");
> +      /* eslint-enable no-shadow */
> +
> +      /* Hack! Fetch DebuggerServerConnection objects directly within DebuggerServer guts. */
> +      for (let id in DebuggerServer._connections) {

Error: Expected indentation of 2 spaces but found 6. [eslint: indent-legacy]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:73
(Diff revision 1)
> -      args: [1, "two", {three: true}]
> +      const { DebuggerServer } = require("devtools/server/main");
> +      /* eslint-enable no-shadow */
> +
> +      /* Hack! Fetch DebuggerServerConnection objects directly within DebuggerServer guts. */
> +      for (let id in DebuggerServer._connections) {
> +        let conn = DebuggerServer._connections[id];

Error: 'conn' is already declared in the upper scope. [eslint: no-shadow]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:80
(Diff revision 1)
> +          module: "chrome://mochitests/content/chrome/devtools/server/tests/mochitest/setup-in-parent.js",
> +          setupParent: "setupParent",
> +          args: [{one: true}, 2, "three"]
> -    });
> +        });
> +      }
> +		}, false);

Error: Unexpected tab character. [eslint: no-tabs]

::: devtools/server/tests/mochitest/test_setupInParentChild.html:80
(Diff revision 1)
> +          module: "chrome://mochitests/content/chrome/devtools/server/tests/mochitest/setup-in-parent.js",
> +          setupParent: "setupParent",
> +          args: [{one: true}, 2, "three"]
> -    });
> +        });
> +      }
> +		}, false);

Error: Expected indentation of 0 spaces but found 2 tabs. [eslint: indent-legacy]

Comment 4

[Alexandre Poirot [:ochameau]]

Oh. I missed this important usage:
  https://searchfox.org/mozilla-central/source/devtools/client/shared/test/test-actor-registry.js#19-29
That allows using this actor for tests:
  https://searchfox.org/mozilla-central/source/devtools/client/shared/test/test-actor.js

It was originally introduced for luciddream, in order to make tests be able to run against remote targets and not only firefox tabs.
Using an actor/front is quite similar to using ContentTask, except that it can work against any target.

* Is it worth keeping this complex ActorRegistry for that?
* Can we somehow still use actors for tests but with a simplier code?
* Is it worth keeping actors/fronts instead of ContentTask?
  There is only 5 tests using this test actor.
  In bug 1444064 I would like to have ContentTask, but connected to a browser toolbox, may be that's a better/easier pattern?

Comment 5

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

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> It was originally introduced for luciddream, in order to make tests be able
> to run against remote targets and not only firefox tabs.
> Using an actor/front is quite similar to using ContentTask, except that it
> can work against any target.

Alex and I discussed this during a meeting today.

We no longer see the value of this TestActor approach and would prefer offering ContentTask-like approaches for future testing needs.

In addition, setupInChild and ActorRegistry are hard to justify for just this one thing alone.  So, assuming only a few tests are involved, we should convert them away from TestActor and then remove these things.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) (on PTO May 10 - 14) from comment #5)
> So, assuming only a few tests are involved, we should
> convert them away from TestActor and then remove these things.
When luciddream was current, I spent quite a bit of time converting a lot of inspector tests to the TestActor. So, this won't be an easy conversion.