As part of converting RootActor to protocol.js, we will also need to update TabActor.

Comment from ChromeActor refactor (from ochameau): nit: This refactoring is interesting. It highlights the cruft of TabActor. This type attribute is misleading as type should be a reserved keyword. It may end up being considered as an event if the value set on type attribute matches an event name. tabAttached is only used in test and doc: https://searchfox.org/mozilla-central/search?q=tabAttached&case=false&regexp=false&path=devtools I'm not sure it is any useful. But keep this patch focused on 1:1 refactoring. I'm mentioning it as an opportunity to mention possible followup cleanups.

Comment on attachment 8976062 [details] Bug 1450956 - Convert TabActor to protocol.js; https://reviewboard.mozilla.org/r/244252/#review250226 This looks like a good first step, especially if DAMP reports no regression! But this is half way throught the TabActor refactoring. Now, we should: * use protocol.js events instead of manual this.conn.send calls For: workerListChanged, frameUpdate, tabDetached and tabNavigated events. * look into custom pools and how we manage child actors TabActor uses custom ActorPool. They are mostly used to easily remove a set of actors when the tab navigates. But that doesn't really fit protocol.js as: * TabActor is now a Pool itself, and so we should now attach actors/pools to it instead of to the DebuggerServerConnection, * As we use ActorPool, we use the old API (addActorPool, addActor) instead of protocol.js one (manage). These points are good candidates for followup as we should be able to land this before these points are addressed. ::: devtools/server/actors/chrome.js:44 (Diff revision 1) > * Protocol.js expects only the prototype object, and does not maintain the prototype > * chain when it constructs the ActorClass. For this reason we are using `extend` to > * maintain the properties of TabActor.prototype > * */ > > const chromePrototype = extend({}, TabActor.prototype); Hum. The overall setup looks weird. At the end, I'm not sure we want to use TabActor.prototype here. Right now, you do that: ``` const chromePrototype = extend({}, TabActor.prototype); chromePrototype.initialize = function(connection) { TabActor.prototype.initialize.call(this, connection); ... } exports.ChromeActor = ActorClassWithSpec(tabSpec, chromePrototype); ``` And TabActor.prototype is defined like this: ``` exports.TabActor = ActorClassWithSpec(tabSpec, tabPrototype); ``` And it is weird as ActorClassWithSpec with do this: ``` cls.prototype = extend(Actor.prototype, generateRequestHandlers(actorSpec, actorProto)); ``` So, if I expand this code, TabActor is defined like this: ``` TabActor.prototype = extend(Actor.prototype, generateRequestHandlers(tabActorSpec, tabActorPrototype)); ``` while ChromeActor is defined like this: ``` const chromePrototype = extend({}, TabActor.prototype); ChromeActor.prototype = extend(Actor.prototype, generateRequestHandlers(tabActorSpec, chromePrototype)); ``` We end up "extending" from Actor twice. I imagine that's not an issues as the prototype argument of ActorClassWithSpec overrides Actor.prototype... But, I'm wondering if we can do something better. What about doing this? ``` const chromePrototype = extend({}, tabPrototype); ``` Does that introduce other issues? Would you find that more disturbing? ::: devtools/server/actors/chrome.js:48 (Diff revision 1) > > const chromePrototype = extend({}, TabActor.prototype); > > chromePrototype.initialize = function(connection) { > - Actor.prototype.initialize.call(this, connection); > - TabActor.call(this, connection); > + TabActor.prototype.initialize.call(this, connection); > + // This is a leftover here. ::: devtools/server/actors/tab.js:205 (Diff revision 1) > <span class="indent">>></span> */ > -function TabActor(connection) { > - // This usage of decorate should be removed in favor of using ES6 extends EventEmitter. > - // See Bug 1394816. > + initialize: function(connection) { > + Actor.prototype.initialize.call(this, connection); > + // This usage of decorate should be removed in favor of using ES6 extends > + // EventEmitter. See Bug 1394816. > - EventEmitter.decorate(this); > + EventEmitter.decorate(this); This should no longer be needed as Actor, which inherits from Pool, inherits from EventEmitter: https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#821 Unless there is some difference between inheritance versus decoration? ::: devtools/server/actors/tab.js:207 (Diff revision 1) > - // This usage of decorate should be removed in favor of using ES6 extends EventEmitter. > - // See Bug 1394816. > + Actor.prototype.initialize.call(this, connection); > + // This usage of decorate should be removed in favor of using ES6 extends > + // EventEmitter. See Bug 1394816. > - EventEmitter.decorate(this); > + EventEmitter.decorate(this); > > - this.conn = connection; > + this.conn = connection; This is redundant with what is done in protocol.js: https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#815-819 ::: devtools/server/actors/tab.js:500 (Diff revision 1) > > /** > * Called when the actor is removed from the connection. > */ > destroy() { > this.exit(); From destroy you should call: Actor.prototype.destroy.call(this);

