875104 - Permanent connect page

Status: NEW

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

I started a discussion over this thread:
  https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.developer-tools/uJShKpU2y48
  http://blog.techno-barje.fr/mockups/connect-page-mockup.png
about how to start integrating features of the simulator, and more broadly, apps development ecosystem. Tuning the device connection workflow would be a very good first step. In this sense, we can easily improve the usability of remote tabs management by opening the remote toolbox in the same Firefox window, exactly like a regular tab toolbox. It will also mean keeping the remote connection open in order to connect once and be able to debug multiple tabs.

This is a very good step forward introducing a similar kind of devtool UI, similar to the Tabs list, but instead would display the App list.

This bug matches second item "2) Tabs selection" in the mailing list thread.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Permanent connect page.

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 752982 [details] [diff] [review]
Permanent connect page.

This patch tweaks the connect page to:
* open the remote toolbox in BOTTOM HostTarget (instead of WINDOW)
* do not close the connect page when opening a toolbox
* disable regular devtools when being on the connect page

An obvious followup would be to work on the browser actor to be able to listen for tab opening/close and be able to update tab list dynamically.

Dave, I haven't been able to get much feedback from devtools folks about my plan, nevertheless, I'd like to start moving forward with such independent improvements.
Could you confirm me that such feature makes sense?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Just wanted to note that I like the plan you laid out on the mailing list and my lack of feedback in this case should be interpreted as agreement :)

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Permanent connect page.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 758644 [details] [diff] [review]
Permanent connect page.

See comment 2 for the patch description.

I made a more generic way of preventing toolboxes to appear for browser UI documents. For now, we need to prevent displaying them on the connect page, but in near future we would also like to prevent it for Apps manager.

Also note that the `keepTargetAlive` toolbox feature I've added here is very handy and we are planning to use it in the simulator in order to reuse the same connection for all simulator features.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Permanent connect page.

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 759072 [details] [diff] [review]
Permanent connect page.

While integrating the toolbox.keepsTargetAlive into the simulator I realized that we actually want target.destroy to be called as it unregister various listeners.
What we do want is to ultimately avoid the connection to be shutdown.
So I move this flag to target and introduced a target.keepClientAlive, that prevent from calling client.close in target's destroy method.

Comment 8

[Paul Rouget [:paul]]

Comment on attachment 759072 [details] [diff] [review]
Permanent connect page.

We're still not sure we want to implement that right now.
Please extract the keepalive part into a new bug.

----------------------

>-function openToolbox(form, chrome=false) {
>+function openToolbox(form, link, chrome=false) {
>+  if (gCurrentToolbox) {
>+    gCurrentToolbox.destroy();
>+    gCurrentToolbox = null;
>+  }

In theory, we should be able to switch to a new target dynamically (something like "toolbox.target = ...").
But I guess we never really supported that.

>+  link.setAttribute("current-target", "true");

Instead of passing the link through, can't you just do:
link = document.querySelector("a:active") ?

>+    // We have to set tab as BottomHost expect a tab attribute on target whereas
>+    // TabTarget ignores any tab being given as options attributes passed to forRemoteTab.
>+    let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
>+    Object.defineProperty(target, "tab", {value: browserWindow.gBrowser.selectedTab});

That sounds a bit hacky. Maybe we should add an option to `showToolbox` instead?

>+const URL_BLACKLIST = [CONNECT_PAGE];

Maybe you want to use a `Set` here instead of an Array.
const URL_BLACKLIST = Set([CONNECT_PAGE]);

>+    // Prevent opening toolbox on some browser UI pages,
>+    // unless we set the chrome pref.
>+    if (target.url) {
>+      let blacklisted = URL_BLACKLIST.some(
>+        function (url) target.url.indexOf(url) == 0
>+      );

With a `Set`: URL_BLACKLIST.has(url);

>+      if (blacklisted && !Services.prefs.getBoolPref("devtools.chrome.enabled")) {
>+        let msg = "Can't open devtools on this browser page.";
>+        deferred.reject(msg);
>+        Cu.reportError(msg);
>+        return deferred.promise;
>+      }
>+    }