If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Need visible indicator while a site has camera/microphone access while Firefox window is open

VERIFIED FIXED in Firefox 20

Status

()

Firefox
Device Permissions
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 1 bug)

unspecified
Firefox 20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia][blocking-gum+])

Attachments

(4 attachments, 10 obsolete attachments)

728.70 KB, image/png
Details
11.86 KB, patch
derf
: review+
Details | Diff | Splinter Review
23.62 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.90 KB, patch
Boriss
: ui-review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #729522 +++

Original mockup: attachment 666072 [details]. Tabs aren't a great place for this, as there's not enough space in pinned tabs and normal tabs can be scrolled off-screen.
Blocking only to have something simple, doesn't need to be fancy.
Whiteboard: [getUserMedia] → [getUserMedia] [blocking-gum+]

Updated

5 years ago
Summary: Need always-visible indicator while a site has camera/microphone access → Need visible indicator while a site has camera/microphone access while Firefox window is open
Maire can add details here as needed.
On 10/18, the gUM team met with the privacy team to discuss what is needed to enable getUserMedia by default (come out from behind the pref).  We agreed that only three things were needed: Bug 804611, Bug 805631 and Bug 805632.  (Those three bugs block gUM.)  This bug should probably be closed as a dup of Bug 805631, so I'm doing that now.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 805631
(Assignee)

Comment 4

5 years ago
Bug 805631 doesn't handle tabs that are scrolled off-screen. The idea I had was adding a transient button to the toolbar that would select the tab with camera access.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to Dão Gottwald [:dao] from comment #4)
> Bug 805631 doesn't handle tabs that are scrolled off-screen. The idea I had
> was adding a transient button to the toolbar that would select the tab with
> camera access.

Ah, ok.  Thanks for the clarification.  Changing to blocking-gum- since this bug fix is not needed to enable the feature by default.
Whiteboard: [getUserMedia] [blocking-gum+] → [getUserMedia] [blocking-gum-]
(Assignee)

Comment 6

