Closed
Bug 1072208
Opened 10 years ago
Closed 10 years ago
New API: ToolSidebar events
Categories
(DevTools :: Framework, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: Honza, Assigned: Honza)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 5 obsolete files)
10.04 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
Brian, I am attaching a patch. What do you think? Honza
Assignee: nobody → odvarko
Attachment #8494420 -
Flags: review?(bgrinstead)
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
> 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•10 years ago
|
||
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?
Attachment #8494420 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 5•10 years ago
|
||
(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
Assignee | ||
Comment 6•10 years ago
|
||
Test included. Honza
Attachment #8494420 -
Attachment is obsolete: true
Attachment #8500443 -
Flags: review?(bgrinstead)
Comment 7•10 years ago
|
||
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); }
Attachment #8500443 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 8•10 years ago
|
||
> 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
Attachment #8500470 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 9•10 years ago
|
||
Yet fixing the code style (brackets). Honza
Attachment #8500443 -
Attachment is obsolete: true
Attachment #8500470 -
Attachment is obsolete: true
Attachment #8500470 -
Flags: review?(bgrinstead)
Attachment #8500471 -
Flags: review?(bgrinstead)
Comment 10•10 years ago
|
||
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
Attachment #8500471 -
Flags: review?(bgrinstead) → review+
Comment 11•10 years ago
|
||
(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•10 years ago
|
||
Pushed to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6439d7076d77. The patch to land will just need some comment changes
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•10 years ago
|
||
> 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
Attachment #8500471 -
Attachment is obsolete: true
Attachment #8501012 -
Flags: review?(bgrinstead)
Comment 14•10 years ago
|
||
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
Attachment #8501012 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 15•10 years ago
|
||
Includes a fix for the test failures. Honza
Attachment #8501012 -
Attachment is obsolete: true
Attachment #8501163 -
Flags: review?(bgrinstead)
Comment 16•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=446d0478409a
Updated•10 years ago
|
Attachment #8501163 -
Flags: review?(bgrinstead) → review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 17•10 years ago
|
||
We will need to document the information from Comment 7 at https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI#ToolPanel
Keywords: dev-doc-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4ab57c3f1745
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ab57c3f1745
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Assignee | ||
Comment 20•10 years ago
|
||
Doc updated at: https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI Honza
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•