From 9defeffe44474d9f1ac58e28aa0853fdad77f337 Mon Sep 17 00:00:00 2001 From: "J. Ryan Stinnett" Date: Tue, 28 Jun 2016 20:10:55 -0500 Subject: [PATCH] Bug 1240912 - Toolbox remains connected across frame swaps. r=ochameau It is now possible keep the toolbox open while toggling RDM open and closed. The toolbox will follow the frame as it moves and update the message manager it uses internally with each move. MozReview-Commit-ID: DvLzCmOXfnb --- devtools/client/framework/toolbox-init.js | 2 +- .../client/responsive.html/browser/swap.js | 17 ++++ .../responsive.html/test/browser/browser.ini | 2 +- .../browser/browser_toolbox_swap_browsers.js | 82 +++++++++++++++++++ devtools/server/main.js | 50 ++++++++--- devtools/shared/transport/transport.js | 36 +++++--- 6 files changed, 165 insertions(+), 24 deletions(-) create mode 100644 devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js diff --git a/devtools/client/framework/toolbox-init.js b/devtools/client/framework/toolbox-init.js index 95eee3075507..2b776dc1796b 100644 --- a/devtools/client/framework/toolbox-init.js +++ b/devtools/client/framework/toolbox-init.js @@ -74,7 +74,7 @@ if (url.search.length > 1) { window.removeEventListener("unload", onUnload); toolbox.destroy(); } - window.addEventListener("unload", onUnload, true); + window.addEventListener("unload", onUnload); toolbox.on("destroy", function () { window.removeEventListener("unload", onUnload); }); diff --git a/devtools/client/responsive.html/browser/swap.js b/devtools/client/responsive.html/browser/swap.js index 53f7d4ae8ad8..deb673bb7c5e 100644 --- a/devtools/client/responsive.html/browser/swap.js +++ b/devtools/client/responsive.html/browser/swap.js @@ -35,6 +35,20 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) { let innerBrowser; let tunnel; + // Dispatch a custom event each time the _viewport content_ is swapped from one browser + // to another. DevTools server code uses this to follow the content if there is an + // active DevTools connection. While browser.xml does dispatch it's own SwapDocShells + // event, this one is easier for DevTools to follow because it's only emitted once per + // transition, instead of twice like SwapDocShells. + let dispatchDevToolsBrowserSwap = (from, to) => { + let CustomEvent = tab.ownerDocument.defaultView.CustomEvent; + let event = new CustomEvent("DevTools:BrowserSwap", { + detail: to, + bubbles: true, + }); + from.dispatchEvent(event); + }; + return { start: Task.async(function* () { @@ -72,6 +86,7 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) { // 4. Swap tab content from the regular browser tab to the browser within // the viewport in the tool UI, preserving all state via // `gBrowser._swapBrowserDocShells`. + dispatchDevToolsBrowserSwap(tab.linkedBrowser, innerBrowser); gBrowser._swapBrowserDocShells(tab, innerBrowser); // 5. Force the original browser tab to be non-remote since the tool UI @@ -115,6 +130,7 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) { // 4. Swap tab content from the browser within the viewport in the tool UI // to the regular browser tab, preserving all state via // `gBrowser._swapBrowserDocShells`. + dispatchDevToolsBrowserSwap(innerBrowser, contentBrowser); gBrowser._swapBrowserDocShells(contentTab, innerBrowser); innerBrowser = null; @@ -126,6 +142,7 @@ function swapToInnerBrowser({ tab, containerURL, getInnerBrowser }) { // 6. Swap the content into the original browser tab and close the // temporary tab used to hold the content via // `swapBrowsersAndCloseOther`. + dispatchDevToolsBrowserSwap(contentBrowser, tab.linkedBrowser); gBrowser.swapBrowsersAndCloseOther(tab, contentTab); gBrowser = null; diff --git a/devtools/client/responsive.html/test/browser/browser.ini b/devtools/client/responsive.html/test/browser/browser.ini index de84aad5510f..3ad1f94a8d64 100644 --- a/devtools/client/responsive.html/test/browser/browser.ini +++ b/devtools/client/responsive.html/test/browser/browser.ini @@ -28,10 +28,10 @@ support-files = [browser_page_state.js] [browser_permission_doorhanger.js] [browser_resize_cmd.js] -skip-if = true # GCLI target confused after swap, will fix in bug 1240912 [browser_screenshot_button.js] [browser_shutdown_close_sync.js] [browser_toolbox_computed_view.js] [browser_toolbox_rule_view.js] +[browser_toolbox_swap_browsers.js] [browser_touch_simulation.js] [browser_viewport_basics.js] diff --git a/devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js b/devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js new file mode 100644 index 000000000000..8834f8331f31 --- /dev/null +++ b/devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js @@ -0,0 +1,82 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +// Verify that toolbox remains open when opening and closing RDM. + +const TEST_URL = "http://example.com/"; + +function getServerConnections(browser) { + ok(browser.isRemoteBrowser, "Content browser is remote"); + return ContentTask.spawn(browser, {}, function* () { + const Cu = Components.utils; + const { require } = Cu.import("resource://devtools/shared/Loader.jsm", {}); + const { DebuggerServer } = require("devtools/server/main"); + if (!DebuggerServer._connections) { + return 0; + } + return Object.getOwnPropertyNames(DebuggerServer._connections); + }); +} + +let checkServerConnectionCount = Task.async(function* (browser, expected) { + let conns = yield getServerConnections(browser); + is(conns.length || 0, expected, "Server connection count"); +}); + +let checkToolbox = Task.async(function* (tab, location) { + let target = TargetFactory.forTab(tab); + ok(!!gDevTools.getToolbox(target), `Toolbox exists ${location}`); +}); + +add_task(function* () { + let tab = yield addTab(TEST_URL); + + // Open toolbox outside RDM + { + // 0: No DevTools connections yet + yield checkServerConnectionCount(tab.linkedBrowser, 0); + let { toolbox } = yield openInspector(); + // 2: One for each tab (starting tab plus the one we opened). Only one truly needed, + // but calling listTabs will create one for each tab. `registerTestActor` calls + // this, triggering the extra tab's actor to be made. + yield checkServerConnectionCount(tab.linkedBrowser, 2); + yield checkToolbox(tab, "outside RDM"); + let { ui } = yield openRDM(tab); + // 3: RDM UI uses an extra connection + yield checkServerConnectionCount(ui.getViewportBrowser(), 3); + yield checkToolbox(tab, "after opening RDM"); + yield closeRDM(tab); + // 2: RDM UI closed, back to one for each tab + yield checkServerConnectionCount(tab.linkedBrowser, 2); + yield checkToolbox(tab, tab.linkedBrowser, "after closing RDM"); + yield toolbox.destroy(); + // 0: All DevTools usage closed + yield checkServerConnectionCount(tab.linkedBrowser, 0); + } + + // Open toolbox inside RDM + { + // 0: No DevTools connections yet + yield checkServerConnectionCount(tab.linkedBrowser, 0); + let { ui } = yield openRDM(tab); + // 1: RDM UI uses an extra connection + yield checkServerConnectionCount(ui.getViewportBrowser(), 1); + let { toolbox } = yield openInspector(); + // 3: One for each tab (starting tab plus the one we opened). Only one truly needed, + // but calling listTabs will create one for each tab. `registerTestActor` calls + // this, triggering the extra tab's actor to be made. + yield checkServerConnectionCount(ui.getViewportBrowser(), 3); + yield checkToolbox(tab, ui.getViewportBrowser(), "inside RDM"); + yield closeRDM(tab); + // 2: RDM UI closed, back to one for each tab + yield checkServerConnectionCount(tab.linkedBrowser, 2); + yield checkToolbox(tab, tab.linkedBrowser, "after closing RDM"); + yield toolbox.destroy(); + // 0: All DevTools usage closed + yield checkServerConnectionCount(tab.linkedBrowser, 0); + } + + yield removeTab(tab); +}); diff --git a/devtools/server/main.js b/devtools/server/main.js index af3417a378ac..65e437978e2c 100644 --- a/devtools/server/main.js +++ b/devtools/server/main.js @@ -1005,7 +1005,24 @@ var DebuggerServer = { // or else fallback to asking the frameLoader itself. let mm = frame.messageManager || frame.frameLoader.messageManager; mm.loadFrameScript("resource://devtools/server/child.js", false); - this._childMessageManagers.add(mm); + + let trackMessageManager = () => { + frame.addEventListener("DevTools:BrowserSwap", onBrowserSwap); + mm.addMessageListener("debug:setup-in-parent", onSetupInParent); + if (!actor) { + mm.addMessageListener("debug:actor", onActorCreated); + } + DebuggerServer._childMessageManagers.add(mm); + }; + + let untrackMessageManager = () => { + frame.removeEventListener("DevTools:BrowserSwap", onBrowserSwap); + mm.removeMessageListener("debug:setup-in-parent", onSetupInParent); + if (!actor) { + mm.removeMessageListener("debug:actor", onActorCreated); + } + DebuggerServer._childMessageManagers.delete(mm); + }; let actor, childTransport; let prefix = connection.allocID("child"); @@ -1044,7 +1061,6 @@ var DebuggerServer = { return false; } }; - mm.addMessageListener("debug:setup-in-parent", onSetupInParent); let onActorCreated = DevToolsUtils.makeInfallible(function (msg) { if (msg.json.prefix != prefix) { @@ -1069,11 +1085,25 @@ var DebuggerServer = { let { NetworkMonitorManager } = require("devtools/shared/webconsole/network-monitor"); netMonitor = new NetworkMonitorManager(frame, actor.actor); - events.emit(DebuggerServer, "new-child-process", { mm }); - deferred.resolve(actor); }).bind(this); - mm.addMessageListener("debug:actor", onActorCreated); + + // Listen for browser frame swap + let onBrowserSwap = ({ detail: newFrame }) => { + // Remove listeners from old frame and mm + untrackMessageManager(); + // Update frame and mm to point to the new browser frame + frame = newFrame; + // Get messageManager from XUL browser (which might be a specialized tunnel for RDM) + // or else fallback to asking the frameLoader itself. + mm = frame.messageManager || frame.frameLoader.messageManager; + // Add listeners to new frame and mm + trackMessageManager(); + + if (childTransport) { + childTransport.swapBrowser(mm); + } + }; let destroy = DevToolsUtils.makeInfallible(function () { // provides hook to actor modules that need to exchange messages @@ -1119,16 +1149,14 @@ var DebuggerServer = { } // Cleanup all listeners + untrackMessageManager(); Services.obs.removeObserver(onMessageManagerClose, "message-manager-close"); - mm.removeMessageListener("debug:setup-in-parent", onSetupInParent); - if (!actor) { - mm.removeMessageListener("debug:actor", onActorCreated); - } events.off(connection, "closed", destroy); - - DebuggerServer._childMessageManagers.delete(mm); }); + // Listen for various messages and frame events + trackMessageManager(); + // Listen for app process exit let onMessageManagerClose = function (subject, topic, data) { if (subject == mm) { diff --git a/devtools/shared/transport/transport.js b/devtools/shared/transport/transport.js index c0fbb1e4c7b5..e83bf8de66e5 100644 --- a/devtools/shared/transport/transport.js +++ b/devtools/shared/transport/transport.js @@ -699,11 +699,11 @@ exports.LocalDebuggerTransport = LocalDebuggerTransport; /** - * A transport for the debugging protocol that uses nsIMessageSenders to + * A transport for the debugging protocol that uses nsIMessageManagers to * exchange packets with servers running in child processes. * - * In the parent process, |sender| should be the nsIMessageSender for the - * child process. In a child process, |sender| should be the child process + * In the parent process, |mm| should be the nsIMessageSender for the + * child process. In a child process, |mm| should be the child process * message manager, which sends packets to the parent. * * |prefix| is a string included in the message names, to distinguish @@ -712,10 +712,10 @@ * This transport exchanges messages named 'debug::packet', where * is |prefix|, whose data is the protocol packet. */ - function ChildDebuggerTransport(sender, prefix) { + function ChildDebuggerTransport(mm, prefix) { EventEmitter.decorate(this); - this._sender = sender; + this._mm = mm; this._messageName = "debug:" + prefix + ":packet"; } @@ -729,13 +729,13 @@ hooks: null, - ready: function () { - this._sender.addMessageListener(this._messageName, this); + _addListener() { + this._mm.addMessageListener(this._messageName, this); }, - close: function () { + _removeListener() { try { - this._sender.removeMessageListener(this._messageName, this); + this._mm.removeMessageListener(this._messageName, this); } catch (e) { if (e.result != Cr.NS_ERROR_NULL_POINTER) { throw e; @@ -744,6 +744,14 @@ // this point with a dead messageManager which only throws errors but does not // seem to indicate in any other way that it is dead. } + }, + + ready: function () { + this._addListener(); + }, + + close: function () { + this._removeListener(); this.emit("close"); this.hooks.onClosed(); }, @@ -756,7 +764,7 @@ send: function (packet) { this.emit("send", packet); try { - this._sender.sendAsyncMessage(this._messageName, packet); + this._mm.sendAsyncMessage(this._messageName, packet); } catch (e) { if (e.result != Cr.NS_ERROR_NULL_POINTER) { throw e; @@ -769,7 +777,13 @@ startBulkSend: function () { throw new Error("Can't send bulk data to child processes."); - } + }, + + swapBrowser(mm) { + this._removeListener(); + this._mm = mm; + this._addListener(); + }, }; exports.ChildDebuggerTransport = ChildDebuggerTransport;