913234 - Disconnecting leaves toolboxes open

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

If you disconnect from the device while an app toolbox is open, the toolbox is left open but completely useless and broken, since the connection has been closed already.

We should close any toolboxes we opened when the connection closes.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Part 1: Refactor projects to forward to device controller

Reduce repetition by having the device controller do the work.  Projects page will just tell it what to do.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Part 2: Close toolbox on disconnect

Close the toolboxes on device disconnect.

I started thinking about how to test this, but it's a bit tricky since you want to connect to an actual app.  Assuming these patches are okay, I'll file a bug to look at testing again once we have a simulator.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ab40f1e6a41a

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 801924 [details] [diff] [review]
Part 1: Refactor projects to forward to device controller

Review of attachment 801924 [details] [diff] [review]:
-----------------------------------------------------------------

I'm fine with this factorization, I like the fact that avoid calling listTabs multiple times.
But also note that we should be able to make actor related code more united by switching to protocol.js (bug 912476).
I don't think it is reasonable to switch to protocol.js just before the merge,
but I started writing sort of "front", in bug 911785. The code you factored should at some point move to it.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for the review!

I just realized that the refactor will conflict with your pending patch in bug 911785 to add install, since it needs the listTabs info too, and there is no "Install" in the devices panel, so it wouldn't make as much sense to continue the forwarding pattern.

Perhaps it makes sense for me to move the remote communication to some new file entirely, that both projects.js and device.js would talk to?  That file would centralize the work in one place, and has an additional benefit of separating this out of the controller for a specific UI panel, which is really the main goal of device.js (it tries to do too much right now).

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I could also skip the refactoring entirely for now, since we are moving quickly, and do that later.

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> I could also skip the refactoring entirely for now, since we are moving
> quickly, and do that later.

What about keeping the listTab but moving the rest to actor-front once it lands, would that work?

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Alexandre Poirot (:ochameau) from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] from comment #6)
> > I could also skip the refactoring entirely for now, since we are moving
> > quickly, and do that later.
> 
> What about keeping the listTab but moving the rest to actor-front once it
> lands, would that work?

I am not entirely sure I follow.  Do you mean that device.js and projects.js would both:

* Call listTabs
* Get this new actor-front
* Call things like start / stop on the front

If so, that could work too, but even then it may make sense to wrap listTabs and the usage of the front in a central place so that device.js and projects.js don't have to repeat those things.

Anyway, I'll link the protocol.js refactor to this for more ideas when that happens, and re-focus this bug to just fixing the open toolboxes.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Close toolboxes on disconnect

Carrying over Alex's r+ from attachment #801925 [details] [diff] [review].

Without the refactor from attachment #801924 [details] [diff] [review], there's two places to make the change now.

Comment 10

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/99f92e712d48

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99f92e712d48