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)
Tracking
(firefox34+ fixed, firefox35+ fixed, firefox36 verified)
VERIFIED
FIXED
Firefox 36
People
(Reporter: rbarker, Assigned: rbarker)
References
Details
Attachments
(2 files, 2 obsolete files)
8.97 KB,
patch
|
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.94 KB,
patch
|
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → rbarker
Comment 3•10 years ago
|
||
Comment on attachment 8502616 [details] [diff] [review] TabMirror update v1 LGTM
Attachment #8502616 -
Flags: review?(mark.finkle) → review+
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8502616 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Carry forward r+ from :wesj and :mfinkle
Attachment #8506356 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=237b0f09b55e
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/67b723fc33f1
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
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
Assignee | ||
Comment 10•10 years ago
|
||
Patch needed to fox tab mirroring with chromecast. Depends on 1080012.
Assignee | ||
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•10 years ago
|
||
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?
Comment 13•10 years ago
|
||
Tracy - Can you please verify this fix on Nightly before uplift?
Flags: needinfo?(twalker)
Comment 14•10 years ago
|
||
I'll take a look at this tomorrow (Tracy doesn't have a Chromecast device).
Flags: needinfo?(twalker) → needinfo?(aaron.train)
Comment 15•10 years ago
|
||
AaronMT - Ping. Do you have time to verify before Monday so that this can make beta4?
Comment 17•10 years ago
|
||
Tab mirroring works; albeit incredibly slow, probably another issue.
Comment 18•10 years ago
|
||
Comment on attachment 8509043 [details] [diff] [review] TabMirror update for beta Beta+
Attachment #8509043 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 19•10 years ago
|
||
Comment on attachment 8506417 [details] [diff] [review] TabMirror update v2 Aurora+
Attachment #8506417 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
status-firefox34:
--- → affected
status-firefox35:
--- → affected
tracking-firefox34:
--- → +
tracking-firefox35:
--- → +
Comment 20•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca31deef301 https://hg.mozilla.org/releases/mozilla-beta/rev/0811a9056ec4
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•