Create shell-remote for launching system-remote

RESOLVED FIXED in Firefox 44

Status

Firefox OS
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: lchang, Assigned: Tommy Kuo (away forever...))

Tracking

unspecified
FxOS-S8 (02Oct)

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 12 obsolete attachments)

34.98 KB, image/png
Details
9.51 KB, patch
Tommy Kuo (away forever...)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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

3 years ago
Created attachment 8620863 [details] [diff] [review]
bug-1156715-create-shell-remote.patch

Comment 2

3 years ago
Created attachment 8620882 [details] [diff] [review]
bug-1156715-create-shell-remote.patch
Attachment #8620863 - Attachment is obsolete: true

Comment 3

3 years ago
Created attachment 8620884 [details] [diff] [review]
[WIP] Handle 'get-display-list' event from system app

This is a hack for bug 1163914.

Comment 4

3 years ago
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.
Attachment #8620882 - Flags: review?(fabrice)

Comment 5

3 years ago
Created attachment 8621456 [details]
Perspective of DOM tree with another top-level window
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.
 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)?
forgot to set the ni flag, please see comment #6 and comment #7.
Flags: needinfo?(khuey)
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.
Attachment #8620882 - Flags: review?(fabrice)
I have confirmed with :alive, he says our system doesn't allow duplicate-launched on app:// protocols.
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 :)
Flags: needinfo?(fabrice)
(Assignee)

Comment 12

3 years ago
I will work on it.
Assignee: nobody → kuoe0
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!
Flags: needinfo?(fabrice)
cancel ni based on comment #11.
Flags: needinfo?(khuey)
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 :)
Attachment #8620882 - Flags: review?(fabrice)
Comment on attachment 8620884 [details] [diff] [review]
[WIP] Handle 'get-display-list' event from system app

This is a hack for bug 1163914.
feature-b2g: --- → 2.2r+
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.
feature-b2g: 2.2r+ → 2.2r?
We'll need alternative solution for 2.2r given that backporting Bug 1116089 requires much more platform dependencies.
feature-b2g: 2.2r? → ---
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())
Attachment #8620882 - Flags: review?(fabrice) → review-
(Assignee)

Comment 20

3 years ago
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.
Attachment #8620882 - Attachment is obsolete: true
Attachment #8620884 - Attachment is obsolete: true
Attachment #8642895 - Flags: review?(fabrice)
(Assignee)

Comment 21

3 years ago
(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.
(Assignee)

Comment 22

3 years ago
Created attachment 8643490 [details] [diff] [review]
Bug-1156715-hg.patch

I updated the patch. More detail in Comment #20.
Attachment #8642895 - Attachment is obsolete: true
Attachment #8642895 - Flags: review?(fabrice)
Attachment #8643490 - Flags: review?(fabrice)
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 *
Attachment #8643490 - Flags: review?(fabrice) → review-
No longer blocks: 1142883
(Assignee)

Comment 24

3 years ago
Created attachment 8658592 [details] [diff] [review]
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.
Attachment #8643490 - Attachment is obsolete: true
Attachment #8658592 - Flags: review?(fabrice)
(Assignee)

Comment 25

3 years ago
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.
Attachment #8658592 - Flags: review?(fabrice)
(Assignee)

Comment 26

3 years ago
Created attachment 8658632 [details] [diff] [review]
Bug-1156715-hg.patch

I already updated the patch. More info at comment #24.
Attachment #8658592 - Attachment is obsolete: true
Attachment #8658632 - Flags: review?(fabrice)
(Assignee)

Comment 27

3 years ago
Created attachment 8659085 [details] [diff] [review]
Bug-1156715-hg.patch

Add debug function.
Attachment #8658632 - Attachment is obsolete: true
Attachment #8658632 - Flags: review?(fabrice)
Attachment #8659085 - Flags: review?(fabrice)
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()
Attachment #8659085 - Flags: review?(fabrice) → review-
(Assignee)

Comment 29

3 years ago
(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?
Flags: needinfo?(fabrice)
(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.
Yep, Shih-Chiang is right, sorry for not having been more precise.
Flags: needinfo?(fabrice)
(Assignee)

Comment 32

3 years ago
Created attachment 8663494 [details] [diff] [review]
Bug-1156715-hg.patch

Hi Fabrice, I updated the patch. Please review it again. Thanks!
Attachment #8659085 - Attachment is obsolete: true
Attachment #8663494 - Flags: review?(fabrice)
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
Attachment #8663494 - Flags: review?(fabrice) → feedback+
(Assignee)

Comment 34

3 years ago
Created attachment 8664109 [details] [diff] [review]
Bug-1156715-hg.patch
Attachment #8663494 - Attachment is obsolete: true
Attachment #8664109 - Flags: review?(fabrice)
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.
Attachment #8664109 - Flags: review?(fabrice)
(Assignee)

Comment 36

3 years ago
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?
That, and I'm sure it works since we do the same for net_error.
(Assignee)

Comment 38

3 years ago
Created attachment 8664646 [details] [diff] [review]
Bug-1156715-hg.patch
Attachment #8664109 - Attachment is obsolete: true
Attachment #8664646 - Flags: review?(fabrice)
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().
Attachment #8664646 - Flags: review?(fabrice)
(Assignee)

Comment 40

3 years ago
Created attachment 8665253 [details] [diff] [review]
Bug-1156715-hg.patch

I refer to other jsm file and add the observe function now.
Attachment #8664646 - Attachment is obsolete: true
Attachment #8665253 - Flags: review?(fabrice)
Comment on attachment 8665253 [details] [diff] [review]
Bug-1156715-hg.patch

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

Thanks!
Attachment #8665253 - Flags: review?(fabrice) → review+
(Assignee)

Comment 42

3 years ago
Created attachment 8667675 [details] [diff] [review]
Bug-1156715-hg.patch

Add reviewer info in commit message. Carry r+ from comment #41.
Attachment #8665253 - Attachment is obsolete: true
Attachment #8667675 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e7ede395793
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S8 (02Oct)
You need to log in before you can comment on or make changes to this bug.