Closed
Bug 1067444
Opened 10 years ago
Closed 10 years ago
Clicking the microphone button on the global webrtc sharing indicator opens a doorhanger with a camera icon
Categories
(Firefox :: Site Permissions, defect)
Tracking
()
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Gavin
:
review+
lmandel
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Hi Florian, should this bug be in the backlog and in the 35.2 priority list?
Flags: needinfo?(florian)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Attachment #8489439 -
Flags: review?(felipc)
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Points: --- → 1
Flags: qe-verify+
Flags: needinfo?(florian)
Flags: firefox-backlog+
Comment 4•10 years ago
|
||
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.
Updated•10 years ago
|
Iteration: --- → 35.2
Updated•10 years ago
|
QA Contact: drno
Comment 5•10 years ago
|
||
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?
Attachment #8489439 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8489439 -
Attachment is obsolete: true
Attachment #8493750 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8493750 -
Attachment is obsolete: true
Attachment #8493750 -
Flags: review?(gavin.sharp)
Attachment #8493774 -
Flags: review?(gavin.sharp)
Comment 8•10 years ago
|
||
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?
Attachment #8493774 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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
Assignee | ||
Comment 10•10 years ago
|
||
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.
Attachment #8493774 -
Flags: approval-mozilla-aurora?
Comment 11•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 12•10 years ago
|
||
Comment on attachment 8493774 [details] [diff] [review]
Patch v2 (with test)
Aurora+
Attachment #8493774 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Verified fixed on Windows 7 32bit and 64bit using latest Aurora 35.0a2 (buildID: 20141019004001) and Firefox 34 Beta 1 (buildID: 20141014134955).
Assignee | ||
Updated•10 years ago
|
Component: General → Device Permissions
You need to log in
before you can comment on or make changes to this bug.
Description
•