From cdb5875c63af93bc9cc94ed793f18d12dd7a1e9a Mon Sep 17 00:00:00 2001 From: Razvan Caliman Date: Tue, 6 Nov 2018 10:42:52 +0000 Subject: [PATCH] Bug 1502857 - Refactor loading tools in Inspector sidebar; r=gl - removes duplication of logic to create each panel. - removes needless exposing of panel instances as properties on `inspector`: - `inspector.layoutview` - `inspector.fontinspector` - `inspector.animationinspector` - `inspector.changesview` - updates tests to not rely on those exposed properties and instead call `inspector.getPanel(toolId)` (previously created panels are stored and a reference is returned). - consolidates panel `destroy()` so we don't have to remember to destroy them individually. MozReview-Commit-ID: GVkP6z7FxKt Differential Revision: https://phabricator.services.mozilla.com/D10053 --HG-- extra : moz-landing-system : lando --- .../client/inspector/animation/test/head.js | 6 +- .../client/inspector/changes/ChangesView.js | 29 ++- devtools/client/inspector/fonts/test/head.js | 2 +- devtools/client/inspector/inspector.js | 178 +++++++----------- devtools/client/inspector/test/shared-head.js | 10 +- 5 files changed, 88 insertions(+), 137 deletions(-) diff --git a/devtools/client/inspector/animation/test/head.js b/devtools/client/inspector/animation/test/head.js index 3d10eb011cad..e7922fab1157 100644 --- a/devtools/client/inspector/animation/test/head.js +++ b/devtools/client/inspector/animation/test/head.js @@ -31,7 +31,7 @@ registerCleanupFunction(() => { const openAnimationInspector = async function() { const { inspector, toolbox } = await openInspectorSidebarTab(TAB_NAME); await inspector.once("inspector-updated"); - const { animationinspector: animationInspector } = inspector; + const animationInspector = inspector.getPanel("animationinspector"); await waitForRendering(animationInspector); const panel = inspector.panelWin.document.getElementById("animation-container"); return { animationInspector, toolbox, inspector, panel }; @@ -405,7 +405,7 @@ const selectAnimationInspector = async function(inspector) { const onUpdated = inspector.once("inspector-updated"); inspector.sidebar.select("animationinspector"); await onUpdated; - await waitForRendering(inspector.animationinspector); + await waitForRendering(inspector.getPanel("animationinspector")); }; /** @@ -428,7 +428,7 @@ const selectNodeAndWaitForAnimations = async function(data, inspector, reason = const onUpdated = inspector.once("inspector-updated"); await selectNode(data, inspector, reason); await onUpdated; - await waitForRendering(inspector.animationinspector); + await waitForRendering(inspector.getPanel("animationinspector")); }; /** diff --git a/devtools/client/inspector/changes/ChangesView.js b/devtools/client/inspector/changes/ChangesView.js index 5d6cca0719a6..4e0f33817bba 100644 --- a/devtools/client/inspector/changes/ChangesView.js +++ b/devtools/client/inspector/changes/ChangesView.js @@ -47,23 +47,18 @@ class ChangesView { this.inspector.target.on("will-navigate", this.onClearChanges); - // Sync the store to the changes stored on the server. The - // syncChangesToServer() method is async, but we don't await it since - // this method itself is NOT async. The call will be made in its own - // time, which is fine since it definitionally brings us up-to-date - // with the server at that moment. - this.syncChangesToServer(); - } - - async syncChangesToServer() { - // Empty the store. - this.onClearChanges(); - - // Add back in all the changes from the changesFront. - const changes = await this.changesFront.allChanges(); - changes.forEach((change) => { - this.onAddChange(change); - }); + // Get all changes collected up to this point by the ChangesActor on the server, + // then push them to the Redux store here on the client. + this.changesFront.allChanges() + .then(changes => { + changes.forEach(change => { + this.onAddChange(change); + }); + }) + .catch(err => { + // The connection to the server may have been cut, for example during test + // teardown. Here we just catch the error and silently ignore it. + }); } onAddChange(change) { diff --git a/devtools/client/inspector/fonts/test/head.js b/devtools/client/inspector/fonts/test/head.js index f1f17bda8e8b..99675e4793ef 100644 --- a/devtools/client/inspector/fonts/test/head.js +++ b/devtools/client/inspector/fonts/test/head.js @@ -56,7 +56,7 @@ var openFontInspectorForURL = async function(url) { testActor, toolbox, inspector, - view: inspector.fontinspector, + view: inspector.getPanel("fontinspector"), }; }; diff --git a/devtools/client/inspector/inspector.js b/devtools/client/inspector/inspector.js index 7c5cc5bb4956..9e0d7b0f2c95 100644 --- a/devtools/client/inspector/inspector.js +++ b/devtools/client/inspector/inspector.js @@ -835,16 +835,13 @@ Inspector.prototype = { if (this._panels.has(id)) { return this._panels.get(id); } + let panel; switch (id) { - case "computedview": - const {ComputedViewTool} = - this.browserRequire("devtools/client/inspector/computed/computed"); - panel = new ComputedViewTool(this, this.panelWin); - break; - case "ruleview": - const {RuleViewTool} = require("devtools/client/inspector/rules/rules"); - panel = new RuleViewTool(this, this.panelWin); + case "animationinspector": + const AnimationInspector = + this.browserRequire("devtools/client/inspector/animation/animation"); + panel = new AnimationInspector(this, this.panelWin); break; case "boxmodel": // box-model isn't a panel on its own, it used to, now it is being used by @@ -852,11 +849,39 @@ Inspector.prototype = { const BoxModel = require("devtools/client/inspector/boxmodel/box-model"); panel = new BoxModel(this, this.panelWin); break; + case "changesview": + const ChangesView = + this.browserRequire("devtools/client/inspector/changes/ChangesView"); + panel = new ChangesView(this, this.panelWin); + break; + case "computedview": + const {ComputedViewTool} = + this.browserRequire("devtools/client/inspector/computed/computed"); + panel = new ComputedViewTool(this, this.panelWin); + break; + case "fontinspector": + const FontInspector = + this.browserRequire("devtools/client/inspector/fonts/fonts"); + panel = new FontInspector(this, this.panelWin); + break; + case "layoutview": + const LayoutView = + this.browserRequire("devtools/client/inspector/layout/layout"); + panel = new LayoutView(this, this.panelWin); + break; + case "ruleview": + const {RuleViewTool} = require("devtools/client/inspector/rules/rules"); + panel = new RuleViewTool(this, this.panelWin); + break; default: // This is a custom panel or a non lazy-loaded one. return null; } - this._panels.set(id, panel); + + if (panel) { + this._panels.set(id, panel); + } + return panel; }, @@ -895,103 +920,50 @@ Inspector.prototype = { await this.addRuleView({ defaultTab }); - // Inject a lazy loaded react tab by exposing a fake React object - // with a lazy defined Tab thanks to `panel` being a function - const layoutId = "layoutview"; - const layoutTitle = INSPECTOR_L10N.getStr("inspector.sidebar.layoutViewTitle2"); - this.sidebar.queueTab( - layoutId, - layoutTitle, + // Inspector sidebar panels in order of appearance. + const sidebarPanels = [ { - props: { - id: layoutId, - title: layoutTitle, - }, - panel: () => { - if (!this.layoutview) { - const LayoutView = - this.browserRequire("devtools/client/inspector/layout/layout"); - this.layoutview = new LayoutView(this, this.panelWin); - } - - return this.layoutview.provider; - }, + id: "layoutview", + title: INSPECTOR_L10N.getStr("inspector.sidebar.layoutViewTitle2"), }, - defaultTab == layoutId); - - this.sidebar.queueExistingTab( - "computedview", - INSPECTOR_L10N.getStr("inspector.sidebar.computedViewTitle"), - defaultTab == "computedview"); - - const animationId = "animationinspector"; - const animationTitle = - INSPECTOR_L10N.getStr("inspector.sidebar.animationInspectorTitle"); - this.sidebar.queueTab( - animationId, - animationTitle, { - props: { - id: animationId, - title: animationTitle, - }, - panel: () => { - const AnimationInspector = - this.browserRequire("devtools/client/inspector/animation/animation"); - this.animationinspector = new AnimationInspector(this, this.panelWin); - return this.animationinspector.provider; - }, + id: "computedview", + title: INSPECTOR_L10N.getStr("inspector.sidebar.computedViewTitle"), }, - defaultTab == animationId); - - // Inject a lazy loaded react tab by exposing a fake React object - // with a lazy defined Tab thanks to `panel` being a function - const fontId = "fontinspector"; - const fontTitle = INSPECTOR_L10N.getStr("inspector.sidebar.fontInspectorTitle"); - this.sidebar.queueTab( - fontId, - fontTitle, { - props: { - id: fontId, - title: fontTitle, - }, - panel: () => { - if (!this.fontinspector) { - const FontInspector = - this.browserRequire("devtools/client/inspector/fonts/fonts"); - this.fontinspector = new FontInspector(this, this.panelWin); - } - - return this.fontinspector.provider; - }, + id: "animationinspector", + title: INSPECTOR_L10N.getStr("inspector.sidebar.animationInspectorTitle"), }, - defaultTab == fontId); + { + id: "fontinspector", + title: INSPECTOR_L10N.getStr("inspector.sidebar.fontInspectorTitle"), + }, + { + id: "changesview", + title: INSPECTOR_L10N.getStr("inspector.sidebar.changesViewTitle"), + }, + ]; - if (Services.prefs.getBoolPref(TRACK_CHANGES_PREF)) { - // Inject a lazy loaded react tab by exposing a fake React object - // with a lazy defined Tab thanks to `panel` being a function - const changesId = "changesview"; - const changesTitle = INSPECTOR_L10N.getStr("inspector.sidebar.changesViewTitle"); - this.sidebar.queueTab( - changesId, - changesTitle, - { + for (const { id, title } of sidebarPanels) { + // The Computed panel is not a React-based panel. We pick its element container from + // the DOM and wrap it in a React component (InspectorTabPanel) so it behaves like + // other panels when using the Inspector's tool sidebar. + if (id === "computedview") { + this.sidebar.queueExistingTab(id, title, defaultTab === id); + } else { + // When `panel` is a function, it is called when the tab should render. It is + // expected to return a React component to populate the tab's content area. + // Calling this method on-demand allows us to lazy-load the requested panel. + this.sidebar.queueTab(id, title, { props: { - id: changesId, - title: changesTitle, + id, + title, }, panel: () => { - if (!this.changesView) { - const ChangesView = - this.browserRequire("devtools/client/inspector/changes/ChangesView"); - this.changesView = new ChangesView(this, this.panelWin); - } - - return this.changesView.provider; + return this.getPanel(id).provider; }, - }, - defaultTab == changesId); + }, defaultTab === id); + } } this.sidebar.addAllQueuedTabs(); @@ -1440,22 +1412,6 @@ Inspector.prototype = { } this._panels.clear(); - if (this.layoutview) { - this.layoutview.destroy(); - } - - if (this.changesView) { - this.changesView.destroy(); - } - - if (this.fontinspector) { - this.fontinspector.destroy(); - } - - if (this.animationinspector) { - this.animationinspector.destroy(); - } - if (this._highlighters) { this._highlighters.destroy(); this._highlighters = null; diff --git a/devtools/client/inspector/test/shared-head.js b/devtools/client/inspector/test/shared-head.js index be43140e0763..ede6f9b9376e 100644 --- a/devtools/client/inspector/test/shared-head.js +++ b/devtools/client/inspector/test/shared-head.js @@ -133,7 +133,7 @@ function openChangesView() { toolbox: data.toolbox, inspector: data.inspector, testActor: data.testActor, - view: data.inspector.changesView, + view: data.inspector.getPanel("changesview"), }; }); } @@ -163,9 +163,9 @@ function openLayoutView() { toolbox: data.toolbox, inspector: data.inspector, boxmodel: data.inspector.getPanel("boxmodel"), - gridInspector: data.inspector.layoutview.gridInspector, - flexboxInspector: data.inspector.layoutview.flexboxInspector, - layoutView: data.inspector.layoutview, + gridInspector: data.inspector.getPanel("layoutview").gridInspector, + flexboxInspector: data.inspector.getPanel("layoutview").flexboxInspector, + layoutView: data.inspector.getPanel("layoutview"), testActor: data.testActor, }; }); @@ -203,7 +203,7 @@ function selectComputedView(inspector) { */ function selectChangesView(inspector) { inspector.sidebar.select("changesview"); - return inspector.changesView; + return inspector.getPanel("changesview"); } /**