Closed
Bug 1037415
Opened 10 years ago
Closed 10 years ago
remove existing webrtc indicator in the browser toolbar
Categories
(Firefox :: Site Permissions, defect)
Firefox
Site Permissions
Tracking
()
People
(Reporter: florian, Assigned: ttaubert)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
florian
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The WebRTC indicator currently appearing in the browser toolbar while a device is being shared (http://i.imgur.com/NHidncc.png) will be replaced by a much more visible global indicator in bug 1037408. Once that's done, we should remove the existing toolbar indicator. Gijs pointed out that this may not be as trivial as it seems because we may need a migration to get rid of it from customized toolbars. The existing unit tests will also need to be adapted.
Flags: firefox-backlog+
Assignee | ||
Comment 1•10 years ago
|
||
Hope I didn't miss anything.
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #0) > Gijs pointed out that this may not be as trivial as it seems because we may > need a migration to get rid of it from customized toolbars. Any pointer on how to do this?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 3•10 years ago
|
||
Marco, can you please assign this to me for this iteration? Thanks!
Iteration: --- → 34.1
Flags: needinfo?(mmucci)
Comment 4•10 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #2) > (In reply to Florian Quèze [:florian] [:flo] from comment #0) > > Gijs pointed out that this may not be as trivial as it seems because we may > > need a migration to get rid of it from customized toolbars. > > Any pointer on how to do this? Generally, http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1346 and further have plenty of stuff that parses and chucks stuff out of currentset. However, you'll also need to adjust the placements stored by CustomizableUI. They're in JSON in a pref. You could either manually read that pref yourself and update it and save it back, assuming CustomizableUI hasn't yet been loaded at the point where the migration runs (keep in mind the pref might not exist at all or be bogus...). Or you could load CustomizableUI and call removeWidgetFromArea("webrtc-button"), which I believe should work... but I've obviously never tested that, so you'd have to do some thorough manual testing.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 5•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #4) > However, you'll also need to adjust the placements stored by CustomizableUI. > They're in JSON in a pref. You could either manually read that pref yourself > and update it and save it back, assuming CustomizableUI hasn't yet been > loaded at the point where the migration runs (keep in mind the pref might > not exist at all or be bogus...). Or you could load CustomizableUI and call > removeWidgetFromArea("webrtc-button"), which I believe should work... but > I've obviously never tested that, so you'd have to do some thorough manual > testing. Mike and Blair, want to give an opinion on which of these should be preferred? Or we could add another bit to CUI itself to just remove things itself, I guess, in line with it now having its own versions anyway?
Flags: needinfo?(mconley)
Flags: needinfo?(bmcbride)
Comment 6•10 years ago
|
||
As far as I can tell, nothing should be migrated here. If webrtc-status-button doesn't exist anymore, any reference to it in toolbar placements should just be ignored. This is just like when you move an add-on provided toolbar widget around and then you uninstall the add-on. By the way, does your patch make the icon in Toolbar.png unused?
Reporter | ||
Comment 7•10 years ago
|
||
Comment on attachment 8460939 [details] [diff] [review] 0001-Bug-1037415-remove-existing-webrtc-indicator-in-the-.patch Review of attachment 8460939 [details] [diff] [review]: ----------------------------------------------------------------- I haven't fully reviewed the rest of the patch, but what I saw looks good. ::: browser/base/content/test/general/browser_devices_get_user_media.js @@ -185,5 @@ > if (!aAlreadyClosed) > expectObserverCalled("recording-window-ended"); > - > - let statusButton = document.getElementById("webrtc-status-button"); > - ok(statusButton.hidden, "WebRTC status button hidden"); Is there a reason why you didn't convert this to an assertWebRTCIndicatorStatus call? @@ +202,5 @@ > > function checkSharingUI() { > yield promisePopupNotification("webRTC-sharingDevices"); > + > + assertWebRTCIndicatorStatus(true, "WebRTC indicator visible"); Should the message be automatically generated by assertWebRTCIndicatorStatus, based on the value of the first parameter? ::: browser/base/content/test/general/head.js @@ +519,5 @@ > + let iw = Cu.import("resource:///modules/webrtcUI.jsm", {}).gIndicatorWindow; > + status = iw && ["_camera", "_microphone", "_screen"].some(prop => iw[prop]); > + } else { > + status = Services.wm.getMostRecentWindow("Browser:WebRTCGlobalIndicator"); > + } You've already done most of the work I expected to happen in bug 1041658. What remains to test here is: - that webrtcUI.showGlobalIndicator matches the value of the "visible" parameter. - that the values of webrtcUI.show{Camera,Microphone,ScreenSharing}Indicator match the attributes set on the global indicators. I think it would make sense to check showGlobalIndicator here, and we can handle the rest in bug 1041658.
Attachment #8460939 -
Flags: review?(florian) → feedback+
Comment 8•10 years ago
|
||
Added to Iteration 34.1. Tim, can you mark this bug as [qa+] or [qa-] for verification.
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
Reporter | ||
Updated•10 years ago
|
QA Whiteboard: [qa?] → [qa+]
Comment 9•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6) > As far as I can tell, nothing should be migrated here. If > webrtc-status-button doesn't exist anymore, any reference to it in toolbar > placements should just be ignored. This is true - everything will work fine without any migration. However, webrtc-status-button will otherwise never be removed from the prefs - we'll be keeping useless data around forever (we don't have an expiration strategy yet). It also means that if any add-on starts using that ID, it'll get treated differently from other add-ons when it gets installed. (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #5) > Or we could add another bit to CUI itself to just remove things > itself, I guess, in line with it now having its own versions anyway? This would by my preferred strategy. Outside code shouldn't be touching that pref, nor need to know it's data structure. IMO, we can do this in a followup - and build a generic mechanism for dealing with this.
Flags: needinfo?(bmcbride)
Comment 10•10 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #9) > (In reply to Dão Gottwald [:dao] from comment #6) > > As far as I can tell, nothing should be migrated here. If > > webrtc-status-button doesn't exist anymore, any reference to it in toolbar > > placements should just be ignored. > > This is true - everything will work fine without any migration. > > However, webrtc-status-button will otherwise never be removed from the prefs > - we'll be keeping useless data around forever (we don't have an expiration > strategy yet). You mean forever even if the user customizes toolbars and the pref gets written again? That would be different from how toolkit's customization code works and we should probably get it in line with that. Otherwise what you say doesn't sound like a problem to me, let alone a new problem. > It also means that if any add-on starts using that ID, it'll > get treated differently from other add-ons when it gets installed. If an add-on was using the same ID, then it likely wants to provide a replacement for the widget we removed, so it would make sense to re-add it where the original one was. This is similar to placing an add-on widget somewhere, removing the add-on and re-installing it. This behavior is also useful when Firefox needs to be downgraded such that webrtc-status-button would exist again. Again, this is how the classic customization code works too.
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #6) > By the way, does your patch make the icon in Toolbar.png unused? Yeah, should we remove this icon in a follow-up and adjust rules for subsequent buttons?
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #7) > ::: browser/base/content/test/general/browser_devices_get_user_media.js > @@ -185,5 @@ > > if (!aAlreadyClosed) > > expectObserverCalled("recording-window-ended"); > > - > > - let statusButton = document.getElementById("webrtc-status-button"); > > - ok(statusButton.hidden, "WebRTC status button hidden"); > > Is there a reason why you didn't convert this to an > assertWebRTCIndicatorStatus call? Must've been left over from a previous approach. Using assertWebRTCIndicatorStatus() now. > @@ +202,5 @@ > > > > function checkSharingUI() { > > yield promisePopupNotification("webRTC-sharingDevices"); > > + > > + assertWebRTCIndicatorStatus(true, "WebRTC indicator visible"); > > Should the message be automatically generated by > assertWebRTCIndicatorStatus, based on the value of the first parameter? Sure, done. > I think it would make sense to check showGlobalIndicator here, and we can > handle the rest in bug 1041658. Ok, did that.
Attachment #8460939 -
Attachment is obsolete: true
Attachment #8461379 -
Flags: review?(florian)
Comment 13•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #10) > (In reply to Blair McBride [:Unfocused] from comment #9) > > (In reply to Dão Gottwald [:dao] from comment #6) > > > As far as I can tell, nothing should be migrated here. If > > > webrtc-status-button doesn't exist anymore, any reference to it in toolbar > > > placements should just be ignored. > > > > This is true - everything will work fine without any migration. > > > > However, webrtc-status-button will otherwise never be removed from the prefs > > - we'll be keeping useless data around forever (we don't have an expiration > > strategy yet). > > You mean forever even if the user customizes toolbars and the pref gets > written again? That would be different from how toolkit's customization code > works and we should probably get it in line with that. If that's true, then toolkit's customization code breaks the "uninstall and reinstall an add-on" (or even enable/disable) case you outlined if people customize inbetween. Come to think of it, that might explain why add-ons were so insistent on always reinserting their buttons if no longer present. CUI doesn't break this case, but yes, it means it'll stay around forever. Hence why we should be cleaning it up. I agree with Blair that we should followup this.
Reporter | ||
Comment 14•10 years ago
|
||
Comment on attachment 8461379 [details] [diff] [review] 0001-Bug-1037415-remove-existing-webrtc-indicator-in-the-.patch, v2 Review of attachment 8461379 [details] [diff] [review]: ----------------------------------------------------------------- I have no opinion on the CustomizableUI discussion in the previous comments, but my understanding is that it will be figured out in the follow-up. The patch looks good to me, thanks.
Attachment #8461379 -
Flags: review?(florian) → review+
Comment 15•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #13) > (In reply to Dão Gottwald [:dao] from comment #10) > > (In reply to Blair McBride [:Unfocused] from comment #9) > > > (In reply to Dão Gottwald [:dao] from comment #6) > > > > As far as I can tell, nothing should be migrated here. If > > > > webrtc-status-button doesn't exist anymore, any reference to it in toolbar > > > > placements should just be ignored. > > > > > > This is true - everything will work fine without any migration. > > > > > > However, webrtc-status-button will otherwise never be removed from the prefs > > > - we'll be keeping useless data around forever (we don't have an expiration > > > strategy yet). > > > > You mean forever even if the user customizes toolbars and the pref gets > > written again? That would be different from how toolkit's customization code > > works and we should probably get it in line with that. > > If that's true, then toolkit's customization code breaks the "uninstall and > reinstall an add-on" (or even enable/disable) case you outlined if people > customize inbetween. Sure, it's treated like a new installation in that case. Given that the user customized toolbars in between, this seems like a reasonable middle ground. In particular it's not clear where exactly the dead reference should be kept when adjacent widgets are moved, so I think it's saner to drop it in that case.
Comment 16•10 years ago
|
||
I agree - let's develop a mechanism to clean it out.
Flags: needinfo?(mconley)
QA Contact: cornel.ionce
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Assignee | ||
Comment 17•10 years ago
|
||
Try is looking good: https://tbpl.mozilla.org/?tree=Try&rev=e1935b9aa6a4
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/30f0a2841510
Comment 19•10 years ago
|
||
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #13) > (In reply to Dão Gottwald [:dao] from comment #10) > > You mean forever even if the user customizes toolbars and the pref gets > > written again? That would be different from how toolkit's customization code > > works and we should probably get it in line with that. > > If that's true, then toolkit's customization code breaks the "uninstall and > reinstall an add-on" (or even enable/disable) case you outlined if people > customize inbetween. Come to think of it, that might explain why add-ons > were so insistent on always reinserting their buttons if no longer present. > > CUI doesn't break this case, but yes, it means it'll stay around forever. > Hence why we should be cleaning it up. I agree with Blair that we should > followup this. Yea, this is intentionally different from what toolkit does, specifically to fix that type of use case & add-ons attempting to fix it themselves (and getting it wrong).
Comment 20•10 years ago
|
||
(In reply to Blair McBride [:Unfocused] from comment #19) > (In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #13) > > If that's true, then toolkit's customization code breaks the "uninstall and > > reinstall an add-on" (or even enable/disable) case you outlined if people > > customize inbetween. Come to think of it, that might explain why add-ons > > were so insistent on always reinserting their buttons if no longer present. > > > > CUI doesn't break this case, but yes, it means it'll stay around forever. > > Hence why we should be cleaning it up. I agree with Blair that we should > > followup this. > > Yea, this is intentionally different from what toolkit does, specifically to > fix that type of use case & add-ons attempting to fix it themselves (and > getting it wrong). Not sure what you mean here. Like I said, if an add-on is uninstalled, the reference on the toolbar disappears due to the user customizing toolbars, and the add-on is re-installed, then toolbars are in the same state they would be in if the add-on was installed for the first time, which is something the add-on obviously needs to handle anyway. Furthermore, if you're concerned about the pref inflating due to removed built-in widgets sticking around forever, then this concern should apply to add-on items too, as add-ons are uninstalled without being re-installed all the time.
https://hg.mozilla.org/mozilla-central/rev/30f0a2841510
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment 22•10 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #20) > Furthermore, if you're concerned about the pref inflating due to removed > built-in widgets sticking around forever, then this concern should apply to > add-on items too, as add-ons are uninstalled without being re-installed all > the time. Yea, that's why I mentioned the want for an expiry mechanism.
Comment 23•10 years ago
|
||
Confirming that the webrtc indicator is removed from the browser toolbar in latest Nightly, build ID: 20140727030204 using the http://queze.net/goinfre/ff_gum_test.html testpage. Tested on Windows 7 64bit, Mac OS X 10.9.3 and Ubuntu 12.04 User Agents: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:34.0) Gecko/20100101 Firefox/34.0 Mozilla/5.0 (X11; Linux i686; rv:34.0) Gecko/20100101 Firefox/34.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Reporter | ||
Comment 24•10 years ago
|
||
Comment on attachment 8461379 [details] [diff] [review] 0001-Bug-1037415-remove-existing-webrtc-indicator-in-the-.patch, v2 Approval Request Comment [Feature/regressing bug #]: bug 1037408 [User impact if declined]: the global indicator replaces the toolbar indicator, which should be removed. [Describe test coverage new/current, TBPL]: currently in m-c. [Risks and why]: Low, mostly code removal. [String/UUID change made/needed]: the patch that landed on trunk removes 2 obsolete strings. We won't remove these strings on aurora.
Attachment #8461379 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox34:
--- → fixed
Updated•10 years ago
|
Attachment #8461379 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•10 years ago
|
||
based on comment 23
Reporter | ||
Comment 26•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e62aaf2657b
QA Whiteboard: [qa!] → [qa+]
Comment 27•10 years ago
|
||
The webRTC indicator is removed from the browser toolbar in latest Aurora, build ID: 20140813004001. Tested on Windows 7 64bit, Ubuntu 12.04 and Mac OS X 10.8.5.
QA Whiteboard: [qa+] → [qa!]
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Reporter | ||
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
•