1037408 - implement the global getUserMedia indicator

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

Attachment: Global indicator mockup

With the addition of screensharing support in getUserMedia (bug 983504), we are concerned that the existing toolbar indicator (http://i.imgur.com/NHidncc.png) may not be visible enough, and we decided (in bug 1031424) to replace it with a global indicator always visible at the top of the screen.

This new global indicator should be a small floating window, on top of all other windows. It should be possible to drag it to the left or right of the screen, but it should stick to the top of the screen.

The attached mockup for this is a subset of attachment 8453837 [details].

Comment 1

[Florian Quèze [:florian]]

If we can't get this ready before 33 merges to aurora, we should think about pre-landing the strings. The changes here don't require many strings, they only need the strings for the tooltip that should be displayed when hovering the icons.

Philipp provided these strings in bug 1031424 comment 30, we then discussed a bit on IRC and agreed to drop the "with <hostname>"/"with <count> pages" part to simplify, as this is only tooltips that we don't expect to be the primary way of informing the user.

So the strings to use are:

Left side (camera and microphone):
Your camera and microphone are being shared. Click to control sharing.
Your microphone is being shared. Click to control sharing.
Your camera is being shared. Click to control sharing.

Right side (screen sharing):
Your screen is being shared. Click to control sharing.
The application <application name> is being shared. Click to control sharing.

Comment 2

[Marco Mucci [:MarcoM]]

Added to Iteration 33.3

Comment 3

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)

> The application <application name> is being shared. Click to control sharing.

This should now say "window" rather than "application", as it's finally window sharing that will be implemented for Fx33.

Comment 4

[:Gijs (he/him)]

Attachment: add global indicator for webrtc things,

Checkpointing. This doesn't do anything but creates strings and vaguely approximates the suggested styling. Missing icons, click behaviour, integration (showing/hiding), button showing/hiding depending on what is shared, dropdowns for multiple tabs... and probably more that I've forgotten. Oh, and it shows if you click the home button, just to have a way to test it and check styling.

Comment 5

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> 
> > The application <application name> is being shared. Click to control sharing.
> 
> This should now say "window" rather than "application", as it's finally
> window sharing that will be implemented for Fx33.

(I still need to update this in the patch, too)

Comment 6

[:Gijs (he/him)]

Attachment: add global indicator for webrtc things,

This patch does:
- fix the appearance of the indicator
- integrate in some way with the existing code (although it'll conflict with the other indicator patches, but it should be easy to resolve...)
- implement the tooltips
- implement the hiding/showing of the buttons
- implement opening/focusing the right tab/window.
- hide the indicator when webrtc stops again
- stays on top

This patch doesn't:
- have tests
- have the requested box-shadow
- do dragging the indicator
- ??? I'm sure I've forgotten something, but at this point I'm not sure what.

In future, I guess we should split bugs such as these up in smaller bits, but either way... this is how far I got.

Comment 7

[Justin Dolske [:Dolske]]

Florian: With Gijs out, were you planning to pick up this work (I gather you two have discussed something like that), or should I find someone else to take it?

Comment 8

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #7)
> Florian: With Gijs out, were you planning to pick up this work (I gather you
> two have discussed something like that), or should I find someone else to
> take it?

Yes, I'll work on this today, but I would still appreciate if you could suggest someone who would be available to review the patch immediately (or at least quickly) once I have it ready.

Comment 9

[Florian Quèze [:florian]]

Attachment: Part 1 (add booleans flags in webrtcUI for each kind of sharing)

Comment 10

[Florian Quèze [:florian]]

Attachment: Part 2 (give parameters to the activeStreams getter)

Comment 11

[Florian Quèze [:florian]]

Attachment: Part 3 (add the global indicator window)

This patch is based on Gijs' attachment 8457505 [details] [diff] [review].

The indicator.css file is almost unmodified (I only added a license header and fixed the preprocessing), and I haven't actually fully reviewed it.

The JS code is changed significantly compared to Gijs' version because I've moved most of the logic to webrtcUI.jsm so that the logic can be shared with the Mac-only indicator (bug 1037430) with minimal changes.

Things that aren't handled by this patch, and that I think would be better addressed in follow-ups given the time constraints:
- moving the window (dragging, but also repositioning it if the screen resolution changes)
- adding tests.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 8458809 [details] [diff] [review]
Part 3 (add the global indicator window)

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

New Binary File: browser/themes/shared/webrtc/[e-mail]
New Binary File: browser/themes/shared/webrtc/[e-mail]

I noticed these were a bit blurry in the UI, seems the source files are just blurry upscales. But sounds like we're waiting for final icons in bug 1037418?

New Binary File: browser/themes/shared/webrtc/firefox-22.png
New Binary File: browser/themes/shared/webrtc/[e-mail]

These two Firefox logos need to go into browser/branding.

Alas the current branding has become a bit of a mess. There is already a browser/branding/official/default22.png, but there's no @2x / 44px version, and the other branding flavors don't even have a 22px version...

So, I'd suggest using the (existing) chrome://branding/content/icon48.png in your CSS, downscaling to 22/44 as needed. We can deal with getting properly sized icons in a followup bug.

::: browser/locales/en-US/chrome/browser/webrtcIndicator.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE : FILE This file contains the webrtc global indicator strings -->

I was trying to figure out why you have an ascii arrow here, and then realized this is probably a cut'n'paste leftover from a .dtd file. :)

