Closed Bug 1315228 Opened 8 years ago Closed 8 years ago

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


(Firefox :: Site Permissions, defect, P1)




Firefox 52
Tracking Status
firefox52 --- fixed


(Reporter: florian, Assigned: florian)


(Depends on 1 open bug)


(Whiteboard: [fxprivacy])


(1 file, 1 obsolete file)

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.)
Attached patch Patch (obsolete) — Splinter Review
The behavior is already tested by for the microphone case, so I didn't bother adding a test for the popupId option specifically.
Attachment #8807511 - Flags: review?(paolo.mozmail)
Assignee: nobody → florian
Comment on attachment 8807511 [details] [diff] [review]

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

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.
Attachment #8807511 - Flags: review?(paolo.mozmail)
(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.
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.
Attached patch Patch v2Splinter Review
Attachment #8807638 - Flags: review?(paolo.mozmail)
Attachment #8807511 - Attachment is obsolete: true
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/
@@ +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?
Attachment #8807638 - Flags: review?(paolo.mozmail) → review+
Priority: -- → P1
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Bug 1315228 - add a popupIconClass option to popup notifications so that the microphone and screensharing icons can be set without hacks, r=paolo.
Depends on: 1316315
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Iteration: --- → 53.1 - Nov 28
Iteration: 53.1 - Nov 28 → ---
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.
You need to log in before you can comment on or make changes to this bug.