From efe9743fe7ffae04bae45e4f7f84bfc89bbe1c46 Mon Sep 17 00:00:00 2001 From: Nicolas Chevobbe Date: Wed, 24 Jun 2020 07:39:06 +0000 Subject: [PATCH] Bug 1495728 - Remove unused ActorPool class. r=ochameau. This class isn't used anymore, and it's safe to remove it. We take this as an opportunity to remove Pool#cleanup and Pool#isEmpty, which each only had one callsite. Some comments are updated to not mention ActorPool. Differential Revision: https://phabricator.services.mozilla.com/D80602 --- devtools/client/devtools-client.js | 6 +- devtools/server/actors/common.js | 120 ------------------ .../server/actors/targets/browsing-context.js | 4 +- .../server/actors/utils/actor-registry.js | 3 +- devtools/server/devtools-server-connection.js | 36 ++---- .../test_connection_closes_all_pools.js | 2 +- devtools/server/tests/xpcshell/testactors.js | 2 +- devtools/shared/protocol/Pool.js | 13 -- devtools/shared/protocol/lazy-pool.js | 4 +- 9 files changed, 23 insertions(+), 167 deletions(-) diff --git a/devtools/client/devtools-client.js b/devtools/client/devtools-client.js index 711c0be9f1ff..7b4109a37b87 100644 --- a/devtools/client/devtools-client.js +++ b/devtools/client/devtools-client.js @@ -614,19 +614,19 @@ DevToolsClient.prototype = { // // In the normal case where we shutdown cleanly, the toolbox tells each tool // to close, and they each call |destroy| on any fronts they were using. - // When |destroy| or |cleanup| is called on a protocol.js front, it also + // When |destroy| is called on a protocol.js front, it also // removes itself from the |_pools| array. Once the toolbox has shutdown, // the connection is closed, and we reach here. All fronts (should have // been) |destroy|ed, so |_pools| should empty. // // If the connection instead aborts unexpectedly, we may end up here with - // all fronts used during the life of the connection. So, we call |cleanup| + // all fronts used during the life of the connection. So, we call |destroy| // on them clear their state, reject pending requests, and remove themselves // from |_pools|. This saves the toolbox from hanging indefinitely, in case // it waits for some server response before shutdown that will now never // arrive. for (const pool of this._pools) { - pool.cleanup(); + pool.destroy(); } }, diff --git a/devtools/server/actors/common.js b/devtools/server/actors/common.js index 04f3fd212afd..fd8d6570d2c3 100644 --- a/devtools/server/actors/common.js +++ b/devtools/server/actors/common.js @@ -6,126 +6,6 @@ const { method } = require("devtools/shared/protocol"); -/** - * Construct an ActorPool. - * - * ActorPools are actorID -> actor mapping and storage. These are - * used to accumulate and quickly dispose of groups of actors that - * share a lifetime. - */ -function ActorPool(connection, label) { - this.conn = connection; - this.label = label; - this._actors = {}; -} - -ActorPool.prototype = { - /** - * Destroy the pool. This will remove all actors from the pool. - */ - destroy: function APDestroy() { - for (const id in this._actors) { - this.removeActor(this._actors[id]); - } - }, - - /** - * Add an actor to the pool. If the actor doesn't have an ID, allocate one - * from the connection. - * - * @param Object actor - * The actor to be added to the pool. - */ - addActor: function APAddActor(actor) { - actor.conn = this.conn; - if (!actor.actorID) { - if (!actor.typeName) { - throw new Error("Actor should a specify a `typeName` attribute"); - } - actor.actorID = this.conn.allocID(actor.typeName); - } - - // If the actor is already in a pool, remove it without destroying it. - if (actor.registeredPool) { - delete actor.registeredPool._actors[actor.actorID]; - } - actor.registeredPool = this; - - this._actors[actor.actorID] = actor; - }, - - /** - * Remove an actor from the pool. If the actor has a destroy method, call it. - */ - removeActor(actor) { - delete this._actors[actor.actorID]; - if (actor.destroy) { - actor.destroy(); - return; - } - // Obsolete destruction method name (might still be used by custom actors) - if (actor.disconnect) { - actor.disconnect(); - } - }, - - get: function APGet(actorID) { - return this._actors[actorID] || undefined; - }, - - has: function APHas(actorID) { - return actorID in this._actors; - }, - - /** - * Returns true if the pool is empty. - */ - isEmpty: function APIsEmpty() { - return Object.keys(this._actors).length == 0; - }, - - /** - * Match the api expected by the protocol library. - */ - unmanage: function(actor) { - return this.removeActor(actor); - }, - - forEach: function(callback) { - for (const name in this._actors) { - callback(this._actors[name]); - } - }, - - // Generator that yields each non-self child of the pool. - *poolChildren() { - if (!this._actors) { - return; - } - for (const actor of Object.values(this._actors)) { - // Self-owned actors are ok, but don't need visiting twice. - if (actor === this) { - continue; - } - yield actor; - } - }, - - dumpPool() { - for (const actor in this._actors) { - console.log(`>> ${actor}`); - } - }, - - // For consistency with Pool.js - // Will be checked by devtools-server-connection. - isTopPool() { - return true; - }, -}; - -exports.ActorPool = ActorPool; - /** * A SourceLocation represents a location in a source. * diff --git a/devtools/server/actors/targets/browsing-context.js b/devtools/server/actors/targets/browsing-context.js index 19f65d3b4ef7..a8724a64a000 100644 --- a/devtools/server/actors/targets/browsing-context.js +++ b/devtools/server/actors/targets/browsing-context.js @@ -542,14 +542,14 @@ const browsingContextTargetPrototype = { }, _createExtraActors() { - // Always use the same ActorPool, so existing actor instances + // Always use the same Pool, so existing actor instances // (created in createExtraActors) are not lost. if (!this._targetScopedActorPool) { this._targetScopedActorPool = new LazyPool(this.conn); } // Walk over target-scoped actor factories and make sure they are all - // instantiated and added into the ActorPool. + // instantiated and added into the Pool. return createExtraActors( ActorRegistry.targetScopedActorFactories, this._targetScopedActorPool, diff --git a/devtools/server/actors/utils/actor-registry.js b/devtools/server/actors/utils/actor-registry.js index 1767da2eda1c..7874f886c03f 100644 --- a/devtools/server/actors/utils/actor-registry.js +++ b/devtools/server/actors/utils/actor-registry.js @@ -27,8 +27,7 @@ const ActorRegistry = { * An object with 3 mandatory attributes: * - prefix (string): * The prefix of an actor is used to compute: - * - the `actorID` of each new actor instance (ex: prefix1). - * (See ActorPool.addActor) + * - the `actorID` of each new actor instance (ex: prefix1). (See Pool.manage) * - the actor name in the listTabs request. Sending a listTabs * request to the root actor returns actor IDs. IDs are in * dictionaries, with actor names as keys and actor IDs as values. diff --git a/devtools/server/devtools-server-connection.js b/devtools/server/devtools-server-connection.js index d507f25125b1..78c103e52075 100644 --- a/devtools/server/devtools-server-connection.js +++ b/devtools/server/devtools-server-connection.js @@ -121,8 +121,8 @@ DevToolsServerConnection.prototype = { /** * Remove a previously-added pool of actors to the connection. * - * @param ActorPool actorPool - * The ActorPool instance you want to remove. + * @param Pool actorPool + * The Pool instance you want to remove. */ removeActorPool(actorPool) { // When a connection is closed, it removes each of its actor pools. When an @@ -205,12 +205,9 @@ DevToolsServerConnection.prototype = { } if (typeof actor !== "object") { - // ActorPools should now contain only actor instances (i.e. objects) + // Pools should now contain only actor instances (i.e. objects) throw new Error( - "Unexpected actor constructor/function in ActorPool " + - "for actorID=" + - actorID + - "." + `Unexpected actor constructor/function in Pool for actorID "${actorID}".` ); } @@ -494,32 +491,25 @@ DevToolsServerConnection.prototype = { }, dumpPool(pool, output = [], dumpedPools) { - let label; - let actorIds = []; - let children = []; + const actorIds = []; + const children = []; if (dumpedPools.has(pool)) { return; } dumpedPools.add(pool); - // TRUE if the pool is an ActorPool - if (pool._actors) { - actorIds = Object.keys(pool._actors); - children = Object.values(pool._actors); - label = pool.label || ""; - } // TRUE if the pool is a Pool - else if (pool.__poolMap) { - for (const actor of pool.poolChildren()) { - children.push(actor); - actorIds.push(actor.actorID); - } - label = pool.label || pool.actorID; - } else { + if (!pool.__poolMap) { return; } + for (const actor of pool.poolChildren()) { + children.push(actor); + actorIds.push(actor.actorID); + } + const label = pool.label || pool.actorID; + output.push([label, actorIds]); dump(`- ${label}: ${JSON.stringify(actorIds)}\n`); children.forEach(childPool => diff --git a/devtools/server/tests/xpcshell/test_connection_closes_all_pools.js b/devtools/server/tests/xpcshell/test_connection_closes_all_pools.js index 84c26191d167..fe73dc8467a7 100644 --- a/devtools/server/tests/xpcshell/test_connection_closes_all_pools.js +++ b/devtools/server/tests/xpcshell/test_connection_closes_all_pools.js @@ -76,7 +76,7 @@ add_task(async function() { // | // \- childActor // - // Since parentActor is also an ActorPool from the point of view of the + // Since parentActor is also a Pool from the point of view of the // DevToolsServerConnection, it will attempt to destroy it when looping on // this._extraPools. But since `parentActor` is also a direct child of `pool`, // it has already been destroyed by the Pool destroy() mechanism. diff --git a/devtools/server/tests/xpcshell/testactors.js b/devtools/server/tests/xpcshell/testactors.js index d239976b5506..b0f23d876e75 100644 --- a/devtools/server/tests/xpcshell/testactors.js +++ b/devtools/server/tests/xpcshell/testactors.js @@ -206,7 +206,7 @@ const TestTargetActor = protocol.ActorClassWithSpec(browsingContextTargetSpec, { actorPool, this ); - if (!actorPool.isEmpty()) { + if (actorPool?._poolMap.size > 0) { this._descriptorActorPool = actorPool; this.conn.addActorPool(this._descriptorActorPool); } diff --git a/devtools/shared/protocol/Pool.js b/devtools/shared/protocol/Pool.js index 45e8c8a70fd5..3d075af16fbe 100644 --- a/devtools/shared/protocol/Pool.js +++ b/devtools/shared/protocol/Pool.js @@ -132,11 +132,6 @@ class Pool extends EventEmitter { return null; } - // True if this pool has no children. - isEmpty() { - return !this.__poolMap || this._poolMap.size == 0; - } - // Generator that yields each non-self child of the pool. *poolChildren() { if (!this.__poolMap) { @@ -204,14 +199,6 @@ class Pool extends EventEmitter { this.__poolMap.clear(); this.__poolMap = null; } - - /** - * For getting along with the devtools server pools, should be removable - * eventually. - */ - cleanup() { - this.destroy(); - } } exports.Pool = Pool; diff --git a/devtools/shared/protocol/lazy-pool.js b/devtools/shared/protocol/lazy-pool.js index ae3b76813ee8..c158ad832fd4 100644 --- a/devtools/shared/protocol/lazy-pool.js +++ b/devtools/shared/protocol/lazy-pool.js @@ -69,7 +69,7 @@ exports.LazyPool = LazyPool; * - _extraActors * An object whose own property names are factory table (and packet) * property names, and whose values are no-argument actor constructors, - * of the sort that one can add to an ActorPool. + * of the sort that one can add to a Pool. * * - conn * The DevToolsServerConnection in which the new actors will participate. @@ -130,7 +130,7 @@ exports.createExtraActors = createExtraActors; * - _extraActors * An object whose own property names are factory table (and packet) * property names, and whose values are no-argument actor constructors, - * of the sort that one can add to an ActorPool. + * of the sort that one can add to a Pool. * * - conn * The DevToolsServerConnection in which the new actors will participate.