@@ +13,5 @@
> +webrtcIndicator.sharingCameraAndMicrophone.tooltip = Your camera and microphone are being shared. Click to control sharing.
> +webrtcIndicator.sharingCamera.tooltip              = Your camera is being shared. Click to control sharing.
> +webrtcIndicator.sharingMicrophone.tooltip          = Your microphone is being shared. Click to control sharing.
> +webrtcIndicator.sharingScreen.tooltip = Your screen is being shared. Click to control sharing.
> +webrtcIndicator.sharingWindow.tooltip = A window is being shared. Click to control sharing.

Random aside, not really something to deal with here.

I wonder if "sharing" is too vague of a term in the context of a screen/window. I think it's ok with a camera/mic, since those are understandable devices, and the user is likely to understand what's happening without thinking about it deeply. Might be worth a ponder if we can find a more descriptive/understandable phrasing (especially in the permission prompt).

Comment 13

[Justin Dolske [:Dolske]]

(In reply to Florian Quèze [:florian] [:flo] from comment #11)

> Things that aren't handled by this patch, and that I think would be better
> addressed in follow-ups given the time constraints:
> - moving the window (dragging, but also repositioning it if the screen
> resolution changes)
> - adding tests.

That's fine, please file these if they're not already filed.

Speaking of tests, I'd suggest giving this a spin through Try. I would not be shocked to find that one of our existing tests is now confused by the presence of the indicator window. :)

Oh, and out of curiosity, any particular reason this is a <window> and not a <panel>?

Comment 14

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #13)

> Speaking of tests, I'd suggest giving this a spin through Try. I would not
> be shocked to find that one of our existing tests is now confused by the
> presence of the indicator window. :)

I was wondering the same thing (more specifically, I was afraid that new window could cause focus issues).

The test queue is huge currently so I don't have the full results yet, but I pushed https://tbpl.mozilla.org/?tree=Try&rev=e9d90fff1a8d a few hours ago, and the results I have for now are all green :).

> Oh, and out of curiosity, any particular reason this is a <window> and not a
> <panel>?

I don't think we really investigated using a <panel>.

Comment 15

[Florian Quèze [:florian]]

(In reply to Justin Dolske [:Dolske] from comment #12)

> I noticed these were a bit blurry in the UI, seems the source files are just
> blurry upscales. But sounds like we're waiting for final icons in bug
> 1037418?

Yes, we are. And I expect most (if not all) the webrtc icons to change soon.

Comment 16

[Florian Quèze [:florian]]

Attachment: Part 3 (add the global indicator window) v2

Addressed review comments, carrying the r+ forward.

Comment 17

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f875dc5e0df7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9cfdc0bbfbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/8833bb7e90de

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All three backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/876659559bd8 along with bug 1037430's patch for devtools orange: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=44161687&tree=Mozilla-Inbound

Comment 19

[Florian Quèze [:florian]]

Re-landing with a fix for the orange (bug 1041155):
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc2ca221443
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b8ec714d79
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe54b4c9be09

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cc2ca221443
https://hg.mozilla.org/mozilla-central/rev/f1b8ec714d79
https://hg.mozilla.org/mozilla-central/rev/fe54b4c9be09

Comment 21

[Marco Mucci [:MarcoM]]

Hi Florin, can you assign a QA contact for verification of this bug.

Comment 22

[Bogdan Maris, Desktop QA]

Verified that the global getUserMedia indicator is implemented plus did some exploratory around this on Windows 7 64bit, Ubuntu 13.04 64bit and Mac OS X 10.9.4 using latest Nightly.
The dependencies will be verified individually.