Closed
Bug 1506549
Opened 6 years ago
Closed 6 years ago
Instantiate all parent/content process Targets via the BrowsingContextTargetFront/ContentProcessTargetFront rather than its form
Categories
(DevTools :: Framework, enhancement, P2)
DevTools
Framework
Tracking
(Fission Milestone:M4, firefox65 fixed)
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
(Whiteboard: dt-fission)
Attachments
(6 files)
For now, the Target for the browser toolbox or browser content toolbox are instantiated via the form of the BrowsingContextTargetActor or ContentProcessTargetActor.
Instead it should be instantiated via its front, by passing the front as Target constructor argument.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
MozReview-Commit-ID: EGWYEmAkbtr
Assignee | ||
Comment 3•6 years ago
|
||
It allows to send the event through the front rather than DebuggerClient.
MozReview-Commit-ID: H8zEwAlUWDb
Depends on D11622
Assignee | ||
Comment 4•6 years ago
|
||
This was only used by test and isn't much useful.
MozReview-Commit-ID: DeIimVmMOOs
Depends on D11623
Assignee | ||
Comment 5•6 years ago
|
||
MozReview-Commit-ID: EKWTGhGo0VM
Depends on D11624
Assignee | ||
Comment 6•6 years ago
|
||
Now that this event is correctly fired on the Front rather than DebuggerClient,
we have to listen for this event on each individual front.
MozReview-Commit-ID: 9MqdHuAEr7U
Depends on D11625
Assignee | ||
Comment 7•6 years ago
|
||
MozReview-Commit-ID: BWsvAcWthnF
Assignee | ||
Comment 8•6 years ago
|
||
Comment on attachment 9024396 [details]
Bug 1506549 - Return target fronts out of RootFront.getProcess and getMainProcess.
This is the very first patch of a long serie, where I want to instantiate Target with a front rather than a form.
For content process target, I only did just and only that in bug 1506545 (https://bugzilla.mozilla.org/attachment.cgi?id=9024381).
I moved the "attachContentProcessTarget" from Target.attach to the TargetFactory.forRemoteTab callsites. In existing codebase (except for Worker target), the target front is always instantiated late, from Target.attach in one of the "attach*****Target" method.
The plan here is to have Target.attach only call the "attach" request of the given target (if it has one, content process target for example doesn't).
And so, the callsites of TargetFactory.forRemoteTab have to somehow instantiate the front.
They already have the actor's form, so they could in theory instantiate the front out of it, but it ends up being easier if we stop passing around the form, and let the original methods returning the form use the right protocol.js's specification and let protocol.js automatically create the front via marshalling layer.
That's what I'm doing in bug 1506548. But not here as getProcess may return two kind of actors/fronts: BrowsingContextTargetFront for parent process of ContentProcessTargetFront for content processes. So I'm not using specification here, but still, the RootFront.getProcess method returns the right Front and RootFront manages the returned front itself. We no longer have to register the Front in DebuggerClient.fronts Pool. It is much more natural regarding protocol.js conventions.
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 9024397 [details]
Bug 1506549 - correctly type workerListChanged on ContentProcessTarget spec.
I don't quite understand why it wasn't failing before... but we were missing this event.
If I don't do that, with previous patch, worker tests start failing!
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 9024398 [details]
Bug 1506549 - Stop returning the actor from Pool.manage.
This was only used by tests and isn't that useful.
I'm removing this as it helps a bit implementing the next patch:
https://phabricator.services.mozilla.com/D11625
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 9024399 [details]
Bug 1506549 - Introduce API to listen for new child fronts of a given type.
This is clearly a fork of your current work on Target.onFront, but put on all the Fronts.
I need that for the last patch of this queue:
https://phabricator.services.mozilla.com/D11626
In about:debugging I don't have a target object, I only have the RootFront (client.mainRoot) from which I have to listen for all ContentProcessTarget actors being created.
So I can't reuse Target.onFront, also, in its current shape, Target.onFront only allows to listen for target scoped actors.
This patch is slightly more generic:
* it allows to listen for any kind child fronts
* on any given Front, it is not restricted to Target objects/fronts
Unfortunately we can't share the code between Target and Front.
1) Target doesn't compose/inherit with Front
2) Target now always have a Front attribute (i.e. activeTab), but it is only available after Target.attach().
We will be able to share the code once Target and Target fronts are merged, or at least once Target always receive a Front in its constructor. I'm very actively working on this via these patches!
Assignee | ||
Comment 12•6 years ago
|
||
Comment on attachment 9024566 [details]
Bug 1506549 - Either pass a front or a form to Target constructor.
Actually, this patch is the very first of this patch queue.
It helps a lot simplifying the implementation of the second patch:
https://phabricator.services.mozilla.com/D11622
As well as bug 1506548:
https://phabricator.services.mozilla.com/D11763
Ideally, I would like to stop exposing/using Target.form, but that's yet another quest.
Hopefully it clarifies a bit how Target.form is fetch from the front class...
Comment 13•6 years ago
|
||
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f259de6cce0
Either pass a front or a form to Target constructor. r=yulia
https://hg.mozilla.org/integration/autoland/rev/97554f465eca
Return target fronts out of RootFront.getProcess and getMainProcess. r=yulia
https://hg.mozilla.org/integration/autoland/rev/490eeba8f9f7
correctly type workerListChanged on ContentProcessTarget spec. r=yulia
https://hg.mozilla.org/integration/autoland/rev/da0d76d0e8fc
Stop returning the actor from Pool.manage. r=yulia
https://hg.mozilla.org/integration/autoland/rev/6be66dea928c
Introduce API to listen for new child fronts of a given type. r=yulia
Comment 14•6 years ago
|
||
Backed out for failing devtools at devtools/client/aboutdebugging/test/browser_service_workers_status.js
Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6be66dea928cff8763658aaf69c52cb37eb5e2c3
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=211740914&repo=autoland&lineNumber=3809
Backout: https://hg.mozilla.org/integration/autoland/rev/d9691eaa9e58b99cdc43812d7af03fd20838cd51
Flags: needinfo?(poirot.alex)
Assignee | ||
Comment 15•6 years ago
|
||
I forgot to land the topmost patch of this queue...
Too bad lando doesn't warn you about landing part of a patch queue!
Flags: needinfo?(poirot.alex)
Comment 16•6 years ago
|
||
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80ebeb322e14
Either pass a front or a form to Target constructor. r=yulia
https://hg.mozilla.org/integration/autoland/rev/bd6fcbbf257d
Return target fronts out of RootFront.getProcess and getMainProcess. r=yulia
https://hg.mozilla.org/integration/autoland/rev/6aa455d0de46
correctly type workerListChanged on ContentProcessTarget spec. r=yulia
https://hg.mozilla.org/integration/autoland/rev/71da82d5b981
Stop returning the actor from Pool.manage. r=yulia
https://hg.mozilla.org/integration/autoland/rev/0ceaf305b926
Introduce API to listen for new child fronts of a given type. r=yulia
https://hg.mozilla.org/integration/autoland/rev/1fdfda9f7f12
Listen for workerListChanged on all content process target fronts. r=yulia
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/80ebeb322e14
https://hg.mozilla.org/mozilla-central/rev/bd6fcbbf257d
https://hg.mozilla.org/mozilla-central/rev/6aa455d0de46
https://hg.mozilla.org/mozilla-central/rev/71da82d5b981
https://hg.mozilla.org/mozilla-central/rev/0ceaf305b926
https://hg.mozilla.org/mozilla-central/rev/1fdfda9f7f12
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Comment 18•5 years ago
|
||
Retroactively moving fixed bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to an appropriate Fission Milestone.
This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:
0ee3c76a-bc79-4eb2-8d12-05dc0b68e732
Fission Milestone: --- → M4
You need to log in
before you can comment on or make changes to this bug.
Description
•