1315228 - The camera icon flashes before the screen sharing icon is displayed on the screen sharing permission prompt

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Florian Quèze [:florian]]

In bug 876041 I used a hack to display the microphone icon instead of the camera icon when we were only requesting the microphone. The screen sharing case uses the same hack: we let the PopupNotifications.jsm code set the popupid to the camera one, and the next time eventCallback is fired, we change the popupid attribute from webrtcUI.jsm. Unfortunately this is using the notification shown callback, as the notification showing callback which would be better is fired right before the popupid is set.

This hack causes the camera icon to be briefly shown before it's replaced by the microphone or screensharing icon. I think this is a regression (or at least it wasn't that noticeable at the time I landed bug 876041). I suspect the change happened when we replaced the PNG icons with the SVG ones; maybe the PNG icons took longer to be decoded, and didn't have time to be displayed before we changed the popupid attribute?

I think the correct solution is to add a popupId option on notifications. (It's unfortunate that there's already an unused popupIconUrl option that does something similar, but requires hardcoding the icon URL from the JS code.)

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch

The behavior is already tested by http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/base/content/test/webrtc/browser_devices_get_user_media.js#56 for the microphone case, so I didn't bother adding a test for the popupId option specifically.

Comment 2

[:Paolo Amadini]

Comment on attachment 8807511 [details] [diff] [review]
Patch

Calling this option popupId is really confusing.

What we need is an option to set an additional class on the icon inside the panel, for example "camera-icon", then use it and remove the [popupid] rules in "notification-icons.inc.css".

I thought there was a bug on file to do that, but I can't find it, maybe it was repurposed. So, while not necessary in this bug, bonus points if you use the new option for the other permissions too :-)

Unless you want to do the full work and set the class on the anchor too, I suspect the name of the option should make it clear that this class is not added to the anchor icon.

Comment 3

[Florian Quèze [:florian]]

(In reply to :Paolo Amadini from comment #2)

> So, while not necessary in this bug, bonus points if you use
> the new option for the other permissions too :-)

It's an option because it's to handle a specific edge case, if we have to use it everywhere, it's no longer an option, and causes more work.

Comment 4

[:Paolo Amadini]

What I'm saying is that the icon to use should have been an explicit PopupNotification API option from the start, defaulting to something sensible if unspecified. There is a similar parameter in most message box APIs. Instead we overloaded the popup ID, which is really used for other purposes, causing the issue you had to work around here.

Converting other consumers to use this capability is more work (although trivial) than doing only WebRTC, and that's why I said I'm fine with only converting the WebRTC consumer here.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 6

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8427db0c3d10

Comment 7

[:Paolo Amadini]

Comment on attachment 8807638 [details] [diff] [review]
Patch v2

Review of attachment 8807638 [details] [diff] [review]:
-----------------------------------------------------------------

Unconditional r+ on the PopupNotifications.jsm changes. We need a specific PopupNotifications.jsm test, but the change is simple enough that we can file a bug for adding it later, and for now it's covered indirectly by the WebRTC tests.

Conditional r+ on the rest with the changes below.

::: browser/themes/shared/notification-icons.inc.css
@@ +113,5 @@
>    list-style-image: url(chrome://browser/skin/notification-icons.svg#login-detailed);
>  }
>  
>  .camera-icon,
> +.webRTC-shareDevices-notification-icon {

Just ".camera-icon", remove the other selector.

@@ +142,1 @@
>    list-style-image: url(chrome://browser/skin/notification-icons.svg#microphone-detailed);

This should be ".popup-notification-icon.microphone-icon".

@@ +146,1 @@
>  .screen-icon {

Just ".screen-icon", remove the other selector.

Adjust tests and code accordingly to use the "camera-icon", "microphone-icon" and "screen-icon" classes.

::: toolkit/modules/PopupNotifications.jsm
@@ +374,5 @@
>     *                     actions needs to have the 'dismiss' property set to true.
> +   *        popupIconClass:
> +   *                     A string. A class that will be applied to the icon in the
> +   *                     popup so that several notifications using the same panel
> +   *                     can use different icons.

nit: This just happens to work with multiple classes separated by space. Should we support it explicitly?

Comment 8

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41e192bec53699a83c3002bb374c13909f6d6135
Bug 1315228 - add a popupIconClass option to popup notifications so that the microphone and screensharing icons can be set without hacks, r=paolo.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41e192bec536

Comment 10

[Andrei Vaida [:avaida]]

We should be able to cover this through our regular WebRTC tests.

Florian, feel free to ni? me if there's anything in particular we should be checking here.