Bug 1233463 introduced loading toolbox in a tab, but so far it only allows debugging local targets. We should allow debugging remote ones. We need to come up with a new set of query parameters to define a target to debug.

Note that it depends on bug 1243452. That's because devtools/client/framework/test/browser_target_from_url.js has to wait for client.close() correctly. It wasn't as it was assuming close returns a promise...

Also, I pushed a clean branch to mozreview, you should see both bug patches in that branch.

Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc675735ec18&selectedJob=26768735 Note that I also patch devtools/client/framework/test/browser_toolbox_sidebar*.js. This is really weird. It seems to be more related to bug 1243452, which I depend on, but I verified and these tests pass if I only apply bug 1243452. I had to ensure destroying the toolbox before finishing the tests otherwise it was trigerring a crash in String c++ API...

Comment on attachment 8786978 [details] Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters. https://reviewboard.mozilla.org/r/75820/#review74562 Looks like a resonable step forward. We might change up URL args later, but we should be free to do so for now at least (no one's depending on our URLs yet...). I think the test to try a little harder to check that it's correct. ::: devtools/client/framework/test/browser_target_from_url.js:106 (Diff revision 2) > + info("Test remote process via TCP Connection"); > + > + let server = yield setupDebuggerServer(false); > + > + let { port } = server.listener; > + let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port)); Shouldn't you check somehow that the host, port, and ws were actually used? ::: devtools/client/framework/test/browser_target_from_url.js:109 (Diff revision 2) > + > + let { port } = server.listener; > + let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port)); > + let topWindow = Services.wm.getMostRecentWindow("navigator:browser"); > + is(target.url, topWindow.location.href); > + is(target.isLocalTab, false); These 4 lines are repeated a lot, maybe make a helper?

Comment on attachment 8787384 [details] Bug 1299503 - Cleanup browser_toolbox_sidebar toolboxes at test end to prevent crashes. https://reviewboard.mozilla.org/r/76164/#review74564

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > ::: devtools/client/framework/test/browser_target_from_url.js:106 > (Diff revision 2) > > + info("Test remote process via TCP Connection"); > > + > > + let server = yield setupDebuggerServer(false); > > + > > + let { port } = server.listener; > > + let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port)); > > Shouldn't you check somehow that the host, port, and ws were actually used? > I tried, but there is nothing on target.client/target.client._transport that reflect any information about the underlying connection we are using :/ It may help another work if we put some information about the client connection somewhere on client or transport. Should I do that?

See comment 10.

(In reply to Alexandre Poirot [:ochameau] from comment #10) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > > ::: devtools/client/framework/test/browser_target_from_url.js:106 > > (Diff revision 2) > > > + info("Test remote process via TCP Connection"); > > > + > > > + let server = yield setupDebuggerServer(false); > > > + > > > + let { port } = server.listener; > > > + let target = yield targetFromURL(new URL("http://foo?type=process&host=127.0.0.1&port=" + port)); > > > > Shouldn't you check somehow that the host, port, and ws were actually used? > > > > I tried, but there is nothing on target.client/target.client._transport that > reflect any information about the underlying connection we are using :/ > It may help another work if we put some information about the client > connection somewhere on client or transport. Should I do that? Ah, I should have remembered that! I am not sure what the most natural place to put it is... Maybe `_getTransport` in security/socket.js should copy the host, port, etc. onto the transport before returning it?

Comment on attachment 8786978 [details] Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters. https://reviewboard.mozilla.org/r/75820/#review74562 > Shouldn't you check somehow that the host, port, and ws were actually used? Just pushed a new changeset with such checks. I tweaked socket.js in order to do that and added some tests against this new code in dbg_socket.js xpcshell test. > These 4 lines are repeated a lot, maybe make a helper? I already had one so I used it!

Comment on attachment 8786978 [details] Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters. Reasking for review as I modified socket.js and dbg_socket.js.

Comment on attachment 8786978 [details] Bug 1299503 - Support connecting to remote targets via about:devtools-toolbox query parameters. https://reviewboard.mozilla.org/r/75820/#review82018 Thanks, seems like a good test improvement! ::: devtools/client/framework/target-from-url.js:106 (Diff revisions 2 - 4) > }); > > function* createClient(params) { > let host = params.get("host"); > let port = params.get("port"); > - let webSocket = params.get("ws"); > + let webSocket = Boolean(params.get("ws")); Hmm, I think `!!params.get("ws")` would be more idiomatic and avoids creating a Boolean object.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f16f77530146 Support connecting to remote targets via about:devtools-toolbox query parameters. r=jryans https://hg.mozilla.org/integration/autoland/rev/39e0e69febc5 Cleanup browser_toolbox_sidebar toolboxes at test end to prevent crashes. r=jryans

https://hg.mozilla.org/mozilla-central/rev/f16f77530146 https://hg.mozilla.org/mozilla-central/rev/39e0e69febc5