1305979 - Improve API for adding a new side panels

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The inspector panel (and its sidebar) are missing simple API for adding new side panels (React components).

Let's improve this a bit (it can also be utilized by future WebExtension API)

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1305979.patch

Julian, simple improvement of the Inspector API for adding new panels.
Extension (or WebExtension API) can utilize it.

Here is a simple extension showing how to use the API.
https://github.com/janodvarko/prototypes/blob/master/inspector-sidebar/index.js#L23

Do you know about an existing test that I could clone to test this API?

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=80cf169cf32f

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8795693 [details] [diff] [review]
bug1305979.patch

Review of attachment 8795693 [details] [diff] [review]:
-----------------------------------------------------------------

I agree we need a test, but I can't think of any existing one to reuse here.
R+ for this patch, I suppose you'll attach the test in a separate patch? Otherwise feel free to reflag.

::: devtools/client/inspector/inspector-panel.js
@@ +578,5 @@
>      this.sidebar.show(defaultTab);
>    },
>  
> +  /**
> +   * Register a side-panel tab.

This API has no consumer in the current codebase.

The comment here should make it clear that this is intended to be used outside of devtools. Let's also add a unit-test to avoid it from being deleted randomly.

@@ +585,5 @@
> +   * @param {string} title tab title
> +   * @param {boolean} selected true if the panel should be selected
> +   * @param {React.Component} panel component. See `InspectorPanelTab` as an example.
> +   */
> +  addSidebarTab: function (id, title, selected, panel) {

nit: small detail but what about swapping the position of the selected & panel attributes? panel seems to be of higher order than selected.

::: devtools/client/inspector/toolsidebar.js
@@ +91,5 @@
> +   * @param {string} title tab title
> +   * @param {boolean} selected true if the panel should be selected
> +   * @param {React.Component} panel component. See `InspectorPanelTab` as an example.
> +   */
> +  addTab: function (id, title, selected, panel) {

Same remark about the argument order selected <-> panel

@@ +122,4 @@
>     * The document must have a title, which will be used as the name of the tab.
>     *
>     * @param {string} tab uniq id
>     * @param {string} url

nit: let's add the missing title param while we're at it.

Comment 4

[Julian Descottes [:jdescottes]]

Related: do you know of any existing addon customizing the inspector? It would be great to work on a non trivial example of a consumer of this new API. (maybe the new Events panel could be a candidate here? Bug 1226640)

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1305979.patch

(In reply to Julian Descottes [:jdescottes] from comment #3)
> Comment on attachment 8795693 [details] [diff] [review]
> bug1305979.patch
> 
> Review of attachment 8795693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree we need a test, but I can't think of any existing one to reuse here.
> R+ for this patch, I suppose you'll attach the test in a separate patch?
Yes, additional patch will follow

> Otherwise feel free to reflag.
> 
> ::: devtools/client/inspector/inspector-panel.js
> @@ +578,5 @@
> >      this.sidebar.show(defaultTab);
> >    },
> >  
> > +  /**
> > +   * Register a side-panel tab.
> 
> This API has no consumer in the current codebase.
> 
> The comment here should make it clear that this is intended to be used
> outside of devtools. Let's also add a unit-test to avoid it from being
> deleted randomly.
Done

> 
> @@ +585,5 @@
> > +   * @param {string} title tab title
> > +   * @param {boolean} selected true if the panel should be selected
> > +   * @param {React.Component} panel component. See `InspectorPanelTab` as an example.
> > +   */
> > +  addSidebarTab: function (id, title, selected, panel) {
> 
> nit: small detail but what about swapping the position of the selected &
> panel attributes? panel seems to be of higher order than selected.
Done

> 
> ::: devtools/client/inspector/toolsidebar.js
> @@ +91,5 @@
> > +   * @param {string} title tab title
> > +   * @param {boolean} selected true if the panel should be selected
> > +   * @param {React.Component} panel component. See `InspectorPanelTab` as an example.
> > +   */
> > +  addTab: function (id, title, selected, panel) {
> 
> Same remark about the argument order selected <-> panel
Done

> 
> @@ +122,4 @@
> >     * The document must have a title, which will be used as the name of the tab.
> >     *
> >     * @param {string} tab uniq id
> >     * @param {string} url
> 
> nit: let's add the missing title param while we're at it.
Done


Thanks for the review!

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1305979-test.patch

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Julian Descottes [:jdescottes] from comment #4)
> Related: do you know of any existing addon customizing the inspector? It
> would be great to work on a non trivial example of a consumer of this new
Somebody was asking on IRC how to build and extension that appends a new side panel, but I don't have link to the actual extension. I build a simple example to test the API here:
https://github.com/janodvarko/prototypes/blob/master/inspector-sidebar/index.js

> API. (maybe the new Events panel could be a candidate here? Bug 1226640)
Yes, definitely. Note that the API can be consumed by extensions as well as DevTools code base (as the comment in the source code says).

Honza

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b8e0953ac97

Honza

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8796504 [details] [diff] [review]
bug1305979-test.patch

Review of attachment 8796504 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for adding a test! 

R+ with my comments addressed :)

::: devtools/client/inspector/test/browser_inspector_addSidebarTab.js
@@ +20,5 @@
> +
> +  info("Adding custom panel.");
> +
> +  // Define custom side-panel.
> +  var tabPanel = React.createFactory(React.createClass({

nit: var -> let 
eslint: component needs a display name

@@ +32,5 @@
> +  }));
> +
> +  // Append custom panel (tab) into the Inspector panel and
> +  // make sure it's selected by default (the last arg = true).
> +  inspector.addSidebarTab("myPanel", "My Panel", tabPanel, true);

Please test adding another panel with selected = false, and check the previous one is still selected.
(currently fails, see my comment in tabbar.js)

@@ +43,5 @@
> +    "Side panel content has been rendered.");
> +
> +  // Finish initialization of the computed panel before
> +  // destroying the toolbox.
> +  yield waitForTick();

I guess this and the toolbox.destroy() come from devtools/client/inspector/test/browser_inspector_sidebarstate.js ?

Here we can let the test close the toolbox. 
We don't need to wait for the computed panel since we don't select it.

::: devtools/client/shared/components/tabs/tabbar.js
@@ +54,5 @@
>        newState.activeTab = tabs.length - 1;
>      }
>  
> +    this.setState(newState, () => {
> +      if (this.props.onSelect) {

The callback should only be called if (this.props.onSelect && selected)

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1305979-test.patch

(In reply to Julian Descottes [:jdescottes] from comment #9)
> Comment on attachment 8796504 [details] [diff] [review]
> bug1305979-test.patch
> 
> Review of attachment 8796504 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for adding a test! 
> 
> R+ with my comments addressed :)
> 
> ::: devtools/client/inspector/test/browser_inspector_addSidebarTab.js
> @@ +20,5 @@
> > +
> > +  info("Adding custom panel.");
> > +
> > +  // Define custom side-panel.
> > +  var tabPanel = React.createFactory(React.createClass({
> 
> nit: var -> let 
> eslint: component needs a display name
Both fixed

> 
> @@ +32,5 @@
> > +  }));
> > +
> > +  // Append custom panel (tab) into the Inspector panel and
> > +  // make sure it's selected by default (the last arg = true).
> > +  inspector.addSidebarTab("myPanel", "My Panel", tabPanel, true);
> 
> Please test adding another panel with selected = false, and check the
> previous one is still selected.
> (currently fails, see my comment in tabbar.js)
Done

> 
> @@ +43,5 @@
> > +    "Side panel content has been rendered.");
> > +
> > +  // Finish initialization of the computed panel before
> > +  // destroying the toolbox.
> > +  yield waitForTick();
> 
> I guess this and the toolbox.destroy() come from
> devtools/client/inspector/test/browser_inspector_sidebarstate.js ?
Yes

> 
> Here we can let the test close the toolbox. 
> We don't need to wait for the computed panel since we don't select it.
Fixed

> 
> ::: devtools/client/shared/components/tabs/tabbar.js
> @@ +54,5 @@
> >        newState.activeTab = tabs.length - 1;
> >      }
> >  
> > +    this.setState(newState, () => {
> > +      if (this.props.onSelect) {
> 
> The callback should only be called if (this.props.onSelect && selected)
Done

Thanks!

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10e13245665e

Honza

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1305979-test.patch

Fixing ESlint warning.

Honza

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1305979-test.patch

One more update

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/13100266a434
Improve API for adding a new side panels. r=jdescottes
https://hg.mozilla.org/integration/fx-team/rev/5515701bca40
Test for addSidebarTab API. r=jdescottes

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13100266a434
https://hg.mozilla.org/mozilla-central/rev/5515701bca40