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
2018-11-06 10:42:52 +00:00 · 2018-11-06 10:42:52 +00:00 · cdb5875c63
--- 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"));
 };

 /**
--- 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) {
--- 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"),
  };
 };

--- 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;
--- 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");
 }

 /**