Closed
Bug 918695
Opened 11 years ago
Closed 11 years ago
[app manager] Clicking on debug when a toolbox is already open should only raise the toolbox
Categories
(DevTools Graveyard :: WebIDE, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 27
People
(Reporter: paul, Assigned: ochameau)
References
Details
Attachments
(2 files)
(deleted),
patch
|
paul
:
review+
jryans
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
paul
:
review+
jryans
:
feedback+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•11 years ago
|
Priority: -- → P2
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → poirot.alex
Assignee | ||
Comment 1•11 years ago
|
||
To show the toolbox we call gDevTools.showToolbox with a target object.
This function ensure raising the existing toolbox, if a toolbox for the same target
object has already been opened.
But the issue is that we were creating one new target object, each time we
called getTargetForApp...
getTargetForApp is calling TargetFactor.forRemote, that has a similar behavior...
but the object we are passing to it (`options`) is always different:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/apps/app-actor-front.js#225
Even if the actor on the device returns always the same actor,
we end up with a new distinct res.actor object.
Assignee | ||
Comment 2•11 years ago
|
||
That device patch is necessary in order to close the toolbox once the app is closed/killed.
It is also really important with the other patch attached to this bug...
It allows to clean up the target we keep in the `exitingTargets` map.
Without this, the keep getting a dead target once we killed (and relaunch) an app,
until we close the toolbox manually (it closes the target).
Assignee | ||
Updated•11 years ago
|
Attachment #819844 -
Flags: review?(paul)
Attachment #819844 -
Flags: feedback?(jryans)
Assignee | ||
Updated•11 years ago
|
Attachment #819846 -
Flags: review?(paul)
Attachment #819846 -
Flags: feedback?(jryans)
Comment on attachment 819844 [details] [diff] [review]
Only instanciate one singleton target per app
Review of attachment 819844 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me!
Attachment #819844 -
Flags: feedback?(jryans) → feedback+
Comment on attachment 819846 [details] [diff] [review]
Ensure dispatching tabDetached event for ContentAppActor when the app is closed/killed
Review of attachment 819846 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/devtools/server/actors/webapps.js
@@ +851,5 @@
> }
> + let actor = this._appActorsMap.get(mm);
> + if (actor) {
> + // The ContentAppActor within the child process doesn't necessary
> + // have to time to uninitialize itself when the app is closed/killed.
Is there any notification, like "you're about to be killed", that the ContentAppActor could watch for instead of duplicating this here?
Attachment #819846 -
Flags: feedback?(jryans) → feedback+
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] from comment #4)
> ::: toolkit/devtools/server/actors/webapps.js
> @@ +851,5 @@
> > }
> > + let actor = this._appActorsMap.get(mm);
> > + if (actor) {
> > + // The ContentAppActor within the child process doesn't necessary
> > + // have to time to uninitialize itself when the app is closed/killed.
>
> Is there any notification, like "you're about to be killed", that the
> ContentAppActor could watch for instead of duplicating this here?
May be. Actually the existing code doesn't even try to dispatch tabDetached. The BrowserTabActor listen for TabClose and we haven't tried to listen for another event.
But I'm afraid that the content process won't be able to do anything in case of OOM kill or crash. So at the end it looks like we have to ensure sending this event from the parent process.
Reporter | ||
Updated•11 years ago
|
Attachment #819844 -
Flags: review?(paul) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #819846 -
Flags: review?(paul) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 6•11 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/e8f2b30e478e
remote: https://hg.mozilla.org/integration/fx-team/rev/de99454fac7f
Keywords: checkin-needed
Comment 7•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e8f2b30e478e
https://hg.mozilla.org/mozilla-central/rev/de99454fac7f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Depends on: 975064
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•5 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•