Closed Bug 1169706 Opened 9 years ago Closed 9 years ago

Link-clicker UI broken for release builds (38 & earlier) - Join doesn't work (TypeError: rootNavigator.mediaDevices.enumerateDevices is not a function)

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
1

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Iteration:
41.2 - Jun 8
Tracking Status
firefox41 --- fixed

People

(Reporter: standard8, Assigned: standard8)

Details

(Keywords: regression)

Attachments

(1 file)

The loop-client link clicker UI is currently broken in FF 38 & earlier.

STR:

1) Get a Hello link
2) Open it in FF 38 or earlier
3) Click Join

Expected Results

=> Device permission prompt displayed

Actual Results

=> Nothing happens, error on console:

"[Dispatcher] Dispatching action caused an exception: " TypeError: rootNavigator.mediaDevices.enumerateDevices is not a function
Stack trace:
hasAudioOrVideoDevices@http://localhost:3000/content/shared/js/utils.js:318:7
ActiveRoomStore<.joinRoom@http://localhost:3000/content/shared/js/activeRoomStore.js:438:1
Dispatcher.prototype._dispatchNextAction/<@http://localhost:3000/content/shared/js/dispatcher.js:74:11
Dispatcher.prototype._dispatchNextAction@http://localhost:3000/content/shared/js/dispatcher.js:72:7
Dispatcher.prototype.dispatch@http://localhost:3000/content/shared/js/dispatcher.js:46:7
StandaloneRoomView<.joinRoom@http://localhost:3000/content/js/standaloneRoomViews.js:421:7
executeDispatch@http://localhost:3000/content/shared/libs/react-0.12.2.js:3282:21
[95]</SimpleEventPlugin.executeDispatch@http://localhost:3000/content/shared/libs/react-0.12.2.js:15905:23
forEachEventDispatch@http://localhost:3000/content/shared/libs/react-0.12.2.js:3270:5
executeDispatchesInOrder@http://localhost:3000/content/shared/libs/react-0.12.2.js:3291:3
[19]</executeDispatchesAndRelease@http://localhost:3000/content/shared/libs/react-0.12.2.js:2666:5
[126]</forEachAccumulated@http://localhost:3000/content/shared/libs/react-0.12.2.js:17883:5
[19]</EventPluginHub.processEventQueue@http://localhost:3000/content/shared/libs/react-0.12.2.js:2871:5
runEventQueueInBatch@http://localhost:3000/content/shared/libs/react-0.12.2.js:10391:3
[62]</ReactEventEmitterMixin.handleTopLevel@http://localhost:3000/content/shared/libs/react-0.12.2.js:10417:5
handleTopLevelImpl@http://localhost:3000/content/shared/libs/react-0.12.2.js:10503:1
[107]</Mixin.perform@http://localhost:3000/content/shared/libs/react-0.12.2.js:16876:13
[54]</ReactDefaultBatchingStrategy.batchedUpdates@http://localhost:3000/content/shared/libs/react-0.12.2.js:9142:7
batchedUpdates@http://localhost:3000/content/shared/libs/react-0.12.2.js:15103:3
[63]</ReactEventListener.dispatchEvent@http://localhost:3000/content/shared/libs/react-0.12.2.js:10598:7
Severity: normal → blocker
The issue is that navigator.mediaDevices doesn't have enumerateDevices in 38, nor does window.MediaStreamTrack have getSources.

Hence we fail :-(

This adds in checks and appropriate unit tests.
Attachment #8613221 - Flags: review?(mdeboer)
Attachment #8613221 - Flags: review?(dmose)
Comment on attachment 8613221 [details] [diff] [review]
Add checks to see if the functions exist in the objects or not.

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

If I understand correctly, in older versions, this reverts us to the older behavior of simply assuming that audio will work, and if it won't, that's the way it goes, because that's the best the old APIs will allow (without manually making a call to see if it fails)?

r=dmose (though I haven't hand tested).

::: browser/components/loop/content/shared/js/utils.js
@@ +311,5 @@
>     *
>     * @param  {Function} callback Called with a boolean which is true if there
>     *                             are audio devices present.
>     */
>    function hasAudioOrVideoDevices(callback) {

The name is impressively confusing for readers of call-site code.  Can you file a bug on that (I'm assuming it makes sense to delay the change given the time-pressure associated with this deploy).
Attachment #8613221 - Flags: review?(dmose) → review+
Attachment #8613221 - Flags: review?(mdeboer)
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #2)
> Comment on attachment 8613221 [details] [diff] [review]
> If I understand correctly, in older versions, this reverts us to the older
> behavior of simply assuming that audio will work, and if it won't, that's
> the way it goes, because that's the best the old APIs will allow (without
> manually making a call to see if it fails)?

Yes, we revert to the older behavior. We can't do anything more than that.


> ::: browser/components/loop/content/shared/js/utils.js
> >    function hasAudioOrVideoDevices(callback) {
> 
> The name is impressively confusing for readers of call-site code.  Can you
> file a bug on that (I'm assuming it makes sense to delay the change given
> the time-pressure associated with this deploy).

I can't see anything confusing here. The only confusing bit is that the call site callback parameter is hasAudio, and should have changed to hasDevices. I can change that before landing it as its just a rename.
(In reply to Mark Banner (:standard8) from comment #3)

> I can't see anything confusing here. The only confusing bit is that the call
> site callback parameter is hasAudio, and should have changed to hasDevices.
> I can change that before landing it as its just a rename.

Yeah, the problem is a combination of that, and the fact that the doxygen stuff about the function only talks about audio, not video.
https://hg.mozilla.org/mozilla-central/rev/8fbc2d79dfed
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: