1156715 - Create shell-remote for launching system-remote

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Luke Chang [:lchang] (inactive)]

This bug is a subtask of bug 1142391. Our proposal is to create shell-remote.html/shell-remote.js for launching system-remote. The shell-remote should be launched whenever an external display is connected and terminated when it's disconnected.

Comment 1

[Shelly Lin [:shelly]]

Attachment: bug-1156715-create-shell-remote.patch

Comment 2

[Shelly Lin [:shelly]]

Attachment: bug-1156715-create-shell-remote.patch

Comment 3

[Shelly Lin [:shelly]]

Attachment: [WIP] Handle 'get-display-list' event from system app

This is a hack for bug 1163914.

Comment 4

[Shelly Lin [:shelly]]

Comment on attachment 8620882 [details] [diff] [review]
bug-1156715-create-shell-remote.patch

Hi Fabrice,

I'm not sure if you are the right person to review this bug, please feel free to forward to a better fit, Thanks a lot!

This patch is for the support of multi-screen on b2g (Bug 1116089).

Once shell.js observe a 'display-changed' event from gecko (Bug 1138287), it will add/remove this display into a list (displayList, an array of nsIDisplayInfo, contains a display ID and a boolean of connection status).

For current design[1], it will then open a new top-level window, with document of shell_remote.html. "shell remote" is just a container, which appends an iframe element of "system remote"(Bug 1156723). At this point, users will see the current selected wallpaper of primary device displayed at the connected external screen. The external screen acts like an "remote" desktop.

[1] The behaviour from detecting a connected display to actually "see" something on that display should have UX involved, which we don't have. So this is just an experimental design.

Comment 5

[Shelly Lin [:shelly]]

Attachment: Perspective of DOM tree with another top-level window

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8620882 [details] [diff] [review]
bug-1156715-create-shell-remote.patch

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

::: b2g/chrome/content/shell_remote.js
@@ +44,5 @@
> +    systemAppFrame.setAttribute('src', 'blank.html');
> +
> +    let container = document.getElementById('container');
> +    this.contentBrowser = container.appendChild(systemAppFrame);
> +    this.contentBrowser.src = homeURL + window.location.hash;

The reason we append the display Id to contentBrowser.src is that we cannot open the same URL twice (even in different top-level window) based on the experiment.

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

 I'm not sure if this is a constraint or not, @khuey do you know if any reason we cannot do this (see comment #6)?

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

forgot to set the ni flag, please see comment #6 and comment #7.

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 8620882 [details] [diff] [review]
bug-1156715-create-shell-remote.patch

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

Clearing the review flag until we know if we can do better than the hash hack.

Comment 10

[Shelly Lin [:shelly]]

I have confirmed with :alive, he says our system doesn't allow duplicate-launched on app:// protocols.

Comment 11

[Shelly Lin [:shelly]]

I've provided the wrong answer, please ignore my previous comments...

The purpose of letting system-remote kept its display ID, is to tell whether to launch the app URL received from broadcastChannel. The flow of launching an app to a specific screen is as follow:

-> tap on an app (with dev-option of multi-screen turns on in Settings)
-> system app requests a list of connected displays from shell
-> shell returns a display list
-> system app pops up a context menu with available displays
-> user choose a display
-> system app broadcasts the choice with broadcastChannel
-> each system-remote receives a message from broadcastChannel
-> ignore if the target display ID is different from its own, or launch the app if the display ID matches

Maybe we can encode/decode the display ID at gecko side if exposing display ID is a bad idea?

Fabrice, could you tell us again why exposing the display ID to system app is not desired? Thanks :)

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

I will work on it.

Comment 13

[[:fabrice] Fabrice Desré]

Ok that's clearer now. I think it's actually ok to expose the ID since it's not persisted anywhere and only accessed by the system app. Using broadcastchannel to exchange messages between screens is pretty cool!

Comment 14

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

cancel ni based on comment #11.

Comment 15

[Shelly Lin [:shelly]]

Comment on attachment 8620882 [details] [diff] [review]
bug-1156715-create-shell-remote.patch

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

Awesome!! Could you review the patch again? Thanks :)

Comment 16

[Shelly Lin [:shelly]]

Comment on attachment 8620884 [details] [diff] [review]
[WIP] Handle 'get-display-list' event from system app

This is a hack for bug 1163914.

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

We'll probably need this to be uplifted to 2.2R for RedSquare to enable flip phones.
There seems to be certain efforts needed, so Tommy will study and have assessment first.
I'm putting feature-b2g:2.2r? for now. 
If this can't be made to 2.2R on time, should consider alternative solution.

Comment 18

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

We'll need alternative solution for 2.2r given that backporting Bug 1116089 requires much more platform dependencies.

Comment 19

[[:fabrice] Fabrice Desré]

Comment on attachment 8620882 [details] [diff] [review]
bug-1156715-create-shell-remote.patch

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

::: b2g/chrome/content/shell.js
@@ +578,5 @@
>      this.sendEvent(target, type, payload);
>    },
>  
> +  // Multi-screen support on b2g. The following implementation will open a new
> +  // top-level window once we recieve a display connected event.

nit: s/recieve/receive

@@ +591,5 @@
> +    let flags = Services.prefs.getCharPref("toolkit.defaultChromeFeatures") +
> +                ',mozDisplayId=' + aDisplay.id;
> +    let shellUrl = './shell_remote.html#' + aDisplay.id;
> +    let win = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +              .getService(Components.interfaces.nsIWindowWatcher)

use Services.ww instead of manually getting the service.

@@ +620,5 @@
> +        return d.id != display.id;
> +      });
> +      this.closeTopLevelWindow(display);
> +    }
> +  },

Please move all this code into its own jsm. shell.js is big enough already ;)
Also, it's not clear to me that you need displayList at all. You just add/remove display objects in it, but openTopLevelWindow and topLevelWindows only uses the id.

::: b2g/chrome/content/shell_remote.html
@@ +2,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> +
> +<html xmlns="http://www.w3.org/1999/xhtml "

nit: extra space after xhtml

