Steps: 1. Go to http://mozilla.github.com/webrtc-landing/gum_test.html 2. Select Video 3. Accept permissions for your camera 4. Reload the page Expected: No doorhanger should present - you are no longer sharing your camera after a reload. Actual: The doorhanger persists after a reload, even though after the reload, you are not sharing your camera anymore.

See 799417 comment 32 (2. point)

(In reply to Dão Gottwald [:dao] from comment #1) > See 799417 comment 32 (2. point) bug 799417 comment 32

(In reply to Dão Gottwald [:dao] from comment #2) > (In reply to Dão Gottwald [:dao] from comment #1) > > See 799417 comment 32 (2. point) > > bug 799417 comment 32 Ah yeah that. Are there bugs filed for all known issues or are there examples of issues we don't have bugs on file for yet? If so, can you get bugs on file then or would you like me to do it?

You said bug 802538 would be the likely cause, so I didn't file it. The first point of bug 799417 comment 32 could still use a bug if desired.

tested this several different ways (with the bug 822956 patches). Works for me, can you retest? if it repros logs with mediamanager:5 would help

(In reply to Randell Jesup [:jesup] from comment #5) > tested this several different ways (with the bug 822956 patches). Works for > me, can you retest? if it repros logs with mediamanager:5 would help Definitely reproducible for me on Win 7 64-bit. For the logs, do I need to build a debug build to get those logs?

Yes, debug build. Thanks!

I see it now: I was confused by 'doorhanger' - the dropdown doorhanger does not stay open, but the icon in the left of the URLbar does stay on when it should not. The transient drop-down button on the right does go away, so I believe the backend is correctly indicating the state. I haven't verified this, however.

This appears to be a popup notifications bug. E.g.: - load https://maps.google.com/ - hit the button for showing your current location - reload the page

Apparently browser code is responsible for telling the popup notifications code that the page changed.

Am I missing something? Is aRequest null in cases where we should remove notifications or non-null in cases where we shouldn't remove them?

Seems like maybe we want to check for LOCATION_CHANGE_SAME_DOCUMENT, but I guess that doesn't cover the tab-switch case. Maybe we should fix that by having the updateCurrentBrowser caller of _callProgressListeners(null, "onLocationChange", ...) pass that flag. What do you think?

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13) > Seems like maybe we want to check for LOCATION_CHANGE_SAME_DOCUMENT, but I > guess that doesn't cover the tab-switch case. Maybe we should fix that by > having the updateCurrentBrowser caller of _callProgressListeners(null, > "onLocationChange", ...) pass that flag. What do you think? Sounds reasonable. Does the aRequest null check behave any differently, though?

Dao - would this bug fix the following scenario or should I file a separate bug? STR 1. Request access to the camera 2. Accept permissions 3. Start playing webcam stream in the video tag 4. Stop playing the webcam stream and release access to the camera (e.g. call stop()) Expected: No UI should be seen that the camera is actively being used. Actual: The green camera icon UI still persists after releasing the camera.

Actually, taking a skim of the patch implies this is a different bug. I'll file a different bug for comment 15.

I also moved this into the (aWebProgress.DOMWindow == content) section. The legacy code effectively did the same by comparing the top level window location's spec.

Comment on attachment 707753 [details] [diff] [review] patch v2 Add a comment before the tabbrowser.xml change to explain that we're passing that flag to make this particular front-end logic work? Arguably "tab switch" isn't really quite the same as "same-document location change", but they're close enough that I think it's fine. We could use some tests for this (testing the notification/popupnotification behavior across navigation/anchor navigation/tab switch). I think there is some coverage in browser_popupNotification.js, but that's only for popup notification and it likely isn't complete.

(In reply to Dão Gottwald [:dao] from comment #14) > Sounds reasonable. Does the aRequest null check behave any differently, > though? Oh, missed this. I did try to track down all the cases where aRequest might be null. I think they do map directly to the cases where this flag is set (with the patch), but that seems like a less-obvious thing to rely on.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #18) > Comment on attachment 707753 [details] [diff] [review] > patch v2 > > Add a comment before the tabbrowser.xml change to explain that we're passing > that flag to make this particular front-end logic work? Arguably "tab > switch" isn't really quite the same as "same-document location change", but > they're close enough that I think it's fine. I tried to write that comment, but couldn't figure out how to phrase it such that it really makes sense. I believe the logic isn't quite sane, for example it's not what the "Disable find commands" code wants when checking the LOCATION_CHANGE_SAME_DOCUMENT flag. So I reverted the tabbrowser.xml change. I moved removeTransientNotifications() into TabsProgressListener.onLocationChange, which is called for background tabs (another bug in the previous code) but not for tab switches. I wanted to move PopupNotifications.locationChange() too, but seemingly that API isn't ready to handle location changes in background tabs.

(In reply to Dão Gottwald [:dao] from comment #20) > I wanted to move PopupNotifications.locationChange() > too, but seemingly that API isn't ready to handle location changes in > background tabs. Which tests fail? It may be relatively straightforward to fix.

It's not about failing tests but API design. PopupNotifications.locationChange is called "to indicate that the current browser's location has changed"; there's no way to call it for browsers other than the current one.

Ah, I see. We could fix that easily enough by giving it a "browser" parameter, I think.

Comment on attachment 710596 [details] [diff] [review] patch v3 Can you file a followup for the XXX, CC me?

Will need an uplift to Aurora to be included in FF 20 release of gUM.

This has been backed out for test_keycodes.xul failures. I have no idea yet what that test has to do with the code that I touched.

(In reply to Dão Gottwald [:dao] from comment #27) > This has been backed out for test_keycodes.xul failures. I have no idea yet > what that test has to do with the code that I touched. It's somehow related to the notification bar changes. I'll move them to a separate bug.

Comment on attachment 711857 [details] [diff] [review] patch v3 without notification bar changes [Approval Request Comment] Bug caused by (feature/regressing bug #): not a recent regression; new exposure via the getUserMedia approval UI User impact if declined: stale popup notifications when reloading a page (see comment 0) Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: none

Verified through some sanity testing on the gum test page on Windows 7 - looks good.

Bug 839445's test covers this bug too.

This is a possible candidate for ESR17 uplift as it fixed bug 831757 (CtP notification vanishes when using pushState()/replaceState() navigation) and other CtP notification issues.

I would rather not uplift this change. If bug 831757 really needs to be addressed for ESR-17, we can devise a more narrowly-scoped fix.

How much narrower can you get than this 4K patch?

Since when is the size of the patch indicative of potential impact? :) The patch as-is affects all popup notifications; if the click-to-play notification was the one we cared about specifically I figured we could try to address that problem more specifically. Thinking about it more, there probably isn't a good way to do that. This patch alone (without bug 839516/bug 788584/bug 839445 which I had kind of conflated this with) is probably fine for uplift to ESR, I guess.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #38) > The patch as-is affects all popup notifications; if the click-to-play > notification was the one we cared about specifically I figured we could try > to address that problem more specifically. Thinking about it more, there > probably isn't a good way to do that. At least from what i've looked at this is a notification problem and not specific to the click-to-play notification. Do you think it would be worth revisiting the patch on bug 831757 as an alternative?

(In reply to Georg Fritzsche [:gfritzsche] from comment #39) > At least from what i've looked at this is a notification problem and not > specific to the click-to-play notification. Yes, I know. But if the driver for an ESR patch is a particular notification, we could potentially come up with an ESR-only fix that is isolated to that notification; that has a lower risk of negatively impacting other notifications (which is why I was hesitating to uplift this patch as-is to ESR). But as I mentioned, there's probably not a good way to do that. > Do you think it would be worth revisiting the patch on bug 831757 as an > alternative? That patch just looks like an (incomplete) combination of the patches for this bug and the other bugs mentioned in comment 38, so I don't see how that would be useful.

If we need to address variants of this bug (like bug 831757) on ESR, we should do it with this patch. I'm just not really clear on the drivers for addressing this bug for ESR17.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #41) > If we need to address variants of this bug (like bug 831757) on ESR, we > should do it with this patch. I'm just not really clear on the drivers for > addressing this bug for ESR17. Click-to-play blocks are in use on ESR17. As this was accelerated, fixes to the click-to-play have been mostly uplifted through the trains and to ESR17.

Yep, we want this fix since bug 831757 will be a painpoint when we choose to use CTP on ESR17.

[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: see comment 43 User impact if declined: see comment 43 Fix Landed on Version: 21, uplifted to 20 Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.