Bug 1654763 - [devtools] Remove implementation, tests and documentation for actorHasMethod r=rcaliman,ochameau,devtools-backward-compat-reviewers

actorhasMethod had several technical limitations making it hard to use consistently. We now removed all the call sites for this method. This changeset removes the method and all its dependencies.

Differential Revision: https://phabricator.services.mozilla.com/D95861

devtools/client/framework/test/browser_target_support.js

@@ -1,8 +1,7 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
-// Test support methods on Target, such as `hasActor`, `getActorDescription`,
-// `actorHasMethod` and `getTrait`.
+// Test support methods on Target, such as `hasActor` and `getTrait`.
 
 async function testTarget(client, target) {
   await target.attach();
@@ -22,58 +21,6 @@ async function testTarget(client, target) {
     false,
     "target.hasActor() false when actor does not exist."
   );
-  // Create a front to ensure the actor is loaded
-  await target.getFront("storage");
-
-  let desc = await target.getActorDescription("storage");
-  is(
-    desc.typeName,
-    "storage",
-    "target.getActorDescription() returns definition data for corresponding actor"
-  );
-  is(
-    desc.events["stores-update"].type,
-    "storesUpdate",
-    "target.getActorDescription() returns event data for corresponding actor"
-  );
-
-  desc = await target.getActorDescription("nope");
-  is(
-    desc,
-    undefined,
-    "target.getActorDescription() returns undefined for non-existing actor"
-  );
-  desc = await target.getActorDescription();
-  is(
-    desc,
-    undefined,
-    "target.getActorDescription() returns undefined for undefined actor"
-  );
-
-  let hasMethod = await target.actorHasMethod("storage", "listStores");
-  is(
-    hasMethod,
-    true,
-    "target.actorHasMethod() returns true for existing actor with method"
-  );
-  hasMethod = await target.actorHasMethod("localStorage", "nope");
-  is(
-    hasMethod,
-    false,
-    "target.actorHasMethod() returns false for existing actor with no method"
-  );
-  hasMethod = await target.actorHasMethod("nope", "nope");
-  is(
-    hasMethod,
-    false,
-    "target.actorHasMethod() returns false for non-existing actor with no method"
-  );
-  hasMethod = await target.actorHasMethod();
-  is(
-    hasMethod,
-    false,
-    "target.actorHasMethod() returns false for undefined params"
-  );
 
   is(
     target.getTrait("giddyup"),

devtools/client/fronts/targets/target-mixin.js

@@ -211,54 +211,6 @@ function TargetMixin(parentClass) {
       );
     }
 
-    /**
-     * Returns a promise for the protocol description from the root actor. Used
-     * internally with `target.actorHasMethod`. Takes advantage of caching if
-     * definition was fetched previously with the corresponding actor information.
-     * Actors are lazily loaded, so not only must the tool using a specific actor
-     * be in use, the actors are only registered after invoking a method (for
-     * performance reasons, added in bug 988237), so to use these actor detection
-     * methods, one must already be communicating with a specific actor of that
-     * type.
-     *
-     * @return {Promise}
-     * {
-     *   "category": "actor",
-     *   "typeName": "longstractor",
-     *   "methods": [{
-     *     "name": "substring",
-     *     "request": {
-     *       "type": "substring",
-     *       "start": {
-     *         "_arg": 0,
-     *         "type": "primitive"
-     *       },
-     *       "end": {
-     *         "_arg": 1,
-     *         "type": "primitive"
-     *       }
-     *     },
-     *     "response": {
-     *       "substring": {
-     *         "_retval": "primitive"
-     *       }
-     *     }
-     *   }],
-     *  "events": {}
-     * }
-     */
-    async getActorDescription(actorName) {
-      if (
-        this._protocolDescription &&
-        this._protocolDescription.types[actorName]
-      ) {
-        return this._protocolDescription.types[actorName];
-      }
-      const description = await this.client.mainRoot.protocolDescription();
-      this._protocolDescription = description;
-      return description.types[actorName];
-    }
-
     /**
      * Returns a boolean indicating whether or not the specific actor
      * type exists.
@@ -273,33 +225,6 @@ function TargetMixin(parentClass) {
       return false;
     }
 
-    /**
-     * Queries the protocol description to see if an actor has
-     * an available method. The actor must already be lazily-loaded (read
-     * the restrictions in the `getActorDescription` comments),
-     * so this is for use inside of tool. Returns a promise that
-     * resolves to a boolean.
-     *
-     * @param {String} actorName
-     * @param {String} methodName
-     * @return {Promise}
-     */
-    actorHasMethod(actorName, methodName) {
-      return this.getActorDescription(actorName).then(desc => {
-        if (!desc) {
-          console.error(
-            `Actor "${actorName}" was not found in the protocol description.
-            Ensure you used the correct typename and that the actor is initialized.`
-          );
-        }
-
-        if (desc?.methods) {
-          return !!desc.methods.find(method => method.name === methodName);
-        }
-        return false;
-      });
-    }
-
     /**
      * Returns a trait from the root actor.
      *

devtools/docs/backend/backward-compatibility.md

@@ -10,11 +10,11 @@ In general, we should strive to maintain feature support for existing servers as
 
 The important compatibility scenarios are:
 
-1. Nightly desktop client **MUST** maintain existing compatibility back to release channel servers.
+- Nightly desktop client **MUST** maintain existing compatibility back to release channel servers.
 
 This is mainly to simplify cross-platform use cases, i.e. desktop Nightly with release Fennec.
 
-2. Servers **MAY** use traits to state a feature is not supported yet.
+- Servers **MAY** use traits to state a feature is not supported yet.
 
 This helps us support alternate environments, which does not implement every possible server feature.
 
@@ -28,9 +28,11 @@ The easiest way to test this is to check your work against a Firefox for Android
 
 ## Feature Detection
 
-Starting with Firefox 36 (thanks to [bug 1069673](https://bugzilla.mozilla.org/show_bug.cgi?id=1069673)), you can use actor feature detection to determine which actors exist and what methods they expose.
+Starting with Firefox 36 (thanks to [bug 1069673](https://bugzilla.mozilla.org/show_bug.cgi?id=1069673)), you can use actor feature detection to determine which actors exist.
 
-1. Detecting if the server has an actor: all you need is access to the `Toolbox` instance, which all panels do, when they get instantiated. Then you can do:
+### Target hasActor helper
+
+Detecting if the server has an actor: all you need is access to the `Toolbox` instance, which all panels do, when they get instantiated. Then you can do:
 
 ```js
 let hasPerformanceActor = toolbox.target.hasActor("performance");
@@ -38,15 +40,24 @@ let hasPerformanceActor = toolbox.target.hasActor("performance");
 
 The `hasActor` method returns a boolean synchronously.
 
-2. Detecting if an actor has a given method: same thing here, you need access to the toolbox:
+### Traits
 
-```js
-toolbox.target.actorHasMethod("domwalker", "duplicateNode").then(hasMethod => {
+Expose traits on an Actor in order to flag certain features as available or not. For instance if a new method "someMethod" is added to an Actor, expose a "supportsSomeMethod" flag in the traits object for the Actor, set to true. When debugging older servers, the flag will be missing and will default to false.
 
-}).catch(console.error);
-```
+Traits need to be forwarded to the client, and stored or used by the corresponding Front. There is no unique way of exposing traits, but there are still a few typical patterns found in the codebase.
 
-The `actorHasMethod` returns a promise that resolves to a boolean.
+For Actors using a "form()" method, for which the Front is automatically created by protocol.js, the usual pattern is to add a "traits" property to the form, that contains all the traits for the actor. The Front can then read the traits in its corresponding "form()" method. Example:
+- [NodeActor form method](https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/devtools/server/actors/inspector/node.js#209)
+- [NodeFront form method](https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/devtools/client/fronts/node.js#145)
+
+For other Actors, there are two options. First option is to define the trait on the Root actor. Those traits will be available both via TargetMixin::getTrait(), and on DevToolsClient.traits. The second option is to implement a "getTraits()" method on the Actor, which will return the traits for the Actor. Example:
+- [CompatibilityActor getTraits method](https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/devtools/shared/specs/compatibility.js#40)
+- [CompatibilitySpec getTraits definition](https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/devtools/shared/specs/compatibility.js#40-43)
+- [CompatibilityFront getTraits method](https://searchfox.org/mozilla-central/rev/e75e8e5b980ef18f4596a783fbc8a36621de7d1e/devtools/client/fronts/compatibility.js#41-47)
+
+Ironically, "getTraits" needs to be handled with backwards compatibility. But there is no way to check that "getTraits" is available on the server other than performing a try catch around the method. See the CompatibilityFront example.
+
+Whenever traits are added, make sure to add a relevant backward compatibility comment so that we know when the trait can be removed.
 
 ## Removing old backward compatibility code
 

devtools/server/actors/root.js

@@ -592,10 +592,6 @@ exports.RootActor = protocol.ActorClassWithSpec(rootSpec, {
     return id == window.docShell.browsingContext.id;
   },
 
-  protocolDescription: function() {
-    return require("devtools/shared/protocol").dumpProtocolSpec();
-  },
-
   /**
    * Remove the extra actor (added by ActorRegistry.addGlobalActor or
    * ActorRegistry.addTargetScopedActor) name |name|.

devtools/server/tests/xpcshell/test_protocolSpec.js

@@ -1,19 +0,0 @@
-"use strict";
-
-const run_test = Test(async function() {
-  initTestDevToolsServer();
-  const connection = DevToolsServer.connectPipe();
-  const client = new DevToolsClient(connection);
-
-  await client.connect();
-
-  const response = await client.mainRoot.protocolDescription();
-
-  assert(response.from == "root", "response.from has expected value");
-  assert(
-    typeof response.types === "object",
-    "response.types has expected type"
-  );
-
-  await client.close();
-});

devtools/server/tests/xpcshell/xpcshell.ini

@@ -217,7 +217,6 @@ skip-if = true # tests for breakpoint actors are obsolete bug 1524374
 [test_requestTypes.js]
 reason = bug 937197
 [test_layout-reflows-observer.js]
-[test_protocolSpec.js]
 [test_client_request.js]
 [test_symbols-01.js]
 [test_symbols-02.js]

devtools/shared/protocol.js

@@ -8,7 +8,6 @@ var { Actor, ActorClassWithSpec } = require("devtools/shared/protocol/Actor");
 var { Pool } = require("devtools/shared/protocol/Pool");
 var {
   types,
-  registeredTypes,
   registerFront,
   getFront,
   createRootFront,
@@ -38,56 +37,3 @@ exports.registerFront = registerFront;
 exports.getFront = getFront;
 exports.createRootFront = createRootFront;
 exports.method = method;
-
-exports.dumpActorSpec = function(type) {
-  const actorSpec = type.actorSpec;
-  const ret = {
-    category: "actor",
-    typeName: type.name,
-    methods: [],
-    events: {},
-  };
-
-  for (const _method of actorSpec.methods) {
-    ret.methods.push({
-      name: _method.name,
-      release: _method.release || undefined,
-      oneway: _method.oneway || undefined,
-      request: _method.request.describe(),
-      response: _method.response.describe(),
-    });
-  }
-
-  if (actorSpec.events) {
-    for (const [name, request] of actorSpec.events) {
-      ret.events[name] = request.describe();
-    }
-  }
-
-  JSON.stringify(ret);
-
-  return ret;
-};
-
-exports.dumpProtocolSpec = function() {
-  const ret = {
-    types: {},
-  };
-
-  for (let [name, type] of registeredTypes) {
-    // Force lazy instantiation if needed.
-    type = types.getType(name);
-    const category = type.category || undefined;
-    if (category === "dict") {
-      ret.types[name] = {
-        category: "dict",
-        typeName: name,
-        specializations: type.specializations,
-      };
-    } else if (category === "actor") {
-      ret.types[name] = exports.dumpActorSpec(type);
-    }
-  }
-
-  return ret;
-};

devtools/shared/specs/page-style.js

@@ -14,11 +14,6 @@ const {
 // Load the shared types for style actors
 require("devtools/shared/specs/style/style-types");
 
-// Preload the style-rule spec to make sure that domstylerule is fully defined.
-// The inspector still uses actorHasMethod, which relies on dumping the protocol
-// specs. This can be removed when actorHasMethod is removed.
-require("devtools/shared/specs/style-rule");
-
 const pageStyleSpec = generateActorSpec({
   typeName: "pagestyle",
 

devtools/shared/specs/root.js

@@ -92,11 +92,6 @@ const rootSpecPrototype = {
       },
     },
 
-    protocolDescription: {
-      request: {},
-      response: RetVal("json"),
-    },
-
     requestTypes: {
       request: {},
       response: RetVal("json"),