1037415 - remove existing webrtc indicator in the browser toolbar

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1037415-remove-existing-webrtc-indicator-in-the-.patch

Hope I didn't miss anything.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please assign this to me for this iteration? Thanks!

Comment 4

[:Gijs (he/him)]

(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.

Comment 5

[:Gijs (he/him)]

(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?

Comment 6

[Dão Gottwald [:dao]]

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 7

[Florian Quèze [:florian]]

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.

Comment 8

[Marco Mucci [:MarcoM]]

Added to Iteration 34.1.  Tim, can you mark this bug as [qa+] or [qa-] for verification.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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.

Comment 10

[Dão Gottwald [:dao]]

(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.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(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?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1037415-remove-existing-webrtc-indicator-in-the-.patch, v2

(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.

Comment 13

[:Gijs (he/him)]

(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 14

[Florian Quèze [:florian]]

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.

Comment 15

[Dão Gottwald [:dao]]

(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

[Mike Conley (:mconley) (:⚙️)]

I agree - let's develop a mechanism to clean it out.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Try is looking good:

https://tbpl.mozilla.org/?tree=Try&rev=e1935b9aa6a4

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/30f0a2841510

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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

[Dão Gottwald [:dao]]

(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.

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/30f0a2841510

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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

[Cornel Ionce [:noni] [Hubs QA]]

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

Comment 24

[Florian Quèze [:florian]]

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.

Comment 25

[Paul Silaghi, QA [:pauly]]

based on comment 23

Comment 26

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e62aaf2657b

Comment 27

[Cornel Ionce [:noni] [Hubs QA]]

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.