1694906 - Stop using TargetFactory::forTab and only use descriptors to open toolboxes

Status: RESOLVED FIXED

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

As discussed on the DevTools team channel, as we want to make this method less used, let's rename it.

Comment 1

[Julian Descottes [:jdescottes]]

Repurposing, I end up deleting all call sites to forTab in the next patch, not worth the extra review.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1694906 - [devtools] Use descriptors instead of targets in all toolbox APIs

Depends on D106405

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59da0caf4b8e
[devtools] Use descriptors instead of targets in all toolbox APIs r=ochameau,nchevobbe

Comment 4

[Sandor Molnar[:smolnar]]

Backed out for causing browser-chrome failures in browser_ext_addon_debugging_netmonitor.

Backout link: https://hg.mozilla.org/integration/autoland/rev/919a1dc56de51e1e215e983af4045886f365c192

Push with failures

Failure log

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1694906 - [devtools] Remove remaining call sites for showToolbox in extension tests

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1694906 - [devtools] Add new option to createDescriptorForTab to support devtools webextensions

Depends on D106426

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1694906 - [devtools] Wait for toolbox-ready instead of toolbox-created in ext-devtools.js

Depends on D107052

As explained in the code comments added here, we now have to wait for toolbox open to be sure to have a valid target

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1694906 - [devtools] Fix race conditions in various devtools webextensions tests

Depends on D107099

With the previous patch, we are waiting for toolbox open before creating contexts.
This makes some tests a bit racy if they are only waiting for toolbox open. They should also wait for the context to be ready.
Firing an event from the devtools_page seems to work fine for this but don't hesitate to suggest a better approach.

Comment 9

[Chris Peterson [:cpeterson]]

zombie says these patches modify a test that he will re-test and re-enable for Fission in bug 1691575.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b5269fe0113
[devtools] Remove remaining call sites for showToolbox in extension tests r=rpl
https://hg.mozilla.org/integration/autoland/rev/284e000deded
[devtools] Use descriptors instead of targets in all toolbox APIs r=ochameau,nchevobbe,rpl
https://hg.mozilla.org/integration/autoland/rev/02428006de51
[devtools] Add new option to createDescriptorForTab to support devtools webextensions r=rpl,ochameau
https://hg.mozilla.org/integration/autoland/rev/c23343373584
[devtools] Wait for toolbox-ready instead of toolbox-created in ext-devtools.js r=rpl
https://hg.mozilla.org/integration/autoland/rev/bd5cb6f8b41f
[devtools] Fix race conditions in various devtools webextensions tests r=rpl

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failures on browser_target_list_frames.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/b91f2d9569bbf9323f2a9dfd86ca69ef70f80bf0
Log link: https://treeherder.mozilla.org/logviewer?job_id=332632108&repo=autoland&lineNumber=10262

There were also damp failures: https://treeherder.mozilla.org/logviewer?job_id=332625236&repo=autoland&lineNumber=1092

Comment 12

[Julian Descottes [:jdescottes]]

Right about the DAMP failure. However the other one already happened before my patch landed: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Tgg_QVTORSSArA5x5LpMiw.0&searchStr=dt3%2Cfis&revision=38607d980ba725a4f0fccdf5f446d7fc3a8c3e8c

Comment 14

[Julian Descottes [:jdescottes]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #11)

Backed out for dt failures on browser_target_list_frames.js

For info I didn't manage to get the failure on 50 retriggers in the autoland push

Comment 15

[Julian Descottes [:jdescottes]]

sparky: hi! sorry for the ping, can someone from the perftest-reviewers group take a look at the small DAMP change in https://phabricator.services.mozilla.com/D106426 . This is a big refactor that blocks a few other devtools bugs and we'd like to get it in as soon as we can. Apologies for missing the DAMP dependency in the previous review cycles (DAMP push at https://treeherder.mozilla.org/jobs?repo=try&revision=814cebcf3f188a766374422d7ab4febe3ff0cb83)

Thanks!

Comment 16

[Greg Mierzwinski [:sparky]]

r+, sorry for the delay!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7d8db2d1f3
[devtools] Remove remaining call sites for showToolbox in extension tests r=rpl
https://hg.mozilla.org/integration/autoland/rev/7b9edbf658b2
[devtools] Use descriptors instead of targets in all toolbox APIs r=ochameau,nchevobbe,rpl,perftest-reviewers,sparky
https://hg.mozilla.org/integration/autoland/rev/b10a7e2b7e8c
[devtools] Add new option to createDescriptorForTab to support devtools webextensions r=rpl,ochameau
https://hg.mozilla.org/integration/autoland/rev/962ff8dd112f
[devtools] Wait for toolbox-ready instead of toolbox-created in ext-devtools.js r=rpl
https://hg.mozilla.org/integration/autoland/rev/969dd6958089
[devtools] Fix race conditions in various devtools webextensions tests r=rpl

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2e7d8db2d1f3
https://hg.mozilla.org/mozilla-central/rev/7b9edbf658b2
https://hg.mozilla.org/mozilla-central/rev/b10a7e2b7e8c
https://hg.mozilla.org/mozilla-central/rev/962ff8dd112f
https://hg.mozilla.org/mozilla-central/rev/969dd6958089