1067444 - Clicking the microphone button on the global webrtc sharing indicator opens a doorhanger with a camera icon

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

Steps to reproduce:
1. Load http://people.mozilla.org/~fqueze2/webrtc/
2. Click the "Audio" button.
3. In the doorhanger, click "Share selected devices"
4. See the global sharing indicator at the top of the screen and click the microphone icon.

Expected result:
The microphone sharing doorhanger opens with a microphone icon.

Actual result:
The sharing doorhanger opens attached to the microphone url bar icon but contains a camera icon.


I see this on Windows, I haven't been able to reproduce it on Mac.

Comment 1

[Marco Mucci [:MarcoM]]

Hi Florian, should this bug be in the backlog and in the 35.2 priority list?

Comment 2

[Florian Quèze [:florian]]

Attachment: Patch

The hack from bug 876041 to have either the camera or microphone icon for a single notification id relies on the "shown" event being sent to the event callback of the notification after the panel has been initialized.

When clicking the microphone icon of the global sharing indicator, the notification is first shown by the webrtcUI code, and then refreshed when the PopupNotifications code receives the "activate" event from the window. When the notification is refreshed, the code currently sends the "showing" notification again, but the "shown" notification is never sent for that case because the code returns early.

Comment 3

[Florian Quèze [:florian]]

(In reply to Marco Mucci [:MarcoM] from comment #1)
> Hi Florian, should this bug be in the backlog and in the 35.2 priority list?

I think it should be in the backlog. I don't think it would have reached the top of the 35.2 priority list, but I debugged this issue immediately to verify it wasn't a regression caused by the changes I'm doing in bug 1056179.

Comment 4

[Marco Mucci [:MarcoM]]

Thanks Florian, I'll get it added.

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Marco Mucci [:MarcoM] from comment #1)
> > Hi Florian, should this bug be in the backlog and in the 35.2 priority list?
> 
> I think it should be in the backlog. I don't think it would have reached the
> top of the 35.2 priority list, but I debugged this issue immediately to
> verify it wasn't a regression caused by the changes I'm doing in bug 1056179.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

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

Hard to tell whether this will break any other consumer expectations, but it seems like the sanest behavior.

Can you write a test for this as well?

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v2 (with test)

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v2 (with test)

I noticed when adding another test in the same file that the test shouldn't contain the goNext() line as it's automatically done by head.js if an onHidden method exists.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8493774 [details] [diff] [review]
Patch v2 (with test)

Ah, I see the difficulty with _update() here. Couldn't you use a PopupNotification eventCallback and just have the test fail if "shown" doesn't fire again?

Comment 9

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #8)

> Ah, I see the difficulty with _update() here. Couldn't you use a
> PopupNotification eventCallback and just have the test fail if "shown"
> doesn't fire again?

As discussed yesterday, the eventCallback is already set automatically for these tests, so if we wanted to go this way, we would probably need to write a completely new test instead of just adding something to the existing test file. This would be possible, but I think it's not worth it.

Try was green on the new test: https://tbpl.mozilla.org/?tree=Try&rev=5845cf9f75a3

https://hg.mozilla.org/integration/fx-team/rev/7d23f5247b47

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8493774 [details] [diff] [review]
Patch v2 (with test)

Approval Request Comment
[Feature/regressing bug #]: The bug has been here for a long time, but it became visible with bug 1037408 that landed for Firefox 33.
[User impact if declined]: A camera icon may be shown for a doorhanger about the microphone.
[Describe test coverage new/current, TBPL]: there's a test included in the patch.
[Risks and why]: could possibly break some consumers of the SHOWN notification, as mentioned in comment 5, but this is unlikely. This small risk is the reason why I'm not requesting beta approval too.
[String/UUID change made/needed]: none.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d23f5247b47

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8493774 [details] [diff] [review]
Patch v2 (with test)

Aurora+

Comment 13

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/60c435a37668

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 32bit and 64bit using latest Aurora 35.0a2 (buildID: 20141019004001) and Firefox 34 Beta 1 (buildID: 20141014134955).