::: b2g/chrome/content/shell_remote.js
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function debug(str) {
> +  dump(' -*- ShellRemote.js: ' + str + '\n');

nit: double quotes for strings.

@@ +7,5 @@
> +function debug(str) {
> +  dump(' -*- ShellRemote.js: ' + str + '\n');
> +}
> +
> +var shellRemote = {

nit: let remoteShell = {...

@@ +9,5 @@
> +}
> +
> +var shellRemote = {
> +
> +  get homeURL() {

rename to indexURL

@@ +15,5 @@
> +  },
> +
> +  get manifestURL() {
> +    return "app://system.gaiamobile.org/manifest.webapp";
> +  },

You can't hardcode these values. Look at what the prefs we use in shell.js.

@@ +53,5 @@
> +
> +};
> +
> +window.onload = function() {
> +  if (shellRemote.hasStarted() == false) {

if (!remoteShell.hasStarted())

Comment 20

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

I update the patch. And I remove the `displayList` to maintain display device in shell.js. We will integrate display list into device manager of Presentation API in next step.

If you want to try the multi-screen feature, you should also apply the patch in Bug 1163914. That patch will maintain a display list in shell.js. And you can query the display list from Gaia.

Comment 21

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to Tommy Kuo [:KuoE0] from comment #20)
> Created attachment 8642895 [details] [diff] [review]
> Bug-1156715-hg.patch
> 
> I update the patch. And I remove the `displayList` to maintain display
> device in shell.js. We will integrate display list into device manager of
> Presentation API in next step.
> 
> If you want to try the multi-screen feature, you should also apply the patch
> in Bug 1163914. That patch will maintain a display list in shell.js. And you
> can query the display list from Gaia.

Please ignore the latest patch in comment20. I found I forgot to update some variable that its name is changed. I'll create a new patch tomorrow. Sorry about that.

Comment 22

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

I updated the patch. More detail in Comment #20.

Comment 23

[[:fabrice] Fabrice Desré]

Comment on attachment 8643490 [details] [diff] [review]
Bug-1156715-hg.patch

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

Please make sure you address all my previous comments. thanks!

::: b2g/chrome/content/shell.js
@@ +52,5 @@
>                                     'nsIStyleSheetService');
>  
> +XPCOMUtils.defineLazyServiceGetter(Services, 'ww',
> +                                   '@mozilla.org/embedcomp/window-watcher;1',
> +                                   'nsIWindowWatcher');

No need for that, just use Services.ww when you need it.

@@ +647,5 @@
> +      this.openTopLevelWindow(display);
> +    } else {
> +      this.closeTopLevelWindow(display);
> +    }
> +  },

I asked in comment #19 to move that in its own file. I still want that ;)

::: b2g/chrome/content/shell_remote.html
@@ +4,5 @@
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> +
> +<html xmlns="http://www.w3.org/1999/xhtml"
> +      id="shellRemote"
> +      windowtype="navigator:browser"

so, I'm a bit worried about using navigator:browser here. We have many places that use Services.wm.getMostRecentWindow("navigator:browser") and expect to find a valid shell object in that window.

Please rename it to navigator:remote-browser instead.

::: b2g/chrome/content/shell_remote.js
@@ +2,5 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

add "use strict";

@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const {utils: Cu} = Components;
> +
> +Cu.import('resource://gre/modules/Services.jsm');

please use double quotes for strings everywhere.

@@ +14,5 @@
> +
> +let remoteShell = {
> +
> +  get indexURL() {
> +    return "app://system.gaiamobile.org/index_remote.html";

no hardcoding please...

@@ +26,5 @@
> +  hasStarted: function remoteShell_hasStarted() {
> +    return this._started;
> +  },
> +
> +  start: function remoteShell_start() {

no need to name functions anymore. So just do start: function() { ... } here and in other functions.

::: b2g/chrome/jar.mn
@@ +13,5 @@
>  * content/shell.html                    (content/shell.html)
>  * content/shell.js                      (content/shell.js)
>    content/shell.css                     (content/shell.css)
> +* content/shell_remote.html             (content/shell_remote.html)
> +* content/shell_remote.js               (content/shell_remote.js)

You don't use the preprocessor in these files, so no need for the *

Comment 24

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

Hi Fabrice, I update the patch with your comment. Because I found that multi-screen would make screen-mirroring not work. So, I let multi-screen enable/disable by settings app.

Comment 25

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8658592 [details] [diff] [review]
Bug-1156715-hg.patch

Sorry, I forgot to update some single quote to double quote and shell_remote.js.

Comment 26

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

I already updated the patch. More info at comment #24.

Comment 27

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

Add debug function.

Comment 28

[[:fabrice] Fabrice Desré]

Comment on attachment 8659085 [details] [diff] [review]
Bug-1156715-hg.patch

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

We're getting close!

::: b2g/app/b2g.js
@@ +1152,5 @@
>  pref("dom.performance.enable_notify_performance_timing", true);
> +
> +// Multi-screen
> +pref("b2g.multiscreen.chrome_remote_url", "chrome://b2g/content/shell_remote.html");
> +pref("b2g.multiscreen.system_remote_url", "app://system.gaiamobile.org/index_remote.html");

The url of the system app should not be hardcoded. b2g.multiscreen.system_remote_url should only be the "index_remote.html" part

::: b2g/chrome/content/shell.js
@@ +754,5 @@
>  }, 'memory-pressure', false);
>  
> +Services.obs.addObserver(MultiscreenHandler.handleDisplayChangeEvent.bind(MultiscreenHandler),
> +                         "display-changed",
> +                         false);

move that into MultiscreenHandler.jsm

::: b2g/chrome/content/shell_remote.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +'use strict';

nit: don't mix single quotes and double quotes in the same file. Use double quotes everywhere.

@@ +10,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function debug(str) {
> +  dump(" -*- ShellRemote.js: " + str + '\n');

comment before landing.

@@ +19,5 @@
> +  get homeURL() {
> +    return Services.prefs.getCharPref("b2g.multiscreen.system_remote_url");
> +  },
> +
> +  get manifestURL() {

please rename to systemAppManifestURL

::: b2g/components/MultiscreenHandler.jsm
@@ +11,5 @@
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function debug(str) {
> +  dump("MultiscreenHandler: " + str + "\n");

nit: aStr, and comment the dump() before landing.

@@ +20,5 @@
> +// Multi-screen support on b2g. The following implementation will open a new
> +// top-level window once we receive a display connected event.
> +let MultiscreenHandler = {
> +
> +  topLevelWindows: {},

use a Map() instead

@@ +22,5 @@
> +let MultiscreenHandler = {
> +
> +  topLevelWindows: {},
> +
> +  init: function() {

Move the addObserver() from shell.js to this function.

You also need to remove this observer when xpcom-shutdown happens.

@@ +34,5 @@
> +    }
> +
> +    let flags = Services.prefs.getCharPref("toolkit.defaultChromeFeatures") +
> +                ",mozDisplayId=" + aDisplay.id;
> +    let remoteShellURL = Services.prefs.getCharPref("b2g.multiscreen.chrome_remote_url") + 

nit: trailing whitespace

@@ +70,5 @@
> +  },
> +
> +};
> +
> +this.MultiscreenHandler = MultiscreenHandler;

call MultiscreenHandler.init()

Comment 29

[Chih-Hsuan Kuo [:tommykuo]]

(In reply to [:fabrice] Fabrice Desré from comment #28)

> > +// Multi-screen
> > +pref("b2g.multiscreen.chrome_remote_url", "chrome://b2g/content/shell_remote.html");
> > +pref("b2g.multiscreen.system_remote_url", "app://system.gaiamobile.org/index_remote.html");
> The url of the system app should not be hardcoded. b2g.multiscreen.system_remote_url should only be the "index_remote.html" part

It can not work when I only leave the "index_remote.html" part. Is there something I should do for that?

Comment 30

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

(In reply to Tommy Kuo [:KuoE0] from comment #29)
> (In reply to [:fabrice] Fabrice Desré from comment #28)
> 
> > > +// Multi-screen
> > > +pref("b2g.multiscreen.chrome_remote_url", "chrome://b2g/content/shell_remote.html");
> > > +pref("b2g.multiscreen.system_remote_url", "app://system.gaiamobile.org/index_remote.html");
> > The url of the system app should not be hardcoded. b2g.multiscreen.system_remote_url should only be the "index_remote.html" part
> 
> It can not work when I only leave the "index_remote.html" part. Is there
> something I should do for that?

You can combined it with another gecko pref "b2g.system_startup_url". I guess this is what @fabrice means.

Comment 31

[[:fabrice] Fabrice Desré]

Yep, Shih-Chiang is right, sorry for not having been more precise.

Comment 32

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

Hi Fabrice, I updated the patch. Please review it again. Thanks!

Comment 33

[[:fabrice] Fabrice Desré]

Comment on attachment 8663494 [details] [diff] [review]
Bug-1156715-hg.patch

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

How are we gonna test that?

::: b2g/chrome/content/shell_remote.js
@@ +18,5 @@
> +
> +  get homeURL() {
> +    let systemAppURL = Services.prefs.getCharPref("b2g.system_startup_url");
> +    let shellRemoteURL = Services.prefs.getCharPref("b2g.multiscreen.system_remote_url");
> +    return systemAppURL.replace("index.html", shellRemoteURL);

no, you can't use string substitution to resolve relative urls... Look at what we do for net_error: http://mxr.mozilla.org/mozilla-central/source/b2g/components/B2GAboutRedirector.js#14

Comment 34

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

Comment 35

[[:fabrice] Fabrice Desré]

Comment on attachment 8664109 [details] [diff] [review]
Bug-1156715-hg.patch

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

::: b2g/chrome/content/shell_remote.js
@@ +16,5 @@
> +
> +let remoteShell = {
> +
> +  get homeURL() {
> +    let systemAppURL = Services.prefs.getCharPref("b2g.system_startup_url");

Why are you not using |let systemManifestURL = Services.prefs.getCharPref("b2g.system_manifest_url");| like I indicated (see http://mxr.mozilla.org/mozilla-central/source/b2g/components/B2GAboutRedirector.js#14) ?
You even have a getter for that.

Comment 36

[Chih-Hsuan Kuo [:tommykuo]]

Hi Fabrice, I think you only hope me use Service.io.newURI to handle the URL. And I think we want to get a "*.html" file, so we get the URL of the same type and change its path. Is there some reason to use "b2g.system_manifest_url" better than "b2g.system_startup_url"? Is it because we have the getter already, we don't need to query the prefs again?

Comment 37

[[:fabrice] Fabrice Desré]

That, and I'm sure it works since we do the same for net_error.

Comment 38

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

Comment 39

[[:fabrice] Fabrice Desré]

Comment on attachment 8664646 [details] [diff] [review]
Bug-1156715-hg.patch

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

Sorry I didn't catch that earlier. That should be the last change!

::: b2g/components/MultiscreenHandler.jsm
@@ +26,5 @@
> +  init: function() {
> +    Services.obs.addObserver(MultiscreenHandler.handleDisplayChangeEvent.bind(MultiscreenHandler),
> +                             "display-changed",
> +                             false);
> +    Services.obs.addObserver(this, "xpcom-shutdown", false);

You didn't implement the observe() method to get this notification. Please add that and remove both observers in this method. Note that bind() returns a new function when called, so you'll have either to save the value you pass to addObserver(..., "display-changed", false), or just do:
addObserver(this, "display-changed", this) and call handleDisplayChangeEvent() from Observe().

Comment 40

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

I refer to other jsm file and add the observe function now.

Comment 41

[[:fabrice] Fabrice Desré]

Comment on attachment 8665253 [details] [diff] [review]
Bug-1156715-hg.patch

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

Thanks!

Comment 42

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug-1156715-hg.patch

Add reviewer info in commit message. Carry r+ from comment #41.

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/6e7ede395793

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e7ede395793