Closed Bug 1465635 Opened 7 years ago Closed 7 years ago

Make Target classes be protocol.js fronts and merge them with old clients

Categories

(DevTools :: Framework, enhancement, P2)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox67 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: jryans, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission)

User Story

We should remove the existing client-side targets:

* TabTarget
* WorkerTarget

and create "TargetFronts" for all "TargetActors" using inheritance to share common functions, similar to actor side:

* BrowsingContextTargetFront (abstract)
* FrameTargetFront
* AddonTargetFront
* WorkerTargetFront
etc.

While doing that, we will have to merge with existing client classes:
* TabClient
* WorkerClient
* AddonClient

Attachments

(9 files, 6 obsolete files)

Bug 1465635 - Convert browser_two_tabs.js to async.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Convert canvas front instantiation to Target.getFront.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Always consider that Target.activeTab is set.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Use Target.title to fetch target's title instead of using its form.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Merge all target fronts with Target class.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Remove Target.activeTab property.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Prevent netmonitor from destroying all "close" event listeners.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Ensure removing BrowsingContextTarget front events when destroying it.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1465635 - Listen to toggle and close key short only for window host type.
47 bytes, text/x-phabricator-request
Details | Review
The "target" API has accumulated lots of cruft over the years. We should explore a new design alongside work for Fission. The user story field contains a potential approach.
Product: Firefox → DevTools
Rephrased this bug, as the goal is now clearer and also includes merging with old clients from devtools/shared/client, like TabClient and WorkerClient.
Summary: Redesign target API into something new → Make Target classes be protocol.js fronts and merge them with old clients
User Story: (updated)
Whiteboard: dt-fission
Assignee: nobody → poirot.alex
If any callback fired by DebuggerClient.close ends up calling close again, it will introduce an infinite loop. Depends on D15826
This patch uses a mixin in order to make it so that all target fronts compose with Target class. Target fronts do not inherit from Target class. Instead, a mixin a created against Target and each target front. We could have gone the other way around and have all target front to inherit from Target. But as of today, we expect Target methods like attach to be called first and only then the target front equivalents. So it feel more natural to call Target methods first and have it call front methods via super. But we could go the other way around and ensure calling Target methods from fronts early. Depends on D15830
Attachment #9034699 - Attachment is obsolete: true
Attachment #9034701 - Attachment is obsolete: true
Attachment #9037301 - Attachment is obsolete: true

When destroying the target, Target.destroy (for local tabs) only calls DebuggerClient.close,
which isn't going to call detach. But we still do need to unregister
the tabNavigated/frameUpdate listener to prevent unecessary event from firing.

Depends on D17609

When destroying the target, Target.destroy (for local tabs) only calls DebuggerClient.close,
which isn't going to call detach. But we still do need to unregister
the tabNavigated/frameUpdate listener to prevent unecessary event from firing.

Attachment #9039130 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a9b181f32189 Convert browser_two_tabs.js to async. r=yulia https://hg.mozilla.org/integration/autoland/rev/00d325ab677a Convert canvas front instantiation to Target.getFront. r=yulia https://hg.mozilla.org/integration/autoland/rev/d9ae7cd34c1a Always consider that Target.activeTab is set. r=yulia https://hg.mozilla.org/integration/autoland/rev/734646bf9829 Use Target.title to fetch target's title instead of using its form. r=yulia https://hg.mozilla.org/integration/autoland/rev/52728b761d5d Merge all target fronts with Target class. r=yulia,jdescottes https://hg.mozilla.org/integration/autoland/rev/11413ebfbcaf Remove Target.activeTab property. r=yulia https://hg.mozilla.org/integration/autoland/rev/9ec017a91e78 Prevent netmonitor from destroying all "close" event listeners. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/e8e363f98525 Ensure removing BrowsingContextTarget front events when destroying it. r=jdescottes

This is an hotfix for: Bug 1465635 - Convert canvas front instantiation to Target.getFront.

We were registering these key shortcut twice.
A first time from devtools-startup.js and another time from Toolbox.
Both shortcut listeners were called when the toolbox was running,
leading the toolbox to reopen while we were expecting it to be closed.

Depends on D17610

Attachment #9040138 - Attachment is obsolete: true
Attachment #9040140 - Attachment is obsolete: true

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f40ae1d72a9d904b408a29da4a26947dad9d241e
But I realize that this try syntax doesn't include mochitests "gpu", which was failing, so here is a new one:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf8b5f08e769324b416f214b5583d195f35f3f5

Flags: needinfo?(poirot.alex)
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2fe3a0ae216b Convert browser_two_tabs.js to async. r=yulia https://hg.mozilla.org/integration/autoland/rev/9c7fe2ba8434 Convert canvas front instantiation to Target.getFront. r=yulia https://hg.mozilla.org/integration/autoland/rev/e6efb1191ec6 Always consider that Target.activeTab is set. r=yulia https://hg.mozilla.org/integration/autoland/rev/fead89ec2d1b Use Target.title to fetch target's title instead of using its form. r=yulia https://hg.mozilla.org/integration/autoland/rev/1dfe4f2eb472 Merge all target fronts with Target class. r=yulia,jdescottes https://hg.mozilla.org/integration/autoland/rev/aff30c79fa91 Remove Target.activeTab property. r=yulia https://hg.mozilla.org/integration/autoland/rev/cb3aef2a4aff Prevent netmonitor from destroying all "close" event listeners. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/76ea544a5404 Ensure removing BrowsingContextTarget front events when destroying it. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/756f2a2d2017 Listen to toggle and close key short only for window host type. r=jdescottes
See Also: → 1671973
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: