Philipp Hancke Reporter
	Description • 10 years ago

steps to reproduce:
1) navigate to https://fippo.github.io/webrtc-landing/gum_test_fippo3.html
2) share a window (queries camera first)
3) navigate to https://talky.io, join a room
4) click "share screen"
5) note the "GUM Test Page - Nightly" in the "Window or screen to share:" list. This is from step 1 and not reflecting the current page title.

This looks like the list is generated during step 1 and cached.

	Randell Jesup [:jesup] (needinfo me)
	Comment 1 • 10 years ago

The full list isn't cached (open a new window after 3 and before 4, and you'll see it in the list), but it appears it doesn't update the titles of existing windows when it Rescan()s.

	Maire Reavy [:mreavy]
	Comment 2 • 10 years ago

Roman, Gian-Carlo, or Matt -- Would any of your feel comfortable taking this bug?  It's a follow on to the rescan bug (Bug 1041369) which we uplifted to Fx33.  I'd love to get this fixed within the next week (or sooner) and uplifted before we go to Beta, if possible.

	Matthew Miller [:m_and_m] Assignee
	Comment 3 • 10 years ago

I can take a look at it next week.

	Maire Reavy [:mreavy]
	Comment 4 • 10 years ago

Thanks, Matt!

	Randell Jesup [:jesup] (needinfo me)
	Comment 6 • 10 years ago

Comment on attachment 8480140 [details] [diff] [review]
Window title in "windows or screen to share" list is not updated when navigating to another page

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +703,5 @@
> +void MediaEngineWebRTCVideoSource::Refresh(int aIndex) {
> +#ifdef MOZ_B2G_CAMERA
> +  mCaptureIndex = aIndex;
> +#else
> +  mCaptureIndex = aIndex;

a) hoist this outside the #ifdef
b) would this actually ever be different?  If it can't, then remove the parameter; if you're worried it might but shouldn't, MOZ_ASSERT or NS_WARN_IF

@@ +705,5 @@
> +  mCaptureIndex = aIndex;
> +#else
> +  mCaptureIndex = aIndex;
> +
> +  // assume the uniqueId hasn't changed ...

"Caller ensures this is the same UniqueId" or "Caller looked up this captureIndex by uniqueID, so it shouldn't change"

	Matthew Miller [:m_and_m] Assignee
	Comment 7 • 10 years ago

(In reply to Randell Jesup [:jesup] from comment #6)
> Comment on attachment 8480140 [details] [diff] [review]
> Window title in "windows or screen to share" list is not updated when
> navigating to another page
> 
> Review of attachment 8480140 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +703,5 @@
> > +void MediaEngineWebRTCVideoSource::Refresh(int aIndex) {
> > +#ifdef MOZ_B2G_CAMERA
> > +  mCaptureIndex = aIndex;
> > +#else
> > +  mCaptureIndex = aIndex;
> 
> a) hoist this outside the #ifdef
> b) would this actually ever be different?  If it can't, then remove the
> parameter; if you're worried it might but shouldn't, MOZ_ASSERT or NS_WARN_IF
> 

The index actually does change, at least for Mac when enumerating windows.

	Matthew Miller [:m_and_m] Assignee
	Comment 9 • 10 years ago

The previous patch had a stupid typo (#ifndef vs. #ifdef) that I thought hadn't been committed, but was.

My apologies for the noise /-:

	Randell Jesup [:jesup] (needinfo me)
	Comment 10 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef2bf9308ed7

	Randell Jesup [:jesup] (needinfo me)
	Comment 11 • 10 years ago

Backed out for various M3 failures (semi-random, UAF or leaving something running/leaked).
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ef2bf9308ed7

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb2483f4928

	Matthew Miller [:m_and_m] Assignee
	Comment 12 • 10 years ago

Patch to address Mochitest-3 failures.  mCaptureIndex is changed when a device is allocated, meaning the enumeration index can be different.  However, the enumeration index is what is used to get the current device name.

Try of the interdiff: < https://tbpl.mozilla.org/?tree=Try&rev=3e0291fd3379 >
Failures in that run appear to be unrelated...

	Randell Jesup [:jesup] (needinfo me)
	Comment 13 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9024c845a9

	Randell Jesup [:jesup] (needinfo me)
	Comment 14 • 10 years ago

Comment on attachment 8481881 [details] [diff] [review]
v4 - Window title in "windows or screen to share" list is not updated when navigating to another page

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: Wrong (old) window title shown

[Describe test coverage new/current, TBPL]: manual testing.  Green on inbound, works in manual testing.  Easily tested by hand

[Risks and why]: fairly simple patch.  Causes use confusion.

[String/UUID change made/needed]: none

	Phil Ringnalda (:philor)
	Comment 15 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8c9024c845a9

	Randell Jesup [:jesup] (needinfo me)
	Comment 16 • 10 years ago

DOn't uplift until bug 1061475 is fiixed and included, please.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd1c98e99569

	Bogdan Maris, Desktop QA
	Comment 18 • 10 years ago

Reproduced the initial issue on old Nightly (2014-08-20), verified that the issue is fixed on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.9.4 using latest Aurora and Firefox 33 beta 2.