1116860 - Get size of second screen from the chromecast

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

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

Attachment: get_size_from_chromecast.patch

Comment 1

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

Attachment: chromecast_send_size.patch

Comment 3

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

Comment on attachment 8543071 [details] [diff] [review]
get_size_from_chromecast.patch

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

>   Services.obs.addObserver((aSubject, aTopic, aData) => this._processMessage(aData), "MediaPlayer:Response", false);
>-
>+  this._sendMessage({start: true});

add spaces:
{ start: true }

> TabMirror.prototype = {

>+  _screenSize: {width: 1280, height: 720},

add spaces here too

>+  _start: function() {

>+    let windowId = this._window.BrowserApp.selectedBrowser.contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils).outerWindowID;
>+    let viewport = this._window.BrowserApp.selectedTab.getViewport();

if _window is null, bad things will happen. I don't suppose we have any alternative though.

>+    log("viewport: " + JSON.stringify(viewport));

Remove?

>   _processMessage: function(data) {

>+    if (msg.sdp && msg.type === "answer") {
>+      this._processAnswer(msg);
>+    } else if (msg.candidate) {
>+      this._processIceCandidate(msg);
>+    } else if (msg.height || msg.width) {

It's kinda nice that msg.type exists for "answer". Using the presence of | width | or | height | might lead to future bugs. How hard would it be to add | msg.type = "size" |?

Looks OK. Think about the msg.type thing.

Asking Randall to take a look too.

Comment 4

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

Comment on attachment 8543072 [details] [diff] [review]
chromecast_send_size.patch

>diff --git a/player.js b/player.js

>+      this._sendMessage({width: window.innerWidth, height: window.innerHeight});

add spaces around the curlies

{ width ... }

Remember to add | type: "size", | if you decide to use it.

Comment 5

[Randall Barker [:rbarker]]

> Asking Randall to take a look too.

Look okay to me.

Comment 6

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

https://hg.mozilla.org/integration/fx-team/rev/a88e871c1dfc

Comment 7

[Carsten Book [:Tomcat]]

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