Closed Bug 1492830 Opened 2 years ago Closed 1 year ago

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

Categories

(DevTools :: Framework, enhancement, P2)

enhancement

Tracking

(firefox68 fixed)

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: ochameau, Assigned: yulia)

References

Details

(Whiteboard: dt-fission)

Attachments

(2 files)

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)
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.
Severity: normal → enhancement
Priority: -- → P2
Depends on: 1225492
Depends on: 1493714
Whiteboard: dt-fission
Blocks: 1494796
Assignee: nobody → ystartsev

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

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.

Attachment #9061665 - Attachment description: Bug 1492830 - Move generic code from attach-thread.js to toolbox and target; r=ochameau,jdescottes → Bug 1492830 - Move toolbox code from attach-thread.js to toolbox; r=ochameau,jdescottes
Pushed by ystartsev@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.