Closed Bug 1473511 Opened 2 years ago Closed 2 years ago

Browsing context target actor: refactor context pool to use actor pool


(DevTools :: General, enhancement, P3)



(firefox63 fixed)

Firefox 63
Tracking Status
firefox63 --- fixed


(Reporter: yulia, Assigned: yulia)




(1 file)

With moving browsing context target actor to protocol.js we have a bit of further refactoring work to do on the pools: We have three pools on the browsing context target: contextPool, targetScopedActorPool, and workerActorPool. Of these three, contextPool is the only one containing one actor: the thread actor. Since Protocol.js provides a pool for every actor, we can have the tread actor be managed directly by the browsing context target actor. For the other two pools, we will work on this in a separate issue
Severity: normal → enhancement
Comment on attachment 8989944 [details]
Bug 1473511 - use protocol.js pool to manage thread actor;

Looks good if you also tweak the destroy codepath and it works as expected.
Otherwise please resubmit for another review if it did not.

::: devtools/server/actors/targets/browsing-context.js:861
(Diff revision 1)
>    /**
> -   * Creates a thread actor and a pool for context-lifetime actors. It then sets
> -   * up the content window for debugging.
> +   * Creates and manages the thread actor as part of the Browsing Context Target pool.
> +   * This sets up the content window for being debugged
>     */
> -  _pushContext() {
> +  _addThreadActor() {

I would rather name the function create and destroy thread actor as "add" may convey the meaning that we could add more than one.
Also it may be good to keep an assert checking that threadActor is null before trying to instanciate it.

::: devtools/server/actors/targets/browsing-context.js:872
(Diff revision 1)
> -  _popContext() {
> -    assert(!!this._contextPool, "No context to pop.");
> +  _removeThreadActor() {
> +    this.unmanage(this.threadActor);
> -
> -    this.conn.removeActorPool(this._contextPool);
> -    this._contextPool = null;
>      this.threadActor.exit();

`unmanage` only removes the actor from its pool:
We never use unmanage except from Actor.destroy:

So it would be better to only call threadActor.destroy(), which should ultimately call unmanage. For that we have to ensure calling Actor.destroy from ThreadActor.destroy (which we do not do yet).

But `ThreadActor._state` is special and if we want to keep the same behavior, from here, we would still call `ThreadActor.exit` and not directly `ThreadActor.destroy`.
Attachment #8989944 - Flags: review?(poirot.alex) → review+
Pushed by
use protocol.js pool to manage thread actor; r=ochameau
(In reply to Yulia Startsev [:yulia] from comment #8)
> New try run:
> jobs?repo=try&revision=27d2069ed2712de9b3559eefe89679403c99a9f2

This try run seems to be for bug 1473569 rather than this bug.
Comment on attachment 8989944 [details]
Bug 1473511 - use protocol.js pool to manage thread actor;

::: devtools/server/actors/thread.js:247
(Diff revision 3)
>     * destroy the debugger and put the actor in the exited state.
>     */
>    exit: function() {
>      this.destroy();
>      this._state = "exited";
> +;

It would be helpful to drop a comment about why `Actor.destroy` is called from `exit` and not from `destroy`.
corrected try run:

I added comments making it clearer what the exit and destroy methods are doing. We can revisit and rename them when we return to the protocol.js refactor of threadActor in #1450284
Pushed by
use protocol.js pool to manage thread actor; r=ochameau
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.