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

RESOLVED FIXED in Firefox 68

Status

enhancement
P2
normal
RESOLVED FIXED
9 months ago
29 days ago

People

(Reporter: ochameau, Assigned: yulia)

Tracking

unspecified
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: dt-fission)

Attachments

(2 attachments)

Reporter

Description

9 months ago
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)
Reporter

Comment 1

9 months ago
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
Reporter

Updated

9 months ago
Depends on: 1225492
Reporter

Updated

9 months ago
Depends on: 1493714
Reporter

Updated

8 months ago
Whiteboard: dt-fission
Assignee

Updated

2 months ago
Blocks: 1494796
Assignee

Updated

2 months ago
Assignee: nobody → ystartsev
Assignee

Comment 2

2 months ago

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

Assignee

Comment 3

2 months ago

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

Comment 4

Last month
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

Comment 5

Last month
bugherder
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.