Closed Bug 1037430 Opened 5 years ago Closed 5 years ago

implement webrtc global indicator in the Mac menubar

Categories

(Firefox :: Device Permissions, defect)

x86
macOS
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
34.1

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

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.
Flags: firefox-backlog+
Depends on: 1037433
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.
Attached patch PatchSplinter Review
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8459031 - Flags: review?(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.
Attachment #8459031 - Flags: review?(dolske) → review+
(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.
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.
(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.
Iteration: --- → 33.3
QA Whiteboard: [qa?]
Re-landing with a fix for the orange (bug 1041155):
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e6b2cf3b2e6
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/8e6b2cf3b2e6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florian, can you mark this bug as [qa+] or [qa-] for verification.
Flags: needinfo?(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.
QA Whiteboard: [qa?] → [qa+]
Flags: needinfo?(florian) → needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: bogdan.maris
Iteration: 33.3 → 34.1
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Component: General → Device Permissions
You need to log in before you can comment on or make changes to this bug.