1481450 - Should EnumerateDevices() need to create GetUserMediaWindowListener? (cleanup)

Status: NEW

(Core :: WebRTC: Audio/Video, enhancement, P2)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

There's some boilerplate in MediaManager::EnumerateDevices() that seems to create an GetUserMediaWindowListener and an inactive SourceListener. [1]

This mirrors what GetUserMedia() does (which also calls EnumerateDevicesImpl()), but solely to track the validity of the windowId (i.e. catch the user navigating away, so we can toss outstanding requests).

With bug 934425 we get a third caller of EnumerateDevicesImpl(), which turns this into a pattern:

As mentioned in bug 934425 comment 49:
> The problem here is that `mgr->IsWindowStillActive(aWindowId)` returns
> always `false` when we call it `ExistInSinks()` method. 
> 
> The `IsWindowStillActive()` is looking for a windows previously stored
> `mActiveWindows` member. In our case the current window does not exist
> there. A window id is stored in `GetUserMedia()` and `EnumerateDevices()`
> methods [1].
> 
> The `ExistInSinks()` method make use of the `EnumerateDevicesImpl()`
> directly so it does not add the window id in `mActiveWindows` . In addition
> to that we would need a `GetUserMediaWindowListener` pointer if we wanted to
> add a window in `ExistInSinks()`. So I need to create one but the window
> listener has no other use in our case. I see that `EnumerateDevices` creates
> a listener even if it does not need it. If I create a window listener in
> `ExistInSinks()` that change (and the one above) is not needed and works as
> is.
> 
> Please let me know what would be the best approach here.

We could clean this up. GetUserMediaWindowListener seems mostly tasked with keeping track of SourceListeners (which only GetUserMedia needs), and communicating with browser chrome for activity UX indicators.

I think the goal would be to only have GetUserMedia() create GetUserMediaWindowListener, and have a simpler IsWindowStillActive() concept for keeping track of outstanding requests that could be used by all three.

Also note that IsWindowStillActive() today affects nsIDocument::CanSavePresentation(). The comment there says:

  // Check if we have active GetUserMedia use

We need to decide whether we mean for this to cover outstanding EnumerateDevices() and FindSinkById() requests.

[1] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/media/MediaManager.cpp#3325-3326
[2] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/base/nsDocument.cpp#8034

Comment 1

[Karl Tomlinson (:karlt)]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #0)

Also note that IsWindowStillActive() today affects
nsIDocument::CanSavePresentation(). The comment there says:

// Check if we have active GetUserMedia use

We need to decide whether we mean for this to cover outstanding
EnumerateDevices() and FindSinkById() requests.

If we continue to abort EnumerateDevices() and FindSinkById() requests because the document goes into B/F cache, then that might be confusing if the document were to come out of B/F cache.

Comment 2

[Karl Tomlinson (:karlt)]

Am I correct in assuming the reason for aborting in EnumerateDevicesImpl() lambdas instead of waiting for the them to complete the sequence is that they use significant resources or may suffer significant delays?