1072208 - New API: ToolSidebar events

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

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

ToolSidebar widget should fire more events that can be utilized by extensions as well as the devtools platform itself.

New events:
* 'show': fired when the side-bar is displayed (fired on the sidebar object)
* 'hide': fired when the side-bar is hidden (fired on the sidebar object)
* 'sidebar-created': fired when the side-bar is created (fired on the parent panel object)
* 'sidebar-destroyed': fired when the side-bar is destroyed (fired on the parent panel object)

Extensions can handle visibility/state changes of the side bar and update custom related UI (e.g. a side bar toggle button or side bar splitter visibility, etc.)

Honza

Comment 1

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

Attachment: bug1072208-1.patch

Brian, I am attaching a patch. What do you think?

Honza

Comment 2

[Girish Sharma [:Optimizer]]

One issue with this request that I see is that except for Inspector, no other tool is actually using this API to create the sidebar (to my knowledge)

Most of the tools are just manually managing the creation/showing/hiding of the sidebar.

Comment 3

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

> One issue with this request that I see is that except for Inspector, no other tool is
> actually using this API to create the sidebar (to my knowledge)
Inspector, WebConsole and Scratchpad 

> Most of the tools are just manually managing the creation/showing/hiding of the sidebar.
All tools should step-by-step adopt the ToolSidebar widget.
(should be done in separate reports I think)

Honza

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8494420 [details] [diff] [review]
bug1072208-1.patch

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

I'm OK with migrating existing panels to use this API in the future - let's file a follow up bug for that.  Please add a test for these events in devtools/framework/test.  Also, do you have any example usage of these events that I can see?

Comment 5

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

(In reply to Brian Grinstead [:bgrins] from comment #4)
> I'm OK with migrating existing panels to use this API in the future - let's
> file a follow up bug for that.
bug 1074710

> Please add a test for these events in
> devtools/framework/test.
ok

> Also, do you have any example usage of these events that I can see?
See this Firebug report:
https://github.com/firebug/firebug.next/issues/88

This particular commit should be useful:
https://github.com/firebug/firebug.next/commit/83221efa6f52da7632ffc23151f38a8f6b0ddfd7

This is the related branch:
https://github.com/firebug/firebug.next/compare/issue88

Honza

Comment 6

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

Attachment: bug1072208-2.patch

Test included.

Honza

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8500443 [details] [diff] [review]
bug1072208-2.patch

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

As discussed on IRC, let's require that EventEmitter functionality is present on a tool panel instance by adding it in toolbox.js if it isn't already present (right after  this._toolPanels.set(id, panel)).  We will need to add documentation for the panel API to cover the following things:

1) Don't use the names on/off/emit/once for functions on your panel object - we are going to decorate EventEmitter functionality with: http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/event-emitter.js.
2) The EventEmitter functionality will not be available in the constructor of your panel - so if you need to fire events at this time, you can do so with:

    const EventEmitter = require("devtools/toolkit/event-emitter");
    function MyPanelInstance() {
      EventEmitter.decorate(this);
    }

Comment 8

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

Attachment: bug1072208-3.patch

> As discussed on IRC, let's require that EventEmitter functionality is
> present on a tool panel instance by adding it in toolbox.js if it isn't
> already present (right after  this._toolPanels.set(id, panel)).  We will
> need to add documentation for the panel API to cover the following things:
Done

Note that it's done after:
this._toolPanels.set(id, panel)

...in case build return value (from the definition structure) is a promise, as well as after:
let built = definition.build(iframe.contentWindow, this);

...in case the return value is the panel object itself.

Where the docs is supposed to be updated?

Honza

Comment 9

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

Attachment: bug1072208-4.patch

Yet fixing the code style (brackets).

Honza

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8500471 [details] [diff] [review]
bug1072208-4.patch

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

::: browser/devtools/framework/test/browser_toolbox_sidebar_events.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Please add a comment to the top of this file explaining what is being tested

::: browser/devtools/framework/toolbox.js
@@ +918,5 @@
>        // Wait till the panel is fully ready and fire 'ready' events.
>        promise.resolve(built).then((panel) => {
>          this._toolPanels.set(id, panel);
>  
> +        if (typeof panel.emit == "undefined") {

This is the second attempt to decorate for panels using the new return value for `build`.  So I'm assuming that this is here for backwards compat with the old style promise return values.  That's fine if this is the case, but we should definitely have a comment explaining it

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> Where the docs is supposed to be updated?

I'm guessing here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI#ToolPanel

Comment 12

[Brian Grinstead [:bgrins]]

Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6439d7076d77.  The patch to land will just need some comment changes

Comment 13

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

Attachment: bug1072208-5.patch

> This is the second attempt to decorate for panels using the new return value for `build`.
> So I'm assuming that this is here for backwards compat with the old style promise return
> values. That's fine if this is the case, but we should definitely have a comment explaining it
True, done.

> I'm guessing here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI#ToolPanel
I see, I'll update the docs as soon as the patch lends.

Thanks Brian for the reviews!
Honza

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8501012 [details] [diff] [review]
bug1072208-5.patch

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

Two scratchpad tests are failing [0] due to the calls to toolPanel.emit in the sidebar.  It is passing an instance of ScratchpadSidebar as the toolPanel - it's probably easiest to just decorate that object in scratchpad.js, around line 2174.

browser/devtools/scratchpad/test/browser_scratchpad_inspect.js
browser/devtools/scratchpad/test/browser_scratchpad_inspect_primitives.js

*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full message: TypeError: this._toolPanel.emit is not a function
Full stack: ToolSidebar@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/framework/sidebar.js:53:3
ScratchpadSidebar@chrome://browser/content/devtools/scratchpad.js:2174:19
Scratchpad.sidebar@chrome://browser/content/devtools/scratchpad.js:429:23
SP_inspect/<@chrome://browser/content/devtools/scratchpad.js:549:9
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:865:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:744:7
*************************

[0]: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6439d7076d77

Comment 15

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

Attachment: bug1072208-6.patch

Includes a fix for the test failures.

Honza

Comment 16

[Brian Grinstead [:bgrins]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=446d0478409a

Comment 17

[Brian Grinstead [:bgrins]]

We will need to document the information from Comment 7 at https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI#ToolPanel

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/4ab57c3f1745

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ab57c3f1745

Comment 20

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

Doc updated at:
https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI

Honza