1492830 - Clarify why part of the "attaching" is being split between TabTarget.attach and Toolbox.open

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

After bug 1485676, target will have an `attach` method, that used to be part of its old `makeRemote` method:
https://searchfox.org/mozilla-central/rev/94e37e71ffbfd39e6ad73ebcda5d77cce8d341ae/devtools/client/framework/target.js#450-477
This method calls BrowsingContextTargetActor.attach request as well as ConsoleActor's startsListeners (which is considered as the "attaching" request).

But at the same time, in Toolbox.open function:
https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#452-464
We do call target.attach, and right after, re-call ConsoleActor's startsLiteners request. And right after, we attach the thread actor.

At the end it is not really clear where all these attaching calls should live, but I think we should unify that to be managed only by one of these two classes.

This issues most likely related to bug 1397020 and 1072764 which should be revisited after bug 1485676.

Also, figuring this out will most likely help merging TabTarget and TabClient (bug 1465635)

Comment 1

[Alexandre Poirot [:ochameau]]

Note that attach it also called from TargetFactory.forRemoteTab:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#58
We may remove that call and let toolbox call it.

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1492830 - Move toolbox code from attach-thread.js to toolbox; r=ochameau,jdescottes

Since there is no toolbox specific code left, this code can be merged with the target-mixin
code

Comment 3

[Yulia Startsev [:yulia]]

Attachment: Bug 1492830 - change worker target threadClient attach pattern to be the same as other targets; r=jdescottes

The worker target is the only target to have a unique thread client attach. It doesn't look
like there is a specific reason for this. In order to remove redundancy from target thread
instantiation, I have made this follow the same pattern as elsewhere, so we can merge the "resume"
method into the thread attach method on target.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91d4f30e0dda
change worker target threadClient attach pattern to be the same as other targets; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/02a438fa11a8
Move toolbox code from attach-thread.js to toolbox; r=jdescottes

Comment 5

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/91d4f30e0dda
https://hg.mozilla.org/mozilla-central/rev/02a438fa11a8