Closed
Bug 1078235
Opened 10 years ago
Closed 10 years ago
New API: Fire panel "destroy" event
Categories
(DevTools :: Framework, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 35
People
(Reporter: Honza, Assigned: Honza)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
7.62 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
Similarly to initialization events, the Toolbox should also fire panel "destroy" event just before a panel is destroyed. There is currently "destroyed" event fired by the toolbox when all panels finishes theirs destroy sequences, which is great, but might be too late in some cases (e.g. when removing listeners). Use case: 1) Append JSTerm listeners (webconsole.js) 2) Remove them when the Toolbox fires "destroyed" -> BUG the JSTerm object is already destroyed and no longer accessible (hud.jsterm set to null). Honza
Assignee | ||
Comment 1•10 years ago
|
||
Attaching patch with suggested events. Honza
Attachment #8500389 -
Flags: review?(bgrinstead)
Comment 2•10 years ago
|
||
Comment on attachment 8500389 [details] [diff] [review] bug1078235-1.patch Review of attachment 8500389 [details] [diff] [review]: ----------------------------------------------------------------- Please add a test for this in browser/devtools/framework - either by modifying browser_devtools_api.js or making a new simpler test just for this one set of events. Either way will work - looking at that test I'm thinking long term we will want to break this into a set of devtools_api-*.js tests to cover smaller and simpler sets of things.
Attachment #8500389 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #2) > Please add a test for this in browser/devtools/framework - either by > modifying browser_devtools_api.js or making a new simpler test just for this > one set of events. Either way will work Test included > looking at that test I'm thinking > long term we will want to break this into a set of devtools_api-*.js tests > to cover smaller and simpler sets of things. Yes, I like the idea. Honza
Assignee: nobody → odvarko
Attachment #8500389 -
Attachment is obsolete: true
Attachment #8500554 -
Flags: review?(bgrinstead)
Comment 4•10 years ago
|
||
Comment on attachment 8500554 [details] [diff] [review] bug1078235-2.patch Review of attachment 8500554 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/toolbox.js @@ +1498,5 @@ > if (this._destroyer) { > return this._destroyer; > } > > + this.emit("destroy"); This makes me think: do we really need a new toolbox 'destroy' event? We already have toolbox->'destroyed' / gDevTools->'toolbox-destroyed' - could we just rename or alias those to the new event name? Or get rid of that and only use this new one? If not, I'd like to understand why having two separate ones is important.
Attachment #8500554 -
Flags: review?(bgrinstead)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #4) > This makes me think: do we really need a new toolbox 'destroy' event? gDevTools listens for "destroy" from the toolbox and fires "toolbox-destroy". The duplication has been introduced already before by firing events on both gDevTools and the toolbox (and I am not big fan of that either). In any case, it's currently done for the life-time management: - toolbox life time (ready and destroy) - panel life time (init, build, ready, destroy) I would keep it (the 'destroy' event) done this way, so it's consistent with the way how life time is managed - and stop doing it for further events we'll introduce. For the future, I tend to think that basic toolbox life time management can be covered by events fired on gDevTools and other events can be fired by the toolbox (or by individual panels). // Extensions can watch life time of the toolbox, // get a reference to the toolbox and register/remove // further event listeners on the toolbox object itself. gDevTools.on("toolbox-ready", onToolboxReady); gDevTools.on("toolbox-destroy", onToolboxDestroy); Honza
Comment 6•10 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] from comment #5) > (In reply to Brian Grinstead [:bgrins] from comment #4) > > This makes me think: do we really need a new toolbox 'destroy' event? > > gDevTools listens for "destroy" from the toolbox and fires "toolbox-destroy". > > The duplication has been introduced already before by firing events on both > gDevTools and the toolbox (and I am not big fan of that either). > > In any case, it's currently done for the life-time management: > - toolbox life time (ready and destroy) > - panel life time (init, build, ready, destroy) > > I would keep it (the 'destroy' event) done this way, so it's consistent with > the way how life time is managed - and stop doing it for further events > we'll introduce. > > For the future, I tend to think that basic toolbox life time management can > be covered by events fired on gDevTools and other events can be fired by the > toolbox (or by individual panels). > > // Extensions can watch life time of the toolbox, > // get a reference to the toolbox and register/remove > // further event listeners on the toolbox object itself. > gDevTools.on("toolbox-ready", onToolboxReady); > gDevTools.on("toolbox-destroy", onToolboxDestroy); > > > Honza And what about the idea of using the existing toolbox-destroyed event and renaming it to toolbox-destroy? It seems to happen at the end of the the destroy function, which is maybe what we want?
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #6) > And what about the idea of using the existing toolbox-destroyed event and > renaming it to toolbox-destroy? It seems to happen at the end of the the > destroy function, which is maybe what we want? I think it make sense to have both events (they are fired at different times). * "toolbox-destroy": Fired at the beginning of the destroy process. This allows e.g. to remove all registered listeners (before corresponding objects are removed/destroyed). * "toolbox-destroyed": Fired when all (asynchronous) panel-destroy sequences (and inspector and perhaps additional future destroy sequences) finish. What I think we need a bit more is the "toolbox-destroy" (or whatever name we use for an event fired at the moment), but the other event is also useful (e.g. updating an UI that indicates that the toolbox is fully destroyed, such as Firebug start button). If we feel strong we can remove the "toolbox-destroyed" event, but it was already implemented that way (fired at the very end of the destroy process) and I think it's still natural place where an event is fired. Honza
Comment 8•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cc7dfd8c1814
Updated•10 years ago
|
Status: NEW → ASSIGNED
Updated•10 years ago
|
Attachment #8500554 -
Flags: review+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
This will need documentation at: https://developer.mozilla.org/en-US/docs/Tools/DevToolsAPI
Keywords: dev-doc-needed
https://hg.mozilla.org/integration/fx-team/rev/9fe8432ce02e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9fe8432ce02e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Assignee | ||
Comment 12•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
•