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

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 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.

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)

(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

(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

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b8e0953ac97 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)

(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

Fixing ESlint warning. Honza

One more update

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

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