5 years ago
I think this bug is needed and bug 805631 is not.
(In reply to Dão Gottwald [:dao] from comment #6)
> I think this bug is needed and bug 805631 is not.

To clarify - this bug is tracking the general privacy need that a user needs to know when a site has camera/microphone access when FF open. bug 805631 is UI polish to Firefox itself based on the UX design to give more clarity when a tab is actively using the camera/microphone. This bug blocks because "a solution" is needed for privacy reasons, but bug 805631 does not block because it's a polished way of doing it (i.e. UX polish). Correct?
(Assignee)

Comment 8

5 years ago
(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Dão Gottwald [:dao] from comment #6)
> > I think this bug is needed and bug 805631 is not.
> 
> To clarify - this bug is tracking the general privacy need that a user needs
> to know when a site has camera/microphone access when FF open.

Yes.

> bug 805631 is
> UI polish to Firefox itself based on the UX design to give more clarity when
> a tab is actively using the camera/microphone. This bug blocks because "a
> solution" is needed for privacy reasons, but bug 805631 does not block
> because it's a polished way of doing it (i.e. UX polish). Correct?

"Polished" is debatable. I think it's an incomplete attempt to address the privacy need and may be WONTFIX.
Updating whiteboard to indicate blocking per discussion with Dao.
Whiteboard: [getUserMedia] [blocking-gum-] → [getUserMedia] [blocking-gum+]
Created attachment 685977 [details]
Mockup: Visual indicator while a site has camera/microphone access

Dao raises a good point that the mockup shown in attachment 666072 [details] does not give an indication of which tab is streaming video once a user is no longer looking at the streaming tab.  A particular concern is that the active tab could be lost amongst other tabs.

To address this, let's apply the styling shown in Bug 805631 even to non-active tabs.  This way, even if the user switches tabs, the steaming tab will be immediately apparent.


While this should largely address the "lost tab" case for most users, two edge cases have been brought up here which are not address: pinned tabs and no-tab-visible.

It's true that if a tab has been pinned, not enough favicon space will exist for both a favicon and a steaming icon.  I believe we can address this separately by styling the favicon of a pinned tab differently to reflect streaming.  After all, we already style the pinned tab different to all others on notification (with a blue glue), so there’s predent that a pinned tab user will likely be familiar with.

A potential issue in this design is that with the favicon restyled rather than showing a video icon, the user could not click on the video menu on the pinned tab if it weren’t active, but would instead click the favicon to switch to that tab and then click the video icon in the URL bar.  It's a drawback, but I think it’s outweighed by the pros of this design.  After all, a user who has pinned a tab both has indicated some level of trust in that site, and that particular tab is easier to find than all others: it is always located to the left of the tab strip.  Thus, the problem of finding the streaming tab to click it and then shut it off is somewhat mitigated.

The case of not seeing any tabs at all (for instance in fullscreen mode or another application) or not seeing a video tab (by having too many tabs) is I think a case that we do not need to address: especially in a first version.  While it may be useful to some and worth addressing later, it provides many drawbacks that I feel are outweighed by the positive.  And, given how much this proposed design already reflects current streaming more than virtually any application that users will be familiar with, frankly I feel that having additional indicators is redundant to the point of being overkill.  We’re already letting users fully navigate away from their streaming video yet still giving notification that it’s running: to do a Firefox-wide or even operating-system-wide notification would mean that we’re indicating video on the active tab URL bar (that users are less likely to leave once they’re streaming) and in the tab strip.  We even have two ways for them to shut the camera off, even ignoring closing the whole window or quitting Firefox.  And, though we should not rely on it, we should keep in mind that most cameras have a visual indication of streaming already.
Created attachment 689505 [details]
Mockup: Global notifications of streaming webcam/microphone on OSX and Windows 7

Attached are system-level notifications that will allow users to quickly jump back to the tab they are streaming data from.  These are consistent with other OS-level notifications and would be visible unless users are in fullscreen mode.
Created attachment 689512 [details]
Icons: OSX toolbar, OSX toolbar clicked, Windows toolbar camera icons
Comment on attachment 689505 [details]
Mockup: Global notifications of streaming webcam/microphone on OSX and Windows 7

Ok; this is the "globally visible notification" for the system tray/etc.  We don't have code to inject icons into there on any OS, so doing this is not possible currently.  Filed a new bug and copied these mockups there (bug 819343).


The request was for a UI button/dropdown that's visible in FF chrome when a site has the mic/camera active, per the conversation at http://irclog.gr/#show/irc.mozilla.org/media/96334.  If there was any confusion, I'm sorry, I thought it was clear.  Can we get any sort of mockup of one within FF chrome so Dao can get going?  Rough is fine
When Bug 819343 can be implemented, we'll have a complete solution here.  In the meantime(In reply to Randell Jesup [:jesup] from comment #13)
> Ok; this is the "globally visible notification" for the system tray/etc.  We
> don't have code to inject icons into there on any OS, so doing this is not
> possible currently.  

When Bug 819343 can be implemented, we'll have a complete solution here.  In the meantime, let's implement a way for a user to jump to a tab sharing video data if they have scrolled away from that tab in the tab strip.  This means that on a Firefox window where video is shared, the user would always in one click be able to jump back to that tab.
Created attachment 690688 [details]
Mockup: Visible indicator when sharing tab scrolled off of the tab bar
Catching up here, but it sounds like the summary of use cases / requirements is:

1) Display an indication when cam/mic is in use, as a privacy guard against users forgetting they are on-air.

2) Allow users to find which tab (tabs?) is currently using the cam/mic, so they can remember, resume, or cancel the interaction. (Largely to avoid the obvious frustration if the browser says the cam/mic is in use, but gives no clue as to what's using it.)

3) When the cam/mic is in use, the browser should provide a way for users to revoke access (bug 805632).


Is there a link to the security/privacy review? (Just want to make sure I understand the general concerns that are driving the specific UI pieces.)


A few challenges / questions to the above:

* A number of complications would go away if only the foreground tab/window got AV data (else it gets silence + black frame). I suspect this would be an onerous restriction, but maybe it's a useful v1.0 starting point?

