Closed
Bug 1037408
Opened 10 years ago
Closed 10 years ago
implement the global getUserMedia indicator
Categories
(Firefox :: Site Permissions, defect)
Firefox
Site Permissions
Tracking
()
People
(Reporter: florian, Assigned: florian)
References
(Depends on 2 open bugs)
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
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].
Assignee | ||
Updated•10 years ago
|
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
Added to Iteration 33.3
Status: NEW → ASSIGNED
Iteration: --- → 33.3
QA Whiteboard: [qa?]
Updated•10 years ago
|
QA Whiteboard: [qa?] → [qa+]
Assignee | ||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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.
Attachment #8457505 -
Flags: feedback?(florian)
Updated•10 years ago
|
Attachment #8456511 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
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?
Flags: needinfo?(florian)
Assignee | ||
Comment 8•10 years ago
|
||
(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.
Flags: needinfo?(florian)
Assignee | ||
Comment 9•10 years ago
|
||
Assignee: gijskruitbosch+bugs → florian
Attachment #8458802 -
Flags: review?(dolske)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8458803 -
Flags: review?(dolske)
Assignee | ||
Comment 11•10 years ago
|
||
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.
Attachment #8457505 -
Attachment is obsolete: true
Attachment #8457505 -
Flags: feedback?(florian)
Attachment #8458809 -
Flags: review?(dolske)
Updated•10 years ago
|
Attachment #8458802 -
Flags: review?(dolske) → review+
Updated•10 years ago
|
Attachment #8458803 -
Flags: review?(dolske) → review+
Comment 12•10 years ago
|
||
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/camera-white-16@2x.png New Binary File: browser/themes/shared/webrtc/microphone-white-16@2x.png 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/firefox-22@2x.png 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).
Attachment #8458809 -
Flags: review?(dolske) → review+
Comment 13•10 years ago
|
||
(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>?
Assignee | ||
Comment 14•10 years ago
|
||
(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>.
Assignee | ||
Comment 15•10 years ago
|
||
(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.
Assignee | ||
Comment 16•10 years ago
|
||
Addressed review comments, carrying the r+ forward.
Attachment #8458809 -
Attachment is obsolete: true
Attachment #8459049 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
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
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
Flags: needinfo?(florian)
Assignee | ||
Comment 19•10 years ago
|
||
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
Flags: needinfo?(florian)
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cc2ca221443 https://hg.mozilla.org/mozilla-central/rev/f1b8ec714d79 https://hg.mozilla.org/mozilla-central/rev/fe54b4c9be09
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Comment 21•10 years ago
|
||
Hi Florin, can you assign a QA contact for verification of this bug.
Flags: needinfo?(florin.mezei)
Updated•10 years ago
|
Flags: needinfo?(florin.mezei)
QA Contact: bogdan.maris
Updated•10 years ago
|
Iteration: 33.3 → 34.1
Comment 22•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Assignee | ||
Updated•10 years ago
|
Component: General → Device Permissions
You need to log in
before you can comment on or make changes to this bug.
Description
•