Closed Bug 918695 Opened 6 years ago Closed 6 years ago

[app manager] Clicking on debug when a toolbox is already open should only raise the toolbox

Categories

(DevTools :: WebIDE, defect, P2)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: paul, Assigned: ochameau)

References

Details

Attachments

(2 files)

No description provided.
Priority: -- → P2
Assignee: nobody → poirot.alex
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.
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).
Attachment #819844 - Flags: review?(paul)
Attachment #819844 - Flags: feedback?(jryans)
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+
(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.
Attachment #819844 - Flags: review?(paul) → review+
Attachment #819846 - Flags: review?(paul) → review+
Blocks: 912891
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8f2b30e478e
https://hg.mozilla.org/mozilla-central/rev/de99454fac7f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.