Closed Bug 1037415 Opened 5 years ago Closed 5 years ago

remove existing webrtc indicator in the browser toolbar

Categories

(Firefox :: Device Permissions, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.1
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: florian, Assigned: ttaubert)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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+
Blocks: 1035577
Blocks: 1040061
Hope I didn't miss anything.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8460939 - Flags: review?(florian)
(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)
Marco, can you please assign this to me for this iteration? Thanks!
Iteration: --- → 34.1
Flags: needinfo?(mmucci)
(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)
(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)
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?
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+
Added to Iteration 34.1.  Tim, can you mark this bug as [qa+] or [qa-] for verification.
QA Whiteboard: [qa?]
Flags: needinfo?(mmucci)
QA Whiteboard: [qa?] → [qa+]
(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)
(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.
(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?
(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)
(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.
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+
(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.
I agree - let's develop a mechanism to clean it out.
Flags: needinfo?(mconley)
QA Contact: cornel.ionce
Whiteboard: [sceensharing-uplift]
Depends on: 1043864
(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).
Depends on: 1043866
(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: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
(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.
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!]
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?
Attachment #8461379 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!]
Whiteboard: [sceensharing-uplift]
Component: General → Device Permissions
You need to log in before you can comment on or make changes to this bug.