* What about WebRTC consumers that are not web pages living in a tab? EG addons, SocialAPI providers. It would be desirable to have common UI for all. (Addons could just be responsible for their own UI, but presumably things like Facebook's SocialAPI integration (which has chat) will want to offer WebRTC eventually.)

* How will background-tab glowyness work in Australis (where only the foreground tab has a distinct shape)?

* WRT #3 (revoking access / 805632), what drives that requirement? Assuming the user is aware of which tab is using the cam/mic; seems like either the page itself offers the functionality (so we don't need to), or the user can just close the tab. [A _separate_ question, but obviously entangled, is how the user revokes a "never ask again" permission.]

And a couple random thoughts:

* A floating persistent window (panel) -- perhaps always-on-top and visible on all desktops -- might be easier-to-implement and more cross-platform mechanism than bug 819343. (But that also feels like it might be really annoying. But^2 maybe it's easily closeable, and we've done our due diligence at protecting the user.)

* ...I'm thinking of how apps like Skype deal with this issue, via a floating active-call window. But Vidyo doesn't do that. Nor Facetime (although, interestingly, it will video-mute ("pause") when a user switches to another desktop, but not audio-mute). So I do think there's precedent for, at least in some cases/contexts, assuming the user is aware of what's going on even if there isn't an obvious indicator. Geolocation (in the browser) is also an example of this -- privacy sensitive, but not globally indicated.

* An alternate way to guard against forgotten background WebRTC tabs might be to have an have some kind of popup/notification to remind the user if that tab hasn't been used in X minutes.
Status: REOPENED → NEW
(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #15)
> Created attachment 690688 [details]
> Mockup: Visible indicator when sharing tab scrolled off of the tab bar

I'm pretty reluctant to do this one (although it's an interesting idea), because our current tabstrip code is already really complicated. And even if we did, it would take a significant amount of time to get right, and I'd worry about it being double-cost to implement for Australis too. 

I do also wonder if the tab-glow, in general, is a good metaphor for indicating AV-state. There's nothing else quite like it (except for the apptab highlight for title changes, which is unrelated). Nor is it extendable to other privacy-related permissions (at least, I think we can agree that it would be crazy to use a green glow for WebRTC, a purple glow for geolocation, an orange glow for $mumble, etc). OTOH, audio/video does seem like a bit of a special case, so I don't think it's outright wrong either. Hmm.
(In reply to Justin Dolske [:Dolske] from comment #16)
>
> Is there a link to the security/privacy review? (Just want to make sure I
> understand the general concerns that are driving the specific UI pieces.)

The security/privacy bug is Bug 795024.
(In reply to Justin Dolske [:Dolske] from comment #16)
> 1) Display an indication when cam/mic is in use, as a privacy guard against
> users forgetting they are on-air.
> 
> 2) Allow users to find which tab (tabs?) is currently using the cam/mic, so
> they can remember, resume, or cancel the interaction. (Largely to avoid the
> obvious frustration if the browser says the cam/mic is in use, but gives no
> clue as to what's using it.)

Also http://www.quickmeme.com/meme/5p2h/
I have ~950 tabs in 22 windows on one machine currently.  This is important.

> 3) When the cam/mic is in use, the browser should provide a way for users to
> revoke access (bug 805632).


> * A number of complications would go away if only the foreground tab/window
> got AV data (else it gets silence + black frame). I suspect this would be an
> onerous restriction, but maybe it's a useful v1.0 starting point?

That would be a so-bad-we-couldn't-show-our-faces problem.  Ok, slight exaggeration, but people look up stuff while chatting all the time (also to review a document together, etherpads, etc, etc).  Seriously, I think we'd be better to stay off than to do that.  It is something considered and rejected by the WGs as a requirement, though I don't think the opposite behavior is required (i.e. you don't *have* to keep the camera active).

Chrome and Opera allow multiple apps in different tabs to capture the same camera at the same time.  We've been strongly considering doing the same (pretty much decided to at this point).

Cases with more than one device in use will be rare for a while (though not totally non-existent).  A real example might be a tab capturing audio continuously in the background to provide voice browser commands, and a foreground tab used for a video chat.

> * What about WebRTC consumers that are not web pages living in a tab? EG
> addons, SocialAPI providers. It would be desirable to have common UI for
> all. (Addons could just be responsible for their own UI, but presumably
> things like Facebook's SocialAPI integration (which has chat) will want to
> offer WebRTC eventually.)

Yes, that's an issue not addressed here (though an addon is responsible for itself).  Any dropdown/whatever listing all users of gUM could reasonably include entries for non-tabs - selecting would focus/expose that window I would presume, and/or inform the app the user wants it exposed.  The user would need to be able to revoke access for non-tabs in some manner, but this ability would only be exposed through a extension (which can do anything) or a chrome feature like Social API (which is trusted content and could add it's own shutdown).

> * How will background-tab glowyness work in Australis (where only the
> foreground tab has a distinct shape)?

Good question.