(In reply to Alexandre Poirot [:ochameau] from comment #4) > * look into custom pools and how we manage child actors > TabActor uses custom ActorPool. They are mostly used to easily remove a > set of actors when the tab navigates. > But that doesn't really fit protocol.js as: > * TabActor is now a Pool itself, and so we should now attach > actors/pools to it instead of to the DebuggerServerConnection, > * As we use ActorPool, we use the old API (addActorPool, addActor) > instead of protocol.js one (manage). About that particular point, it may help to replace `ActorPool` by protocol.js `Pool`: https://searchfox.org/mozilla-central/source/devtools/shared/protocol.js#815 At least we would have protocol.js jargon and use pool.manage(actor) instead of pool.addActor(actor). The only issue I see left with that approach is that the pools are still going to be registered to the DebuggerServerConnection and not to the tab actor. So the actor will still have to manually manage its pools and call destroy on them on destroy.

Comment on attachment 8976062 [details] Bug 1450956 - Convert TabActor to protocol.js; https://reviewboard.mozilla.org/r/244252/#review250226 > Hum. The overall setup looks weird. > At the end, I'm not sure we want to use TabActor.prototype here. > > Right now, you do that: > ``` > const chromePrototype = extend({}, TabActor.prototype); > chromePrototype.initialize = function(connection) { > TabActor.prototype.initialize.call(this, connection); > ... > } > exports.ChromeActor = ActorClassWithSpec(tabSpec, chromePrototype); > ``` > And TabActor.prototype is defined like this: > ``` > exports.TabActor = ActorClassWithSpec(tabSpec, tabPrototype); > ``` > > And it is weird as ActorClassWithSpec with do this: > ``` > cls.prototype = extend(Actor.prototype, generateRequestHandlers(actorSpec, actorProto)); > ``` > > So, if I expand this code, TabActor is defined like this: > ``` > TabActor.prototype = extend(Actor.prototype, generateRequestHandlers(tabActorSpec, tabActorPrototype)); > ``` > while ChromeActor is defined like this: > ``` > const chromePrototype = extend({}, TabActor.prototype); > ChromeActor.prototype = extend(Actor.prototype, generateRequestHandlers(tabActorSpec, chromePrototype)); > ``` > > We end up "extending" from Actor twice. I imagine that's not an issues as the prototype argument of ActorClassWithSpec overrides Actor.prototype... > But, I'm wondering if we can do something better. > > What about doing this? > ``` > const chromePrototype = extend({}, tabPrototype); > ``` > Does that introduce other issues? Would you find that more disturbing? this looks good, thanks for bringing that up

Comment on attachment 8976062 [details] Bug 1450956 - Convert TabActor to protocol.js; https://reviewboard.mozilla.org/r/244252/#review255152 Sorry for the delay, I thought you were going to address more points from comment 4... It looks good, thanks for working on this! Could you open followup bugs for comment 4? We can address events and pool independently.

Pushed by ystartsev@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7cfd6a1fa407 Convert TabActor to protocol.js; r=ochameau

Landing this so that jryans can keep moving with the renaming work! the pools and events should be less disruptive than the migration itself. I will do that as the next step :)