Closed Bug 1080701 Opened 10 years ago Closed 10 years ago

TabMirror needs to be updated to work with the chromecast server.

Categories

(Firefox for Android Graveyard :: Screencasting, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox34+ fixed, firefox35+ fixed, firefox36 verified)

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 + fixed
firefox35 + fixed
firefox36 --- verified

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

Attachments

(2 files, 2 obsolete files)

The chromecast server was updated reduce start up time on the chromecast device. These changes are required to allow fennec to connect to the updated server.
Attached patch TabMirror update v1 (obsolete) — Splinter Review
Comment on attachment 8502616 [details] [diff] [review]
TabMirror update v1

Updated code to work with current version of chromecast server.
Attachment #8502616 - Flags: review?(wjohnston)
Attachment #8502616 - Flags: review?(mark.finkle)
Assignee: nobody → rbarker
Comment on attachment 8502616 [details] [diff] [review]
TabMirror update v1

LGTM
Attachment #8502616 - Flags: review?(mark.finkle) → review+
Comment on attachment 8502616 [details] [diff] [review]
TabMirror update v1

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

Seems pretty good. Some comments. If you wanted, I think you could make this a simpler to read with Task.jsm. i.e. something like:

_setLocalDescription: function (sdp) {
  return new Promise((resolve, reject) => { this.pc.setLocalDescription(sdp, resolve, reject); });
},

_createOffer: function() {
  return new Promise((resolve, reject) => { this.pc.createOffer(resolve, reject); });
},

_getUserMedia: function(contraints) {
  return new Promise((resolve, reject) => { this.getUserMedia(constraints, resolve, reject); });
},

_onGumSuccess: Task.async(function *(stream) {
  let stream = yield this._getUserMedia(constraints);
  this.pc.addStream(stream);

  let offer = yield this._createOffer();
  yield this.setLocalDescription(offer);
  this.sendMessage(offer);
}),

::: mobile/android/modules/TabMirror.jsm
@@ +18,5 @@
>    let getUserMedia = window.navigator.mozGetUserMedia.bind(window.navigator);
>  
> +  this.deviceId = deviceId;
> +  this.RTCSessionDescription = window.mozRTCSessionDescription;
> +  this.RTCIceCandidate = window.mozRTCIceCandidate;

These redefinitions are usually just in place for cross browser compat. Do you need them here?

@@ +25,4 @@
>  
>    let config = {
>      iceServers: [{ "url": "stun:stun.services.mozilla.com" }]
>    };

You could just pull this out as a const at the top of the file if you want.

@@ +33,1 @@
>      log("Failure creating Webrtc object");

You might want to throw here?

@@ +75,5 @@
> +
> +  _processMessage: function(data) {
> +    if (!data) {
> +      return;
> +    }

Space after

@@ +81,5 @@
> +    try {
> +      msg = JSON.parse(data);
> +    } catch(ex) {
> +      log("ex: " + ex);
> +    }

I'd throw out this try catch. If this fails, we should throw and exit, not continue.

@@ +83,5 @@
> +    } catch(ex) {
> +      log("ex: " + ex);
> +    }
> +
> +    if (msg) {

And I'd probably yank this check out as well, or at least reverse it: if (!msg) return;

@@ +98,5 @@
> +  },
> +
> +  // Signaling methods
> +  _processAnswer: function(sdp) {
> +    this.pc.setRemoteDescription(new this.RTCSessionDescription(sdp),

This is probably right, but it confuses me. We check for msg.sdp below and then pass msg here, but in this function, we've renamed msg to sdp. Is that right?

@@ +119,5 @@
> +    this.pc.setLocalDescription(sdp, () => this._setLocalSuccessOffer(sdp), failure);
> +  },
> +
> +  _onIceCandidate: function (candidate) {
> +    this._sendMessage(candidate.candidate);

candidate.candidate also makes me sad :( but it looks like its kinda of the spec.... You might rename this argument "msg" or something though.

@@ +136,1 @@
>      log("Could not get video stream");

Do you need to shut down the pc here?

@@ +155,2 @@
>        };
> +      Services.androidBridge.handleGeckoMessage(obj);

We should use Messaging.jsm in here.
Attachment #8502616 - Flags: review?(wjohnston) → review+
Attached patch TabMirror update v2 (obsolete) — Splinter Review
Attachment #8502616 - Attachment is obsolete: true
Carry forward r+ from :wesj and :mfinkle
Attachment #8506356 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67b723fc33f1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Patch needed to fox tab mirroring with chromecast. Depends on 1080012.
Comment on attachment 8509043 [details] [diff] [review]
TabMirror update for beta

Approval Request Comment
[Feature/regressing bug #]:Tab Mirror from Fennec to Chromecast
[User impact if declined]:Fennec is unable to start a tab mirror session and leaves the chromecast with a blank black screen.
[Describe test coverage new/current, TBPL]:I don't believe tab mirror has any test coverage.
[Risks and why]: none know
[String/UUID change made/needed]:none

depends on bug 1080012 being uplifted first.
Attachment #8509043 - Flags: approval-mozilla-beta?
Comment on attachment 8506417 [details] [diff] [review]
TabMirror update v2

Approval Request Comment
[Feature/regressing bug #]:Tab Mirror from Fennec to Chromecast
[User impact if declined]:Fennec is unable to start a tab mirror session and leaves the chromecast with a blank black screen.
[Describe test coverage new/current, TBPL]:I don't believe tab mirror has any test coverage.
[Risks and why]: none know
[String/UUID change made/needed]:none
Attachment #8506417 - Flags: approval-mozilla-aurora?
Tracy - Can you please verify this fix on Nightly before uplift?
Flags: needinfo?(twalker)
I'll take a look at this tomorrow (Tracy doesn't have a Chromecast device).
Flags: needinfo?(twalker) → needinfo?(aaron.train)
AaronMT - Ping. Do you have time to verify before Monday so that this can make beta4?
Tab mirroring works; albeit incredibly slow, probably another issue.
Status: RESOLVED → VERIFIED
Flags: needinfo?(aaron.train)
Comment on attachment 8509043 [details] [diff] [review]
TabMirror update for beta

Beta+
Attachment #8509043 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8506417 [details] [diff] [review]
TabMirror update v2

Aurora+
Attachment #8506417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.