> * WRT #3 (revoking access / 805632), what drives that requirement? Assuming
> the user is aware of which tab is using the cam/mic; seems like either the
> page itself offers the functionality (so we don't need to), or the user can
> just close the tab. [A _separate_ question, but obviously entangled, is how
> the user revokes a "never ask again" permission.]

Well, you can't trust the site to allow revocation or to actually do it, especially for devices without "access" lights (mics especially).  Close/navigate does cover that for tabs, and we can trust chrome uses like Social API and extensions (which we have to trust) to provide their own revocation (or to link into whatever we provide).

> And a couple random thoughts:
> 
> * A floating persistent window (panel) -- perhaps always-on-top and visible
> on all desktops -- might be easier-to-implement and more cross-platform
> mechanism than bug 819343. (But that also feels like it might be really
> annoying. But^2 maybe it's easily closeable, and we've done our due
> diligence at protecting the user.)

We've discussed something like that (especially for fullscreen modes, doubly so on mobile devices - or reserving one edge of the screen, or a invokable bar that's normally hidden or hideable by the user).  Worth considering for v2+ and mobile.

> * ...I'm thinking of how apps like Skype deal with this issue, via a
> floating active-call window. But Vidyo doesn't do that. Nor Facetime
> (although, interestingly, it will video-mute ("pause") when a user switches
> to another desktop, but not audio-mute). 

I suspect it doesn't mute if you focus another app and hide the window, though.  Just a guess; I haven't used it really.

> So I do think there's precedent
> for, at least in some cases/contexts, assuming the user is aware of what's
> going on even if there isn't an obvious indicator. Geolocation (in the
> browser) is also an example of this -- privacy sensitive, but not globally
> indicated.

Agreed; and this limits the privacy concerns somewhat.  As does not having persistent permissions - any tab with access you gave access to, in this session.  But we can't trust the apps to revoke access on their own in all cases, or to make it clear they still have access or are capturing.

> * An alternate way to guard against forgotten background WebRTC tabs might
> be to have an have some kind of popup/notification to remind the user if
> that tab hasn't been used in X minutes.

