1341305 - Implement devtools.panels.elements.createSidebarPane

Status: VERIFIED FIXED

(WebExtensions :: Developer Tools, defect, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This bug is a followup of Bug 1341304 (Implement devtools.elements.onSelectionChanged) and its goal is to provide the `devtools.elements.createSidebarPane` API method, which is used by a devtools addon that wants to provide an additional custom sidebar to the Inspector panel (e.g. the Ember Inspector, which provides Ember informations related to the currently selected DOM element).  

This API method has the following signature on Chrome:

- chrome.devtools.elements.createSidebarPane(string title, function callback)

The newly created ExtensionSidebarPane instance, that is passed as the parameter of the method callback, contains the following properties:

- onShown / onHidden events

- setExpression(string expression, string rootTitle, function callback), where the expression is evaluated like in the inspectedWindow.eval method (e.g. it requires the $0, filed as Bug 1300590, to be defined, so that the expression can use it to retrieve the desidered properties from the current selected node) and then shown into a variables view panel, and the callback is called once the expression has been executed and its result set in the sidebar panel

- setObject(string jsonObject, string rootTitle, function callback), where the passed object is directly set in the variables view panel and the callback is called once the object has been set in the sidebar panel

- setPage(string path), where the passed string in a path to an extension url

A common usage pattern of this Inspector sidebar API is using the setExpression method paired with the devtools.elements.onSelectionChanged event.

See https://developer.chrome.com/extensions/devtools_panels#type-ExtensionSidebarPane for more information about this API.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Errata corrige: 
- the `elements` property should be inside the `devtools.panels` namespace (like in `devtools.panels.elements`)

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review commit: https://reviewboard.mozilla.org/r/162154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162154/

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review commit: https://reviewboard.mozilla.org/r/162156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/162156/

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162154/diff/1-2/

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/1-2/

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

https://reviewboard.mozilla.org/r/162156/#review170448

This looks good to me.  I guess we can't use all the schema-derived proxy stuff in the extension process for ExtensionSidebarPane since it is created dynamically and not just part of the browser/chrome namespace?  That's kind of a bummer, it would make the implementation here simpler (though the complexity elsewhere probably would not make it worthwhile)

::: browser/components/extensions/ext-c-devtools-panels.js:243
(Diff revision 2)
>      return {
>        devtools: {
>          panels: {
> +          elements: {
> +            createSidebarPane(title) {
> +              return context.cloneScope.Promise.resolve().then(async () => {

Is this necessary?  I think you can just decalre `createSidebarePane()` as `async` and get the same effect without needing any promise chaining or referencing `cloneScope`

::: browser/components/extensions/ext-devtools-panels.js:306
(Diff revision 2)
> +    this.onSidebarCreated = this.onSidebarCreated.bind(this);
> +
> +    this.toolbox.once(`extension-sidebar-created-${this.id}`, this.onSidebarCreated);
> +    this.toolbox.on(`inspector-sidebar-select`, this.onSidebarSelect);
> +
> +    this.waitSidebarCreated = new Promise(resolve => {

is this used anywhere?

::: browser/components/extensions/ext-devtools-panels.js:360
(Diff revision 2)
> +      // Defer the sidebar.setObject call.
> +      this._initializeSidebar = () => this.extensionSidebar.setObject(object);
> +    }
> +  }
> +
> +  close() {

nit: move this up to be right below the constructor?

::: browser/components/extensions/ext-devtools-panels.js:361
(Diff revision 2)
> +      this._initializeSidebar = () => this.extensionSidebar.setObject(object);
> +    }
> +  }
> +
> +  close() {
> +    if (this.destroyed) {

initialize `this.destroyed` to false in the constructor?

::: browser/components/extensions/ext-devtools-panels.js:400
(Diff revision 2)
>                  return () => {
>                    toolboxSelectionObserver.off("selectionChanged", listener);
>                  };
>                }).api(),
> +            createSidebarPane(title) {
> +              const baseId = `${context.extension.id}-${context.contextId}-${nextPanelId++}`;

nit: move this to a shared function?

::: browser/components/extensions/test/browser/browser_ext_devtools_panels_elements_sidebar.js:21
(Diff revision 2)
> +
> +    const onShownListener = (event, sidebarInstance) => {
> +      browser.test.sendMessage(`devtools_sidebar_${event}`, sidebarInstance);
> +    };
> +
> +    sidebar1.onShown.addListener(onShownListener.bind(null, "shown", "sidebar1"));

nit: just write the argument as `() => onShownListener("shown", "sidebar")` ?

::: browser/components/extensions/test/browser/browser_ext_devtools_panels_elements_sidebar.js:118
(Diff revision 2)
> +     "The first registered sidebar has been removed");
> +
> +  is(inspector.sidebar.getTabPanel(sidebarIds[1]), undefined,
> +     "The second registered sidebar has been removed");
> +
> +  // await new Promise(() => {});

take this out before landing

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

https://reviewboard.mozilla.org/r/162154/#review170406

::: devtools/client/framework/toolbox.js:99
(Diff revision 2)
>    this._target = target;
>    this._win = contentWindow;
>    this.frameId = frameId;
>  
>    this._toolPanels = new Map();
> +  this._inspectorExtensionSidebars = new Map();

Can this be a WeakMap so that removed extension sidebars can be easily GC'd?

::: devtools/client/framework/toolbox.js:1406
(Diff revision 2)
>    },
>  
>    /**
> +   * Register an extension sidebar for the inspector panel.
> +   *
> +   * @param {string} id an uniq sidebar id

s/an uniq/An unique
Move after after id to the next line

::: devtools/client/framework/toolbox.js:1410
(Diff revision 2)
> +   *
> +   * @param {string} id an uniq sidebar id
> +   * @param {string} title a title for the sidebar
> +   */
> +  async registerInspectorExtensionSidebar(id, title) {
> +    this._inspectorExtensionSidebars.set(id, {id, title});

I think we can move the if statement check on this statement, and then we can keep the larger part of the code below inline.

We can do:

if (!this._inspector) {
  this._inspectorExtensionSidebars.set(id, {id, title});
  return;
}

const inspector = this.getPanel...
...

::: devtools/client/framework/toolbox.js:1415
(Diff revision 2)
> +    this._inspectorExtensionSidebars.set(id, {id, title});
> +
> +    // Register the extension sidebar immediately if the inspector
> +    // has been already created.
> +    if (this._inspector) {
> +      const inspector = await this.getPanel("inspector");

this.getPanel doesn't return a promise. await is not needed.

::: devtools/client/framework/toolbox.js:1465
(Diff revision 2)
>    loadTool: function (id) {
>      if (id === "inspector" && !this._inspector) {
> -      return this.initInspector().then(() => {
> -        return this.loadTool(id);
> +      return this.initInspector().then(async () => {
> +        const res = await this.loadTool(id);
> +
> +        const inspector = await this.getPanel("inspector");

Doesn't need await for getPanel since this is doing a get on a Map. I also noticed that we do "const inspector = await this.getPanel("inspector");" on this file too and we should remove the await there as well.

::: devtools/client/framework/toolbox.js:1465
(Diff revision 2)
>    loadTool: function (id) {
>      if (id === "inspector" && !this._inspector) {
> -      return this.initInspector().then(() => {
> -        return this.loadTool(id);
> +      return this.initInspector().then(async () => {
> +        const res = await this.loadTool(id);
> +
> +        const inspector = await this.getPanel("inspector");

I don't believe we need to call createExtensionSidebar if we are depending on the tool loading first. A more appropriate place might be to call it in inspector.setupSidebar

http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#601

I think we might want to move the map and methods out of the toolbox and into the inspector.

::: devtools/client/framework/toolbox.js:1468
(Diff revision 2)
> -        return this.loadTool(id);
> +        const res = await this.loadTool(id);
> +
> +        const inspector = await this.getPanel("inspector");
> +
> +        // Create any additional sidebar registered by devtools extensions.
> +        for (const [sidebarId, {title}] of this._inspectorExtensionSidebars) {

I think this logic and the 2 methods for register/unregisterInspectorExtensionSidebar should be moved into inspector.js. 

Since we require the inspector to be loaded anyways, we might as well have a method called setupSidebarExtension() in inspector.js that is called after setupSidebar() in _deferredOpen

::: devtools/client/inspector/extensions/actions/index.js:12
(Diff revision 2)
> +const { createEnum } = require("devtools/client/shared/enum");
> +
> +createEnum([
> +
> +  // Update the extension sidebar with an object TreeView.
> +  "EXTENSION_SIDEBAR_OBJECT_TREEVIEW",

Maybe "EXTENSION_SIDEBAR_OBJECT_TREEVIEW_UPDATE"? We should postfix it was _UPDATE to be clear what this is doing

::: devtools/client/inspector/extensions/actions/sidebar.js:17
(Diff revision 2)
> +module.exports = {
> +
> +  /**
> +   * Update the sidebar with an object treeview.
> +   */
> +  objectTreeView(sidebarId, object) {

s/objectTreeView/updateObjectTreeView

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:25
(Diff revision 2)
> +
> +/**
> + * The ObjectTreeView React Component is used in the SidebarComponent to provide
> + * a UI viewMode which shows a tree view of the passed JavaScript object.
> + */
> +const ObjectTreeView = createFactory(createClass({

Might want to consider splitting this into its own component file.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:35
(Diff revision 2)
> +  },
> +
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  render() {
> +    const {object} = this.props;

s/{object}/{ object }

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:53
(Diff revision 2)
> +        cropLimit: 50,
> +      }));
> +    };
> +
> +    return TreeView({
> +      object: object,

This can just be object

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:55
(Diff revision 2)
> +    };
> +
> +    return TreeView({
> +      object: object,
> +      mode: MODE.SHORT,
> +      columns: columns,

This can just be columns

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:56
(Diff revision 2)
> +
> +    return TreeView({
> +      object: object,
> +      mode: MODE.SHORT,
> +      columns: columns,
> +      renderValue: renderValue,

This can just be renderValue,

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:73
(Diff revision 2)
> + * - an ExtensionPage UI used to show an extension page (which is used by the sidebar.setPage
> + *   WebExtensions APIs).
> + *
> + * TODO: implement the ExtensionPage viewMode.
> + */
> +const SidebarComponent = createClass({

s/SidebarComponent/ExtensionSidebar

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:83
(Diff revision 2)
> +  },
> +
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  render() {
> +    const {id} = this.props;

s/{id}/{ id }

::: devtools/client/inspector/extensions/extension-sidebar.js:30
(Diff revision 2)
> + * (using the registerInspectorExtensionSidebar method) and, once the Inspector has been
> + * created, the toolbox uses the Inpector createExtensionSidebar method to create the
> + * ExtensionSidebar instances and then it registers them to the Inspector.
> + *
> + * @param {Inspector} inspector
> + *   The inspector where the sidebar should be hooked to.

These JS param description needs to be indented up until it is below the '{' of the previous line.

::: devtools/client/inspector/extensions/extension-sidebar.js:32
(Diff revision 2)
> + * ExtensionSidebar instances and then it registers them to the Inspector.
> + *
> + * @param {Inspector} inspector
> + *   The inspector where the sidebar should be hooked to.
> + * @param {Object} options
> + * @param {string} extensionId

s/string/String/g

::: devtools/client/inspector/extensions/extension-sidebar.js:32
(Diff revision 2)
> + * ExtensionSidebar instances and then it registers them to the Inspector.
> + *
> + * @param {Inspector} inspector
> + *   The inspector where the sidebar should be hooked to.
> + * @param {Object} options
> + * @param {string} extensionId

s/extensionId/options.extensionId

::: devtools/client/inspector/extensions/extension-sidebar.js:34
(Diff revision 2)
> + * @param {Inspector} inspector
> + *   The inspector where the sidebar should be hooked to.
> + * @param {Object} options
> + * @param {string} extensionId
> + *   The add-on Id of the extension that owns the sidebar.
> + * @param {string} id

s/id/options.id

::: devtools/client/inspector/extensions/extension-sidebar.js:36
(Diff revision 2)
> + * @param {Object} options
> + * @param {string} extensionId
> + *   The add-on Id of the extension that owns the sidebar.
> + * @param {string} id
> + *   The uniq id of the sidebar.
> + * @param {string} title

s/title/options.title

::: devtools/client/inspector/extensions/extension-sidebar.js:68
(Diff revision 2)
> +
> +  /**
> +   * Remove the sidebar from the related inspector instance and dispatch a removeExtensionSidebar
> +   * action which removes the related state from the Inspector store.
> +   */
> +  remove() {

We should make sure we have an appropriate destroy method for ExtensionSidebar that aren't needed anymore. We need to remove our references and set this.inspector/store/extensionId/id/title/_provider to null if we don't need this anymore after it is removed.

::: devtools/client/inspector/extensions/test/head.js:7
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +/* eslint no-unused-vars: [2, {"vars": "local"}] */
> +/* import-globals-from ../../test/head.js */
> +"use strict";

Add a new line before "use strict";

::: devtools/client/inspector/inspector.js:673
(Diff revision 2)
> +   * @returns {ExtensionSidebar} an object which expose a simple API to control
> +   *   the sidebar panel.
> +   */
> +  createExtensionSidebar: function (id, title, extensionId) {
> +    const extensionSidebar = new ExtensionSidebar(this, {extensionId, id, title});
> +    this.sidebar.addTab(id, title, extensionSidebar.provider, false);

Use this.addSidebarTab(id, title, extensionSidebar.provider, false);

::: devtools/client/inspector/reducers.js:15
(Diff revision 2)
>  exports.boxModel = require("devtools/client/inspector/boxmodel/reducers/box-model");
>  exports.fontOptions = require("devtools/client/inspector/fonts/reducers/font-options");
>  exports.fonts = require("devtools/client/inspector/fonts/reducers/fonts");
>  exports.grids = require("devtools/client/inspector/grids/reducers/grids");
>  exports.highlighterSettings = require("devtools/client/inspector/grids/reducers/highlighter-settings");
> +exports.extensionsSidebar = require("devtools/client/inspector/extensions/reducers/sidebar");

This list should be alphabetically sorted. So, it should be move after exports.boxModel

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162154/diff/2-3/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/2-3/

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

https://reviewboard.mozilla.org/r/162156/#review170448

> Is this necessary?  I think you can just decalre `createSidebarePane()` as `async` and get the same effect without needing any promise chaining or referencing `cloneScope`

yeah, unfortunately it is needed, because if the function is marked as async, then it currently returns a promise object that the extension code has not the privileges to use (because it has been created by the chrome privileged code).

it is also used by the panels.create method for the same reason, in the meantime I added an inline comment which explains why it is needed near to where it is used.

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/3-4/

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162154/diff/3-4/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/4-5/

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

https://reviewboard.mozilla.org/r/162154/#review176608

::: commit-message-7ce55:1
(Diff revision 4)
> +Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Needs to add r=gl at the end of commit-message

::: devtools/client/framework/toolbox.js:1452
(Diff revision 4)
>  
>    /**
> +   * Retrieve the registered inspector extension sidebars
> +   * (used by the inspector panel during its deferred initialization).
> +   */
> +

Remove this empty new line.

::: devtools/client/framework/toolbox.js:1466
(Diff revision 4)
> +   *        An unique sidebar id
> +   * @param {Object} options
> +   * @param {String} options.title
> +   *        A title for the sidebar
> +   */
> +  async registerInspectorExtensionSidebar(id, {title}) {

It doesn't look like we need to do this destructuring  if we are gonna pass the same object into the Map. It is probably safe to s/{title}/options and also set the id with the options. I would imagine any new changes to the options would also have to be saved.

::: devtools/client/framework/toolbox.js:1483
(Diff revision 4)
> +  },
> +
> +  /**
> +   * Unregister an extension sidebar for the inspector panel.
> +   *
> +   * @param {string} id

s/string/String

::: devtools/client/framework/toolbox.js:1488
(Diff revision 4)
> +   * @param {string} id
> +   *        An unique sidebar id
> +   */
> +  unregisterInspectorExtensionSidebar(id) {
> +    const sidebarDef = this._inspectorExtensionSidebars.get(id);
> +    if (sidebarDef) {

Do an early return instead, so you can keep everything in the if block un-indented.

if (!sidebarDef) {
  return;
}

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:17
(Diff revision 4)
> +} = require("devtools/client/shared/vendor/react");
> +
> +const { connect } = require("devtools/client/shared/vendor/react-redux");
> +
> +const ObjectTreeViewClass = require("./ObjectTreeView");
> +const ObjectTreeView = createFactory(ObjectTreeViewClass);

s/ObjectTreeViewClass/require("./ObjectTreeView")

There is no need to declare ObjecttreeViewClass.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:20
(Diff revision 4)
> +
> +const ObjectTreeViewClass = require("./ObjectTreeView");
> +const ObjectTreeView = createFactory(ObjectTreeViewClass);
> +
> +/**
> + * The SidebarComponent is a React component with 2 supported viewMode:

s/SidebarComponent/ExtensionSidebar

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:38
(Diff revision 4)
> +  },
> +
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  render() {
> +    const { id } = this.props;

extensionsSidebar should be destructured here.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:40
(Diff revision 4)
> +  mixins: [ addons.PureRenderMixin ],
> +
> +  render() {
> +    const { id } = this.props;
> +
> +    // Use the sidebar uniq id to retrieve the sidebar state from the Redux Store.

This comment is probably unnecessary

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:42
(Diff revision 4)
> +  render() {
> +    const { id } = this.props;
> +
> +    // Use the sidebar uniq id to retrieve the sidebar state from the Redux Store.
> +    let {
> +      viewMode,

You can specify the default value when destructuring: { viewMode = "empty-sidebar" } = this.props.extensionsSidebar

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:44
(Diff revision 4)
> +
> +    // Use the sidebar uniq id to retrieve the sidebar state from the Redux Store.
> +    let {
> +      viewMode,
> +      object
> +    } = this.props.extensionsSidebar[id] || {};

extensionsSidebar prop is not declared in the propTypes.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:46
(Diff revision 4)
> +    let {
> +      viewMode,
> +      object
> +    } = this.props.extensionsSidebar[id] || {};
> +
> +    // Default the viewMode to an empty-sidebar.

These 2 new lines aren't necessary after specifying the default value when destructuring.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:55
(Diff revision 4)
> +
> +    switch (viewMode) {
> +      case "object-treeview":
> +        sidebarContentEl = ObjectTreeView({object});
> +        break;
> +

Remove this empty new line.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:58
(Diff revision 4)
> +        sidebarContentEl = ObjectTreeView({object});
> +        break;
> +
> +      case "empty-sidebar":
> +        break;
> +

Remove this empty new line.

::: devtools/client/inspector/extensions/components/ExtensionSidebar.js:65
(Diff revision 4)
> +        throw new Error(`Unknown ExtensionSidebar viewMode: "${viewMode}"`);
> +    }
> +
> +    const className = "devtools-monospace extension-sidebar inspector-tabpanel";
> +
> +    return dom.div({id, className}, sidebarContentEl);

s/{id, className}/{ id, className }

::: devtools/client/inspector/extensions/components/ObjectTreeView.js:16
(Diff revision 4)
> +} = require("devtools/client/shared/vendor/react");
> +
> +const { REPS, MODE } = require("devtools/client/shared/components/reps/reps");
> +const { Rep } = REPS;
> +const TreeViewClass = require("devtools/client/shared/components/tree/tree-view");
> +const TreeView = createFactory(TreeViewClass);

s/TreeViewClass/require("devtools/client/shared/components/tree/tree-view")

::: devtools/client/inspector/extensions/components/ObjectTreeView.js:19
(Diff revision 4)
> +const { Rep } = REPS;
> +const TreeViewClass = require("devtools/client/shared/components/tree/tree-view");
> +const TreeView = createFactory(TreeViewClass);
> +
> +/**
> + * The ObjectTreeView React Component is used in the SidebarComponent to provide

s/SidebarComponent/ExtensionSidebar

::: devtools/client/inspector/extensions/components/ObjectTreeView.js:23
(Diff revision 4)
> +/**
> + * The ObjectTreeView React Component is used in the SidebarComponent to provide
> + * a UI viewMode which shows a tree view of the passed JavaScript object.
> + */
> +const ObjectTreeView = createClass({
> +  displayName: "ExtensionSidebarObjectTreeView",

s/ExtensionsSidebarObjectTreeView/ObjectTreeView

displayName should reflect the file name.

::: devtools/client/inspector/extensions/components/ObjectTreeView.js:38
(Diff revision 4)
> +
> +    let columns = [{
> +      "id": "value",
> +    }];
> +
> +    // Render the node value (omitted on the root element if it has childrens).

s/childrens/children

::: devtools/client/inspector/extensions/components/ObjectTreeView.js:41
(Diff revision 4)
> +    }];
> +
> +    // Render the node value (omitted on the root element if it has childrens).
> +    let renderValue = props => {
> +      if (props.member.level === 0 && props.member.hasChildren) {
> +        return undefined;

s/return undefined;/return;

::: devtools/client/inspector/inspector.js:709
(Diff revision 4)
> +   *
> +   * @param {String} id
> +   *        The id of the sidebar tab to destroy.
> +   */
> +  removeExtensionSidebar: function (id) {
> +    if (this._panels.has(id)) {

Do a early return so, we can just keep all this main logic un-indented.

if (!this._panels.has(id)) {
  return;
}

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

There is also an ObjectInspector that is shared between the console and debugger, which might be doing the same thing as the TreeView and Reps. I would check with nchevobbe, bgrins or jlast if we could integrate it here.

Comment 16

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #15)
> There is also an ObjectInspector that is shared between the console and
> debugger, which might be doing the same thing as the TreeView and Reps. I
> would check with nchevobbe, bgrins or jlast if we could integrate it here.


Yeah, sounds good to me, if we could share more and get something simpler it would be great, I'm adding a needinfo assigned to both nchevobbe and jlast to get their point of view.

In case it can be helpful to evaluate this, it is probably worth to mention that before going for the custom component I took a look at the existent devtools UIs that expose a similar UI (e.g. the JSONViewer and the ObjectInspector) and (at least iirc) the ObjectInspector seems to be more oriented to be used for objects that are coming from the RDP protocol (e.g. it is able to retrieve the other "remote" pieces of the objects on demand when the user navigate the tree), and so I opted to reuse the shared TreeView component to create a small custom component which was enough to cover the necessary needs (which is to render a plain local json object).

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162154/diff/4-5/

Comment 18

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/5-6/

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

> In case it can be helpful to evaluate this, it is probably worth to mention that before going for the custom component I took a look at the existent devtools UIs that expose a similar UI (e.g. the JSONViewer and the ObjectInspector) and (at least iirc) the ObjectInspector seems to be more oriented to be used for objects that are coming from the RDP protocol (e.g. it is able to retrieve the other "remote" pieces of the objects on demand when the user navigate the tree), and so I opted to reuse the shared TreeView component to create a small custom component which was enough to cover the necessary needs (which is to render a plain local json object).

You are totally right Luca, the ObjectInspector is built on top of the Tree component and handle loading things through the RDP protocol. And even if we could use it as a plain local json tree too, it is not doable at the moment (we expressly checks that we have a RDP grip in order to tell if a node can be expanded).

Not sure if it would be worth it to tweak it so we can handle local json objects too, it is really aimed at RDP objects at the moment, and would be even more as I'm working on a patch that would require consumer to pass a function to create ObjectClient.

Comment 20

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

https://reviewboard.mozilla.org/r/162154/#review177608

::: devtools/client/framework/toolbox.js:1465
(Diff revisions 4 - 5)
> +   * @param {String} options.id
> +   *        An unique sidebar id
>     * @param {String} options.title
>     *        A title for the sidebar
>     */
> -  async registerInspectorExtensionSidebar(id, {title}) {
> +  async registerInspectorExtensionSidebar(options) {

I think there was a misunderstanding from my last review comment. I meant that we would probably do set(id, options) in we ever added more items into the options object. So, there was no need to destructure options, but to always make sure we store all the options in the inspectExtensionSidebars Map. 

I would prefer to revert the changes where we wrapped the sidebar id into the options.

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162154/diff/5-6/

Comment 22

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/6-7/

Comment 23

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

https://reviewboard.mozilla.org/r/162154/#review176608

> s/return undefined;/return;

I had to leave the explicit `return undefined` here (otherwire eslint complains that the arrow function doesn't always return a value).

Comment 24

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890979 [details]
Bug 1341305 - Provide an internal DevTools API to register an Extension Sidebar for the inspector panel.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162154/diff/6-7/

Comment 25

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8890980 [details]
Bug 1341305 - Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162156/diff/7-8/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/656e084e334f
Provide an internal DevTools API to register an Extension Sidebar for the inspector panel. r=gl
https://hg.mozilla.org/integration/autoland/rev/9b8dbd9c83ab
Implement devtools.panels.elements.createSidebarPane and sidebar's setObject APIs. r=aswan

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/656e084e334f
https://hg.mozilla.org/mozilla-central/rev/9b8dbd9c83ab

Comment 28

[Dan]

This doesn't work in Nightly - 57.0a1 (2017-09-07)

The Sidebar Panel is created, but the title does not display above the tab. The title is displayed in the overflow menu though.

The implementation also appears incomplete as the setPage method is not available. 

My comments are based on the fact that this is marked as RESOLVED FIXED.

Regards,
Dan

Comment 29

[Will Bamberg [:wbamberg]]

Luca, I've written some pages for this:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/ElementsPanel/createSidebarPane
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/ExtensionSidebarPane

A few things came up.

* `setExpression`:
    * `rootTitle` is described[1] as optional. But when I omitted it (for example, if I omitted "" from the example[2] (that I copied from the test code), nothing was displayed.
    * The JSON description [3] says "JavaScript objects and DOM nodes are displayed in an expandable tree". I'm not sure what exactly "JavaScript object" means here, but I tried with a DOM node ($0) and I get "Object [Object]", not an expandable tree.


* `onShown` and `onHidden`: the JSON for these says "Fired when the sidebar pane becomes visible as a result of user switching to the panel that hosts it." "The panel" in this context I'd take to be the Inspector, but the behavior I see (and have documented) is that these events fire when the user selects a tab in the sidebar itself, and never when the user selects a different tool.

[1] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/devtools_panels.json#218
[2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/devtools.panels/ExtensionSidebarPane/setExpression#Examples
[3] http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/devtools_panels.json#190

Comment 30

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Will,

I just tested these scenarios on both Firefox and Chrome to get a picture of which ones of them are going to be compatibility issues:

- sidebar.setObject (and optional rootTitle)
  - on Chrome sidebar.setObject doesn't show any property if the passed object is a string
  - on Firefox sidebar.setObject currently allows to use a string as the passed object, but the panel doesn't render the string object if the rootTitle is missing (but the rootTitle can be omitted when the passed object is a js object or an array and the sidebar is rendered), I've just filed Bug 1412310 related to the missing rootTitle behavior 

- sidebar.setExpression
  - on Chrome sidebar.setExpression, the expression have to return a js object or a DOM node (while if the evaluated expression returns a plain string, nothing is shown in the sidebar)
  - on Firefox sidebar.setExpression doesn't currently support expressions that returns a non-serializable result (e.g. a js cyclic object or a DOM node), fixing this incompatibility has been filed as Bug 1403130

- sidebar.onShown and sidebar.onHidden
  - on Chrome sidebar.onShown/onHidden are fired when the sidebar panel has been selected or deselected,
    but it also send this events if the selected devtools panel has been changed while the particular sidebar panel
    was the currently selected one.
  - on Firefox sidebar.onShown/onHidden are not currently fired when the currently selected devtools panel has been changes
    even if the particular sidebar panel was the currently selected one, I've just filed Bug 1412317 to fix this incompatibility with the behavior on Chrome.

Comment 31

[Luca Greco [:rpl] [:luca] [:lgreco]]

cleared an old needinfo.

Comment 32

[Will Bamberg [:wbamberg]]

Thanks for the detailed analysis, Luca. I've filed a PR to update the compat notes: https://github.com/mdn/browser-compat-data/pull/567.

Comment 33

[Will Bamberg [:wbamberg]]

Marking this dev-doc-complete, but let me know if we need anything else.

Comment 34

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 35

[CosminB]

This issue is verified as fixed on Firefox 59.0a1 (20171128100440), Firefox 58.0b7 (20171127135700) and Firefox 57.0 (20171112125346) under Wind 7 64-bit and Mac OS X 10.13.1

The `devtools.elements.createSidebarPane` API method is implemented and it has been already verified that works in Bug 1398729 and Bug 1398727.