Closed Bug 1305979 Opened 5 years ago Closed 5 years ago

Improve API for adding a new side panels

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox52 fixed)

RESOLVED FIXED
Firefox 52
Tracking Status
firefox52 --- fixed

People

(Reporter: Honza, Assigned: Honza)

References

Details

Attachments

(2 files, 4 obsolete files)

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
Attached patch bug1305979.patch (obsolete) — Splinter Review
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
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Attachment #8795693 - Flags: review?(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.
Attachment #8795693 - Flags: review?(jdescottes) → review+
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)
Attached patch bug1305979.patchSplinter Review
(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
Attachment #8795693 - Attachment is obsolete: true
Attachment #8796503 - Flags: review+
Attached patch bug1305979-test.patch (obsolete) — Splinter Review
Attachment #8796504 - Flags: review?(jdescottes)
(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 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)
Attachment #8796504 - Flags: review?(jdescottes) → review+
Attached patch bug1305979-test.patch (obsolete) — Splinter Review
(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
Attachment #8796504 - Attachment is obsolete: true
Attachment #8796577 - Flags: review+
Attached patch bug1305979-test.patch (obsolete) — Splinter Review
Fixing ESlint warning.

Honza
Attachment #8796577 - Attachment is obsolete: true
Attachment #8797185 - Flags: review+
One more update
Attachment #8797185 - Attachment is obsolete: true
Attachment #8797192 - Flags: review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/13100266a434
https://hg.mozilla.org/mozilla-central/rev/5515701bca40
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Duplicate of this bug: 925439
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.