1450956 - Convert TabActor to protocol.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Yulia Startsev [:yulia]]

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

Comment 1

[Yulia Startsev [:yulia]]

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 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1450956 - Convert TabActor to protocol.js;

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

Comment 3

[Yulia Startsev [:yulia]]

Comment on attachment 8976062 [details]
Bug 1450956 - Convert TabActor to protocol.js;

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

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

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

[Yulia Startsev [:yulia]]

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 8

[Yulia Startsev [:yulia]]

Comment on attachment 8976062 [details]
Bug 1450956 - Convert TabActor to protocol.js;

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

Comment 9

[Alexandre Poirot [:ochameau]]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cfd6a1fa407
Convert TabActor to protocol.js; r=ochameau

Comment 11

[Yulia Startsev [:yulia]]

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

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7cfd6a1fa407