Closed Bug 1078235 Opened 10 years ago Closed 10 years ago

New API: Fire panel "destroy" event

Categories

(DevTools :: Framework, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: Honza, Assigned: Honza)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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
Attached patch bug1078235-1.patch (obsolete) — Splinter Review
Attaching patch with suggested events.

Honza
Attachment #8500389 - Flags: review?(bgrinstead)
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)
(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 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)
(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
(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?
(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
Status: NEW → ASSIGNED
Attachment #8500554 - Flags: review+
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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.