1037015 - support tab casting to chromecast

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch, firefox code

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: WIP patch, chromecast code

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: chromecast_tabcast.patch

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: chromecast_tabcast.patch

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8454731 [details] [diff] [review]
chromecast_tabcast.patch

Just trying to understand parts of this patch

>diff --git a/mobile/android/base/ChromeCast.java b/mobile/android/base/ChromeCast.java
>         this.context = context;
>         this.route = route;
>+        this.mirror = route.supportsControlCategory(CastMediaControlIntent.categoryForCast("D40D28D6"));

This is basically saying that application "D40D28D6" is on the Chromecast? And that app is our endpoint for being able to tab mirror?

Also, "D40D28D6" should be a constant

>+    private String mSessionId;
>+    MirrorChannel mMirrorChannel;
>+    boolean mApplicationStarted = false;
>+
>+    class MirrorChannel implements MessageReceivedCallback {
>+
>+        /**
>+         * @return custom namespace
>+         */
>+        public String getNamespace() {
>+            return "urn:x-cast:org.mozilla.castin";

"castin" ? Typo? or should that be something else anyway since casting is the one verb you don't seem to like.

>+        public void onMessageReceived(CastDevice castDevice, String namespace,
>+                                      String message) {
>+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent(
>+                                                                           "MediaPlayer:Response", message));

I assume this is the JSON message (or something) that our endpoint app sends?
nit: The wrapping seems unneeded here. Just make long lines.

>+        }
>+
>+        public void sendMessage(String message) {
>+            if (apiClient != null && mMirrorChannel != null) {
>+                try {
>+                    Cast.CastApi.sendMessage(apiClient, mMirrorChannel.getNamespace(), message)
>+                        .setResultCallback(
>+                                           new ResultCallback<Status>() {
>+                                               @Override
>+                                                   public void onResult(Status result) {
>+                                               }
>+                                           });
>+                } catch (Exception e) {
>+                    Log.e(LOGTAG, "Exception while sending message", e);
>+                }
>+            }
>+        }
>+    }
>+    private class MirrorCallback implements ResultCallback<ApplicationConnectionResult> {
>+
>+        final EventCallback callback;
>+        MirrorCallback(final EventCallback callback) {
>+            this.callback = callback;
>+        }
>+
>+
>+        public void onResult(
>+                             ApplicationConnectionResult result) {
>+            Status status = result.getStatus();
>+            if (status.isSuccess()) {
>+                ApplicationMetadata applicationMetadata = result
>+                    .getApplicationMetadata();
>+                mSessionId = result
>+                    .getSessionId();
>+                String applicationStatus = result
>+                    .getApplicationStatus();
>+                boolean wasLaunched = result
>+                    .getWasLaunched();
>+                mApplicationStarted = true;

Line wrapping in here seems unneeded and/or just wrong

>diff --git a/mobile/android/base/MediaPlayerManager.java b/mobile/android/base/MediaPlayerManager.java
>                                                                         "MediaPlayer:Get",
>-                                                                        "MediaPlayer:End");
>+							                "MediaPlayer:End",
>+                                                                        "MediaPlayer:Mirror",
>+                                                                        "MediaPlayer:Message");

Indent

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js
>--- a/mobile/android/chrome/content/CastingApps.js
>+++ b/mobile/android/chrome/content/CastingApps.js
>@@ -58,11 +58,26 @@ var CastingApps = {
>     Services.obs.addObserver(this, "Casting:Play", false);
>     Services.obs.addObserver(this, "Casting:Pause", false);
>     Services.obs.addObserver(this, "Casting:Stop", false);
>+    Services.obs.addObserver(this, "Casting:Mirror", false);
> 
>     BrowserApp.deck.addEventListener("TabSelect", this, true);
>     BrowserApp.deck.addEventListener("pageshow", this, true);
>     BrowserApp.deck.addEventListener("playing", this, true);
>     BrowserApp.deck.addEventListener("ended", this, true);
>+    NativeWindow.menu.add("mirror tab", "drawable://casting", function() {
>+      function callbackFunc(aService) {
>+        let app = SimpleServiceDiscovery.findAppForService(aService);
>+        if (app)
>+          app.mirror(function() {
>+          });
>+      }
>+
>+      function testFunc(aService) {
>+        Cu.reportError("testing: " + aService);
>+        return aService.mirror == true;
>+      }
>+      this.prompt(callbackFunc, testFunc);
>+    }.bind(this));
>   },
> 
>   uninit: function ca_uninit() {
>@@ -74,6 +89,7 @@ var CastingApps = {
>     Services.obs.removeObserver(this, "Casting:Play");
>     Services.obs.removeObserver(this, "Casting:Pause");
>     Services.obs.removeObserver(this, "Casting:Stop");
>+    Services.obs.removeObserver(this, "Casting:Mirror");
> 
>     NativeWindow.contextmenus.remove(this._castMenuId);
>   },
>@@ -99,6 +115,12 @@ var CastingApps = {
>           this.closeExternal();
>         }
>         break;
>+      case "Casting:Mirror":
>+        {
>+	  Cu.import("chrome://browser/content/cast.js");

You are loading this like a JSM, not like a chrome script. A JSM has no window, but you seem to want one. CastingApps.js is loaded as a chrome script, so we could load this one like it too. In fact, just add a reference here:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#99

And whenever some code uses TabMirror (see below), the JS file will be autoloaded.

On the other hand, if you want to keep this as a JSM, let make it a JSM. It might be less work to keep it a JSM now that I think about it, but:
* Put it in mobile/android/modules
* Use .jsm
* import from resource://gre/modules/TabMirror.jsm

>diff --git a/mobile/android/chrome/content/cast.js b/mobile/android/chrome/content/cast.js

This file:
* Should be named "TabMirror.js" or "TabMirror.jsm" based on how we change it
* Has way too many "var" and extra blank lines and debugging output
* TabMirrorer -> TabMirror

>+var TabMirrorer = function(deviceId) {

>+  var window = Services.wm.getMostRecentWindow("navigator:browser");

If this is a JS chrome script, no need window. You just have the chrome window. If this is a JSM, just pass the window into the constructor from CastingApps

>+  var consoleService = Components.classes["@mozilla.org/consoleservice;1"]
>+    .getService(Components.interfaces.nsIConsoleService);

Services.console.logStringMessage - but don't use it

Use:
 let log = Cu.import("resource://gre/modules/AndroidLog.jsm", {}).AndroidLog.d.bind(null, "TabMirror");

Try to cleanup the overuse of var and the spacing a bit

>diff --git a/mobile/android/modules/MediaPlayerApp.jsm b/mobile/android/modules/MediaPlayerApp.jsm

> MediaPlayerApp.prototype = {
>+  mirror: function mirror(callback) {
>+    send("MediaPlayer:Mirror", {id: this.id}, (result) => {

nit: use spaces

{ id: this.id }

I'd probably put mirror(...) at the end of the object, not the beginning

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: chromecast_tabcast.patch

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: chromecast_tabcast.patch

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8460551 [details] [diff] [review]
chromecast_tabcast.patch

All the Java edits have a lot of indenting changes making it hard to see what's changed. Don't land this without seriously looking at the indenting issue.

Lot's of nits too.

>diff --git a/mobile/android/base/ChromeCast.java b/mobile/android/base/ChromeCast.java

>+    private boolean mirror;

I know I asked for "mirror" but after looking at the code a few times, I think "canMirror" is easier to understand.

>             obj.put("location", device.getIpAddress().toString());
>             obj.put("modelName", device.getModelName());
>+            obj.put("mirror", mirror);

I am tempted to use "canMirror" for the JSON object too, but I think we should think about how to better handle a "supports" style system here, so let's go with "mirror" in the JSON for now. We can change it later.

>diff --git a/mobile/android/base/MediaPlayerManager.java b/mobile/android/base/MediaPlayerManager.java

>-    private static final boolean SHOW_DEBUG = false;
>+    private static final boolean SHOW_DEBUG = true;

Should this be false to land?

>+            Log.e(LOGTAG, "Couldn't find a display for this id: " + message.getString("id") + " for message: " + event);
>+            if (callback != null)
>+                callback.sendError(null);

{} around the block

>+        } else if ("MediaPlayer:Message".equals(event)) {
>+            display.message(message.getString("data"), callback);

getString or optString?

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

>     BrowserApp.deck.addEventListener("ended", this, true);
>+    NativeWindow.menu.add(Strings.browser.GetStringFromName("casting.mirror_tab"), "drawable://casting", function() {

1. add a blank line before the NativeWindow line
2. casting.mirrorTab (see below)

We normally move main menus into Java, not building them via JS. I think this might be a temporary UI though, so we can do this for now.

>+      function testFunc(aService) {
>+        Cu.reportError("testing: " + aService);
>+        return aService.mirror == true;
>+      }
>+      this.prompt(callbackFunc, testFunc);

testFunc -> filterFunc

>+      case "Casting:Mirror":
>+        {
>+          Cu.import("resource://gre/modules/TabMirror.jsm");
>+          this.mirrorer = new TabMirror(aData, window);

You never use this variable so I assume it's just to keep the object from getting GC'ed, or maybe you need to add code later to "stop mirroring" :)

I'm not a fan of "mirrorer" so let's just rename to "this.tabMirror"

>diff --git a/mobile/android/locales/en-US/chrome/browser.properties b/mobile/android/locales/en-US/chrome/browser.properties

> casting.prompt=Cast to Device
>+casting.mirror_tab=Mirror Tab

casting.mirrorTab seems like our style

>diff --git a/mobile/android/modules/TabMirror.jsm b/mobile/android/modules/TabMirror.jsm

I don't understand most of the WebRTC stuff happening in this file. I can only assume this has been tested enough to feel good about it.

>+  let consoleService = Components.classes["@mozilla.org/consoleservice;1"]
>+    .getService(Components.interfaces.nsIConsoleService);

You don't use consoleService, so remove it.

There is a lot of logging in this file. Consider removing some of it now, or file a followup to do it later. It's too chatty for shipping.

>+  let create_offer_success = function(sdp) {
>+
>+    log("Successfully created offer " + JSON.stringify(sdp));

Remove the blank line

Careful with dumping too much info into logcat

>+  let create_answer_success = function(sdp) {
>+    log("Successfully created answer " + JSON.stringify(sdp));

Careful with dumping too much info into logcat

>+  let config = {
>+    iceServers: [{"url":"stun:stun.services.mozilla.com"}]

nit: Use spaces inside the {}

>+  if (screenWidth/screenHeight > maxWidth/maxHeight) {

>+    videoWidth = Math.ceil(videoHeight * maxWidth/maxHeight);

nit: add spaces around the / operator

>+  let constraints = {
>+    video: {
>+      mediaSource: "browser",
>+      browserArgs: {

I still don't like "browserArgs". It doesn't sound right for something we might want to be a public API

>+      advanced: [{width: {min: videoWidth, max: videoWidth},
>+                  height: {min: videoHeight, max: videoHeight}},
>+                 {aspectRatio: maxWidth/maxHeight}]

Use spaces inside the {}

>+  Cu.reportError("gUM constraints: " + JSON.stringify(constraints));

This doesn't look like an error, it looks like debugging. Remove it, or use your log function.

>+  getUserMedia( constraints, gUM_success, gUM_failure);

Remove the extra space after the parens

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: just_chromecast_no_ws.patch

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 8460551 [details] [diff] [review]
> chromecast_tabcast.patch
> 
> All the Java edits have a lot of indenting changes making it hard to see
> what's changed. Don't land this without seriously looking at the indenting
> issue.
I ran a script to clean up the WS in ChromeCast.java and there was a lot of clean up to be done. Here's a patch without the WS clean up. I can land this instead and leave the WS for the rest of the file as-is if you want

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 8460551 [details] [diff] [review]
> chromecast_tabcast.patch
> >+        } else if ("MediaPlayer:Message".equals(event)) {
> >+            display.message(message.getString("data"), callback);
> 
> getString or optString?
went with:
        } else if ("MediaPlayer:Message".equals(event) && message.has("data")) {
            display.message(message.getString("data"), callback);

> 
> >diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js
> 
> >     BrowserApp.deck.addEventListener("ended", this, true);
> >+    NativeWindow.menu.add(Strings.browser.GetStringFromName("casting.mirror_tab"), "drawable://casting", function() {
> 
> 1. add a blank line before the NativeWindow line
> 2. casting.mirrorTab (see below)
> 
> We normally move main menus into Java, not building them via JS. I think
> this might be a temporary UI though, so we can do this for now.

> 
> >+      case "Casting:Mirror":
> >+        {
> >+          Cu.import("resource://gre/modules/TabMirror.jsm");
> >+          this.mirrorer = new TabMirror(aData, window);
> 
> You never use this variable so I assume it's just to keep the object from
> getting GC'ed, or maybe you need to add code later to "stop mirroring" :)
I do. Follow up patch :-).

> I still don't like "browserArgs". It doesn't sound right for something we
> might want to be a public API
It's not intended to be public

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/b350df009721

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8463451 [details] [diff] [review]
just_chromecast_no_ws.patch

Thanks for the non-ws patch

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8453873 [details] [diff] [review]
WIP patch, chromecast code

moved this patch over to bug 1048335

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8460551 [details] [diff] [review]
chromecast_tabcast.patch

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

Now that we have the right build tools, we want to uplift Chromecast support and Tab mirroring on Chromecast devices.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8460551 [details] [diff] [review]
chromecast_tabcast.patch

Approval Request Comment
[Feature/regressing bug #]: New feature
[User impact if declined]: No mirroring tabs to chromecast devices
[Describe test coverage new/current, TBPL]: There isn't much :(
[Risks and why]: This is moderate risk stuff. We're adding/changing a lot of chromecast code. We're asking for uplift because of partner agreements with these types of tech and because we want to do PR around casting and mirroring in general for the 33 release.
[String/UUID change made/needed]: None.

Comment 16

[Wesley Johnston (:wesj)]

Warning if this

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 8460551 [details] [diff] [review]
chromecast_tabcast.patch

No going to uplift this.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Release noted as "Tab mirroring support for Chromecast"