Helps with some cases, not with some other malicious apps.  (Example: apps asks for access to create a user image for a site, but never gives up the MediaStream and continues to capture and stream the audio and/or video while you play their html5 game, or navigate around not noticing you're in an iframe, etc.)  Worth considering for v2+.
(In reply to Justin Dolske [:Dolske] from comment #16)
> * How will background-tab glowyness work in Australis (where only the
> foreground tab has a distinct shape)?

Just wanted to note that this was addressed in bug 805631 in this mockup: https://bug805631.bugzilla.mozilla.org/attachment.cgi?id=685978
Created attachment 692403 [details]
Mockup: Temporary media sharing toolbar dropdown icon for Firefox 19

We met today to talk about the way forward on the visible indicators of WebRTC/GUM sharing for the next release of Firefox.  While the filesystem-level icons in attachment 689505 [details] could provide a long-term solution to the global notification issue (with some caveats - may not appear consistently on Windows/OSX, Gnome3 needs another solution), it's not doable technically in time for Firefox 19.  Without global notifications, glowing tabs and especially solutions for revealing sharing tabs (such as in attachment 690688 [details]) are also too difficult to implement within the Firefox 19 timeframe.

So, as a temporary solution so that WebRTC can land pref'd-on in Firefox 19, a Firefox dropdown-toolbar icon will appear while a user has enabled webcam or microphone sharing.  This toolbar item will display the URLs which the user is sharing video and allow the user to jump to those tabs.

The permission icon, as other notifications, will still display in the URL bar of the tab where webcam access is being shared.  We agreed that it would be a nice-to-have, but not a blocker, for users to be able to click that icon and see an arrow-panel notification allowing them to cease or modify that site's access to their camera or mic.

While it's not in scope for the next few versions of Firefox, in this meeting we also discussed the need to develop an integrated, larger solution for users to manage the permissions they've given to sites.  This should handle both the management of permissions, the revocation and giving of permissions, and allow users to get a clear view of what permissions have been shared with which sites.  This would encompass WebRTC/GuM as well as other permissions such as geolocation and shared cookies.  Such a solution has been on the UX team's radar for at least a couple years, but actually getting a timeframe and dedicated effort is a great goal for 2013.
Created attachment 692404 [details]
Mockup: Temporary media sharing toolbar dropdown icon for Firefox 19
Attachment #692403 - Attachment is obsolete: true
Keywords: uiwanted
(Assignee)

Updated

5 years ago
Assignee: nobody → dao
Created attachment 693300 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams
Created attachment 693409 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams

WIP - adding Observer notifications to the previous patch for getusermedia-active/inactive with windowIDs.  We may not need this notification, and rely on the existing recording-device-events notifications (which don't carry the windowID)

Updated

5 years ago
Attachment #693300 - Attachment is obsolete: true
Created attachment 693671 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams

This should be what Dao needs for the backend side; need to figure out the right reviewer

Updated

5 years ago
Attachment #693409 - Attachment is obsolete: true

Updated

5 years ago
Attachment #693671 - Flags: review?(tterribe)
Comment on attachment 693671 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams

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

I think there's some missing bits here, so r- for now. There may be more: I need to check more closely, but I'll do that after you've tested these changes.

::: dom/media/MediaManager.cpp
@@ +683,5 @@
>    already_AddRefed<nsIGetUserMediaDevicesSuccessCallback> mSuccess;
>    already_AddRefed<nsIDOMGetUserMediaErrorCallback> mError;
>  };
>  
> +NS_IMPL_THREADSAFE_ISUPPORTS1(MediaManager, nsIObserver)

Shouldn't this also need to take nsIMediaManager?

::: dom/media/MediaManager.h
@@ +276,4 @@
>  {
>  public:
> +  static already_AddRefed<MediaManager> GetInstance();
> +

You also need an NS_DECL_NSIMEDIAMANAGERSERVICE below, right?
Attachment #693671 - Flags: review?(tterribe) → review-
Created attachment 694979 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams

Updated

5 years ago
Attachment #693671 - Attachment is obsolete: true
Comment on attachment 694979 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams

missing interface defs corrected
Attachment #694979 - Flags: review?(tterribe)
Comment on attachment 694979 [details] [diff] [review]
Backend support for list of documents that have active gUM MediaStreams

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

Okay, I think everything else looks fine. r=me
Attachment #694979 - Flags: review?(tterribe) → review+
For the back-end piece (added [leave-open]):

https://tbpl.mozilla.org/?tree=Try&rev=ab7aef3d8c88
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b76a1d0adf5
Whiteboard: [getUserMedia] [blocking-gum+] → [getUserMedia] [blocking-gum+] [leave-open]
Target Milestone: --- → Firefox 20
(Assignee)

Updated

5 years ago
Whiteboard: [getUserMedia] [blocking-gum+] [leave-open] → [getUserMedia][blocking-gum+][leave open]
https://hg.mozilla.org/mozilla-central/rev/7b76a1d0adf5
(Assignee)

Updated

5 years ago
Attachment #685977 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #689505 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #690688 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #689512 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 695541 [details] [diff] [review]
patch

Known issues:

1. I couldn't add the camera/microphone/camera+microphone labels seen in the mockup, as that information isn't available here, short of guessing it based on the permission UI. Could probably be handled in a followup bug with some back-end work.

2. Reloading a tab kills the stream and sends "recording-device-events", but nsIMediaManagerService.activeMediaCaptureWindows erroneously still contains that window at that point, so the indicator doesn't go away. Needs a bug filed.

3. Sensible icons are missing. I'll file a new bug or let bug 798336 cover this.

4. There's no popup notification in tabs with active streams. Probably doesn't make much sense as long as we don't have the option to pause or kill streams from the UI. At least it's not quite clear what should appear in the popup right now. Just some message like "This page has access to your camera or microphone"? Without any actions available? That's not quite the use case popup notifications were designed for.
Attachment #695541 - Flags: review?(gavin.sharp)
(In reply to Dão Gottwald [:dao] from comment #32)
> Created attachment 695541 [details] [diff] [review]
> patch
> 
> Known issues:
> 
> 1. I couldn't add the camera/microphone/camera+microphone labels seen in the
> mockup, as that information isn't available here, short of guessing it based
> on the permission UI. Could probably be handled in a followup bug with some
> back-end work.

Ok; I'll look at how it appears.  I think that's ok.

> 2. Reloading a tab kills the stream and sends "recording-device-events", but
> nsIMediaManagerService.activeMediaCaptureWindows erroneously still contains
> that window at that point, so the indicator doesn't go away. Needs a bug
> filed.

Bug 802538 is likely the cause.  Currently it appears we're leaking listeners, which are the basis of the activeMediaCaptureWindows() return.

> 3. Sensible icons are missing. I'll file a new bug or let bug 798336 cover
> this.

Ok.

> 
> 4. There's no popup notification in tabs with active streams. Probably
> doesn't make much sense as long as we don't have the option to pause or kill
> streams from the UI. At least it's not quite clear what should appear in the
> popup right now. Just some message like "This page has access to your camera
> or microphone"? Without any actions available? That's not quite the use case
> popup notifications were designed for.

That's where the option will be later (very soon I hope), so I think it makes sense to put in now without the option to kill access.  Comments?
Depends on: 802538
(Assignee)

Comment 34

5 years ago
Created attachment 695644 [details] [diff] [review]
patch v2

> > 4. There's no popup notification in tabs with active streams. Probably
> > doesn't make much sense as long as we don't have the option to pause or kill
> > streams from the UI. At least it's not quite clear what should appear in the
> > popup right now. Just some message like "This page has access to your camera
> > or microphone"? Without any actions available? That's not quite the use case
> > popup notifications were designed for.
> 
> That's where the option will be later (very soon I hope), so I think it
> makes sense to put in now without the option to kill access.  Comments?

Ok, I checked the popup notification API and it allows for the absence of any actions, so this is feasible.
Attachment #695541 - Attachment is obsolete: true
Attachment #695541 - Flags: review?(gavin.sharp)
Attachment #695644 - Flags: review?(gavin.sharp)
Attachment #695644 - Flags: review?(dolske)
Comment on attachment 695644 [details] [diff] [review]
patch v2

>diff --git a/browser/base/content/browser-webrtcUI.js b/browser/base/content/browser-webrtcUI.js

>+let WebrtcIndicator = {

>+  menuCommand: function (aMenuitem) {
>+    let streamData = this._menuitemData.get(aMenuitem);

This should probably handle streamData being null (e.g. if the tab closed itself and we GCed while the menu was open).

>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul

>+      <toolbarbutton id="webrtc-status-button"

>+                     orient="horizontal"

Why is this needed? I don't understand how this makes sense on a toolbarbutton.

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY webrtcIndicatorButton.label "Media Streams">
>+<!ENTITY webrtcIndicatorButton.tooltip "Currently active media streams">

This seems a bit jargony, but I'm not sure I have a better suggestion. Maybe Boriss does.

>diff --git a/browser/modules/webrtcUI.jsm b/browser/modules/webrtcUI.jsm

>+      chromeWin.PopupNotifications.show(aBrowser, "webRTC-sharingDevices", message,
>+                                        "webRTC-sharingDevices-notification-icon", mainAction,
>+                                        secondaryActions, options);

Hrm, this highlights an existing issue with the code - webRTC-notification-icon does not exist. With the patch, both webRTC-sharingDevices-notification-icon and webRTC-shareDevices-notification-icon do not exist. That parameter is supposed to be a reference to an <image> in the document to use as the anchor (see other examples in browser.xul's notification-popup-box). When the anchorID element doesn't exist PopupNotifications falls back to using the default anchor. So as far as I can tell this means that the existing webRTC-notification-icon styling is not used, and with this patch that extends to webRTC-sharingDevices-notification-icon/webRTC-shareDevices-notification-icon. We should probably have a single "webRTC-notification-icon" given that the same styling is currently used for both, and just add it to browser.xul's notification-popup-box.

>+function updateGlobalIndicator() {

>+    let browserWindow = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                                     .getInterface(Ci.nsIWebNavigation)
>+                                     .QueryInterface(Ci.nsIDocShellTreeItem)
>+                                     .rootTreeItem
>+                                     .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                     .getInterface(Ci.nsIDOMWindow)
>+                                     .wrappedJSObject;

Using chromeEventHandler is a bit shorter and avoids the need for the wrappedJSObject AFAIK:
let browserWindow = win.QueryInterface(Ci.nsIInterfaceRequestor)
                       .getInterface(Ci.nsIWebNavigation)
                       .QueryInterface(Ci.nsIDocShell)
                       .chromeEventHandler.ownerDocument.defaultView;

>+    let tab = browserWindow.gBrowser &&
>+              browserWindow.gBrowser._getTabForContentWindow(contentWindow.top);

Followup fodder, but we really should un-prefix this method (and maybe have it use .top automatically).

>+    if (tab) {
>+      webrtcUI.activeStreams.push({
>+        uri: contentWindow.location.href,
>+        tab: tab

Holding on to a tab is scary from a leak risk perspective (if recording-device-events isn't fired reliably we end up leaking entire windows). If we had reliable global tab IDs (bug 529477) we could use that to mitigate this risk.\

Do we need to cache this data in the front-end at all? For updating the button we only need to track the existence of media streams, and we can populate the menu on-demand using MediaManagerService.activeMediaCaptureWindows when the popup is opened, right?

>diff --git a/browser/themes/browserShared.inc b/browser/themes/browserShared.inc

>+%define primaryToolbarButtons

Hmm, unrelated to this patch, but I wonder whether the social toolbar button ("social-toolbar-item") should be in here. Probably not for the mac styles that use this #define, but I don't understand the Windows ones.

>diff --git a/browser/themes/pinstripe/browser.css b/browser/themes/pinstripe/browser.css

>+#webrtc-status-button /* temporary placeholder */,

Should also make sure to support retina when this is updated. Probably wouldn't hurt to put a bug # in these "temporary placeholder" comments.

We also need some tests for this.
Attachment #695644 - Flags: review?(gavin.sharp)
Attachment #695644 - Flags: review?(dolske)
Attachment #695644 - Flags: review-
(Assignee)

Updated

5 years ago
Depends on: 824825
(Assignee)

Comment 36

5 years ago
Created attachment 695847 [details] [diff] [review]
patch v3

> >+      <toolbarbutton id="webrtc-status-button"
> 
> >+                     orient="horizontal"
>
> Why is this needed? I don't understand how this makes sense on a toolbarbutton.

It's needed for the dropmarker due to type="menu".

> We should probably have a single "webRTC-notification-icon" given that the
> same styling is currently used for both, and just add it to browser.xul's
> notification-popup-box.

The mockup wants a separate icon for this (one that glows green).

> We also need some tests for this.

What kind of tests? I think the most interesting bits here are activeMediaCaptureWindows and the observer notifications, but those aren't under the UI's control.
Attachment #695644 - Attachment is obsolete: true
Attachment #695847 - Flags: review?(gavin.sharp)
(Assignee)

Comment 37

5 years ago
Boriss, I'm using "Currently active media streams" as the tooltip for button. Are you ok with that? Do you have an alternative suggestion?
Flags: needinfo?(jboriss)
Comment on attachment 695847 [details] [diff] [review]
patch v3

Should the button be customizable? If a user accidentally removes it they'll stop getting notifications, and that seems undesirable. It's also a bit strange (though not without precedent) to have a customizable button that isn't always visible.
Attachment #695847 - Flags: review?(gavin.sharp) → review+
(In reply to Dão Gottwald [:dao] from comment #36)
> What kind of tests?

Tests that ensure that the the UI appears and is populated correctly when WebRTC streams are active, and that it disappears when they aren't.
(Assignee)

Comment 40

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #38)
> Comment on attachment 695847 [details] [diff] [review]
> patch v3
> 
> Should the button be customizable? If a user accidentally removes it they'll
> stop getting notifications, and that seems undesirable. It's also a bit
> strange (though not without precedent) to have a customizable button that
> isn't always visible.

Right now the button isn't customizable. This was intentional from my side.
(Assignee)

Comment 41

5 years ago
landed without the label and tooltip for the toolbar button:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc359d500e94
(Assignee)

Comment 42

5 years ago
Created attachment 696253 [details] [diff] [review]
label and tooltip for the toolbar button
Attachment #696253 - Flags: ui-review?(jboriss)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #39)
> (In reply to Dão Gottwald [:dao] from comment #36)
> > What kind of tests?
> 
> Tests that ensure that the the UI appears and is populated correctly when
> WebRTC streams are active, and that it disappears when they aren't.

I don't think that's possible right now.

Our automation infrastructure currently does not support running mochitests with real cameras. The automation that has been written so far is using fake streams, but fake streams disregard the permission prompt.

You'll probably need to wait for bug 815002 to land to be able to get automation here.
(In reply to Jason Smith [:jsmith] from comment #43)
> Our automation infrastructure currently does not support running mochitests
> with real cameras. The automation that has been written so far is using fake
> streams, but fake streams disregard the permission prompt.

It's not necessary to have real cameras to automate UI tests - we can cause the relevant events to fire and mock MediaManagerService such that its activeMediaCaptureWindows is populated. Having "fake streams" that don't disregard the permission prompt is also a possibility.
(In reply to Dão Gottwald [:dao] from comment #40)
> Right now the button isn't customizable. This was intentional from my side.

Oh, I guess I got confused because you added it to the defaultset. Is there any point in doing that for a non-customizable toolbar item? social-toolbar-item isn't there.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #44)
> (In reply to Jason Smith [:jsmith] from comment #43)
> > Our automation infrastructure currently does not support running mochitests
> > with real cameras. The automation that has been written so far is using fake
> > streams, but fake streams disregard the permission prompt.
> 
> It's not necessary to have real cameras to automate UI tests - we can cause
> the relevant events to fire and mock MediaManagerService such that its
> activeMediaCaptureWindows is populated. Having "fake streams" that don't
> disregard the permission prompt is also a possibility.

True, that's a possibility. In the automation whimboo and I have been working though, we have been trying to push for approaches that exercise as much of the stack as possible to get deeper coverage. That's probably something that can happen after bug 815002 lands though.
(Assignee)

Comment 47

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #45)
> (In reply to Dão Gottwald [:dao] from comment #40)
> > Right now the button isn't customizable. This was intentional from my side.
> 
> Oh, I guess I got confused because you added it to the defaultset. Is there
> any point in doing that for a non-customizable toolbar item?
> social-toolbar-item isn't there.

Any item absent from the defaultset would be moved to the end in new profiles or when restoring the default set.
https://hg.mozilla.org/mozilla-central/rev/dc359d500e94
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #35)
> Comment on attachment 695644 [details] [diff] [review]
> patch v2
> >+<!ENTITY webrtcIndicatorButton.label "Media Streams">
> >+<!ENTITY webrtcIndicatorButton.tooltip "Currently active media streams">
> 
> This seems a bit jargony, but I'm not sure I have a better suggestion. Maybe
> Boriss does.
I agree that this is a big jargony, and not very consistent with the tooltip language we're using in the toolbar.  To be more consistent in our language both in GuM in this menu and in other toolbar items, let's instead write:

For sharing camera with audio (camera and mic): "Display sites you are currently sharing your camera and microphone with"

For sharing microphone only (no camera): "Display sites you are currently sharing your microphone with"

For sharing camera only (no mic): "Display sites you are currently sharing your camera with"

I don't think there's a need to adjust the language if only one site is being shared with.
Flags: needinfo?(jboriss)
(Assignee)

Comment 50

5 years ago
Since we potentially display multiple sites with different access characteristics (camera and mic, only camera, only mic) and on top of that, the code responsible for the button currently doesn't know about those characteristics, I'd slightly alter the string to say: "Display sites you are currently sharing your camera or microphone with"

And we still need a label. How about "Camera / Microphone Access"?
Sounds good to me!
Attachment #696253 - Flags: ui-review?(jboriss) → ui-review+
(Assignee)

Comment 52

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f390f26b2f7e
Whiteboard: [getUserMedia][blocking-gum+][leave open] → [getUserMedia][blocking-gum+]
https://hg.mozilla.org/mozilla-central/rev/f390f26b2f7e
Status: NEW → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: verifyme
QA Contact: jsmith
Blocks: 825594
Depends on: 828269

Updated

5 years ago
Depends on: 840218
Verified through some exploratory testing on desktop on Windows 7, OS X 10.7, and Ubuntu 12 with the gum test page and my test pc app with single vs. multiple tabs + concurrent use. As of today's build, there was one followup that Dao already knows about - the share notification is always indicating the last set of devices shared on the last gum call, not the union.
Status: RESOLVED → VERIFIED

Updated

5 years ago
Keywords: verifyme
Blocks: 795024
Component: General → Device Permissions

Comment 55

3 years ago
(In reply to Dão Gottwald [:dao] from comment #0)
> +++ This bug was initially created as a clone of Bug #729522 +++
> 
> Original mockup: attachment 666072 [details]. Tabs aren't a great place for
> this, as there's not enough space in pinned tabs and normal tabs can be
> scrolled off-screen.

Ditto on tabs not being a great place to do this.

Would really like it configurable, disable/turn off once, or permanently as well as move it OFF the tab bar or be able to drag if off.

Thanks.

My apologies if this is in the wrong place to comment on the bad placement of this indicator, please let me know if there's a better place.  Thanks again,

Rich

Comment 56

2 years ago
> Would really like it configurable, disable/turn off once, or permanently as
> well as move it OFF the tab bar or be able to drag if off.

I would very much like these options too. Is there another bug tracking the UX for the microphone/camera sharing always-on-top window. I use a tiling window manager and this notification appears even when Firefox is in another workspace and the application I want to look at is fullscreen. It's very annoying.
You need to log in before you can comment on or make changes to this bug.