Closed Bug 1037015 Opened 5 years ago Closed 5 years ago

support tab casting to chromecast

Categories

(Firefox for Android :: Screencasting, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
relnote-firefox --- 34+
fennec 33+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Whiteboard: [dont-auto-uplift])

Attachments

(3 files, 4 obsolete files)

No description provided.
Attached patch WIP patch, firefox code (obsolete) — Splinter Review
tracking-fennec: ? → 33+
Attachment #8453873 - Flags: feedback?(mark.finkle)
Attachment #8453871 - Flags: feedback?(mark.finkle)
Depends on: 1006186
Depends on: 1037389
Depends on: 1037488
Depends on: 1037644
Attached patch chromecast_tabcast.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8453871 - Attachment is obsolete: true
Attachment #8453871 - Flags: feedback?(mark.finkle)
Attachment #8454711 - Flags: review?(mark.finkle)
Attached patch chromecast_tabcast.patch (obsolete) — Splinter Review
Attachment #8454711 - Attachment is obsolete: true
Attachment #8454711 - Flags: review?(mark.finkle)
Attachment #8454731 - Flags: review?(mark.finkle)
Attachment #8454731 - Flags: review?(wjohnston)
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
Attachment #8454731 - Flags: review?(wjohnston)
Attachment #8454731 - Flags: review?(mark.finkle)
Attachment #8454731 - Flags: feedback+
Attached patch chromecast_tabcast.patch (obsolete) — Splinter Review
Attachment #8454731 - Attachment is obsolete: true
Attachment #8460549 - Flags: review?(mark.finkle)
Attachment #8460549 - Attachment is obsolete: true
Attachment #8460549 - Flags: review?(mark.finkle)
Attachment #8460551 - Flags: review?(mark.finkle)
Depends on: 1041700
Depends on: 1041493
No longer depends on: 1037389
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
Attachment #8460551 - Flags: review?(mark.finkle) → review+
(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
Attachment #8463451 - Flags: feedback?(mark.finkle)
(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
https://hg.mozilla.org/mozilla-central/rev/b350df009721
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment on attachment 8463451 [details] [diff] [review]
just_chromecast_no_ws.patch

Thanks for the non-ws patch
Attachment #8463451 - Flags: feedback?(mark.finkle) → feedback+
Depends on: 1046423
Depends on: 1046971
Comment on attachment 8453873 [details] [diff] [review]
WIP patch, chromecast code

moved this patch over to bug 1048335
Attachment #8453873 - Flags: feedback?(mark.finkle)
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.
Attachment #8460551 - Flags: approval-mozilla-aurora?
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.
Warning if this
Whiteboard: [dont-auto-uplift]
Attachment #8460551 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1055562
Comment on attachment 8460551 [details] [diff] [review]
chromecast_tabcast.patch

No going to uplift this.
Attachment #8460551 - Flags: approval-mozilla-aurora+
Release noted as "Tab mirroring support for Chromecast"
You need to log in before you can comment on or make changes to this bug.