1037430 - implement webrtc global indicator in the Mac menubar

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

On Mac, instead of having a floating window at the top of the screen (bug 1037408), we would like to have something integrated in the menubar.

See attachment 8453883 [details] for the expected result.

This needs platform support for adding icons to the menubar.

Comment 1

[Florian Quèze [:florian]]

Here is the specification that Philipp emailed me:
<quote>
I thought a little about the Mac menu bar icons. As you mentioned, it is quite uncommon for them to do anything other than opening a menu. So here’s a proposal:

If there’s only one site using devices, the menu should have the following items:

    Sharing <device type> with <domain> (this line is greyed out)
    Control sharing (takes you to the tab and opens the door hanger)


If there’s more than one site:

    Sharing <device type> with 3 tabs (greyed out)
    Control sharing on <domain 1>
    Control sharing on <domain 2>
    Control sharing on <domain 3>


Device type can be Camera, Microphone or Screen.
</quote>

We discussed a bit and here are the changes we decided:
- use the tab titles instead of the domains, for consistency with what we are doing with the other indicators.
- adding a fourth device type: "Window". If there's both a screen share and a window share, we display "screen" as the window is a subset of the screen.

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 8459031 [details] [diff] [review]
Patch

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

::: browser/locales/en-US/chrome/browser/webrtcIndicator.properties
@@ +20,5 @@
> +# LOCALIZATION NOTE : The following strings are only used on Mac for
> +# menus attached to icons near the clock on the mac menubar.
> +
> +# LOCALIZATION NOTE (webrtcIndicator.sharing*With.menuitem):
> +# %S is the title of the tab using the share.

Hmm. I thought we were using URLs, but I see we changed this awhile ago, and in fact I supported it in bug 871931 comment 6. I got thinking about this because the menu isn't obviously associated with Firefox, and so the potential for malicious/confusing titles is greater.

But I think that's addressed here by always having the "Sharing device with" prefix.

::: browser/modules/webrtcUI.jsm
@@ +583,5 @@
>  
>    if (webrtcUI.showGlobalIndicator) {
> +    if (!gIndicatorWindow)
> +      gIndicatorWindow = getGlobalIndicator();
> +    else

Seems like this should really be:

  if (!gIndicatorWindow)
    gIndicatorWindow = getGlobalIndicator();
  gIndicatorWindow.updateIndicatorState();

(i.e., do the else unconditionally)

Then the OS X flavor of getGlobalIndicator() doesn't need to do an updateIndicatorState itself.

Comment 4

[Florian Quèze [:florian]]

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

> ::: browser/modules/webrtcUI.jsm
> @@ +583,5 @@
> >  
> >    if (webrtcUI.showGlobalIndicator) {
> > +    if (!gIndicatorWindow)
> > +      gIndicatorWindow = getGlobalIndicator();
> > +    else
> 
> Seems like this should really be:
> 
>   if (!gIndicatorWindow)
>     gIndicatorWindow = getGlobalIndicator();
>   gIndicatorWindow.updateIndicatorState();
> 
> (i.e., do the else unconditionally)
> 
> Then the OS X flavor of getGlobalIndicator() doesn't need to do an
> updateIndicatorState itself.

I think this would fail when the indicator is a window, because we would attempt to call updateIndicatorState before the window's load event.

Comment 5

[Justin Dolske [:Dolske]]

Actually, one more comment. Bug 1037433 comment 4 notes that hidpi icons are not working yet, but that bug 899334 will fix this (which now has a reviewed patch).

Should a hidpi icon be added here now?

If not, a followup bug would be good.

Comment 6

[Florian Quèze [:florian]]

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

> Should a hidpi icon be added here now?
> 
> If not, a followup bug would be good.

I would prefer a follow-up, as I'm currently unable to test if the hidpi icon works, due to bug 899334.

Comment 7

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1db5409ea062

Comment 8

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

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

Comment 9

[Florian Quèze [:florian]]

Re-landing with a fix for the orange (bug 1041155):
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6b2cf3b2e6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e6b2cf3b2e6

Comment 11

[Marco Mucci [:MarcoM]]

Hi Florian, can you mark this bug as [qa+] or [qa-] for verification.

Comment 12

[Florian Quèze [:florian]]

qa+. Hi Florin, this is the Mac-specific version of bug 1037408, so I think it would be ideal to have the same person doing QA for both.

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Verified that the webrtc global indicator for Mac is implemented plus did some exploratory around this on Mac OS X 10.9.4 using latest Nightly.
The dependencies will be verified individually.