Closed Bug 1056213 Opened 5 years ago Closed 5 years ago

Window title in "windows or screen to share" list is not updated when navigating to another page

Categories

(Core :: WebRTC: Audio/Video, defect)

34 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox33 --- verified
firefox34 --- verified

People

(Reporter: philipp+bugzilla, Assigned: m_and_m)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Whiteboard: [screensharing-uplift]
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.
Flags: needinfo?(rskalish)
Flags: needinfo?(linuxwolf)
Flags: needinfo?(gpascutto)
I can take a look at it next week.
Flags: needinfo?(rskalish)
Flags: needinfo?(linuxwolf)
Flags: needinfo?(gpascutto)
Thanks, Matt!
Assignee: nobody → linuxwolf
Attachment #8480140 - Flags: review?(rjesup)
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"
Attachment #8480140 - Flags: review?(rjesup) → review+
(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.
Attachment #8480140 - Attachment is obsolete: true
Attachment #8480175 - Flags: review?(rjesup) → review+
The previous patch had a stupid typo (#ifndef vs. #ifdef) that I thought hadn't been committed, but was.

My apologies for the noise /-:
Attachment #8480175 - Attachment is obsolete: true
Attachment #8480228 - Flags: review?(rjesup)
Attachment #8480228 - Flags: review?(rjesup) → review+
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...
Attachment #8480228 - Attachment is obsolete: true
Attachment #8481881 - Flags: review?(rjesup)
Attachment #8481881 - Flags: review?(rjesup) → review+
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
Attachment #8481881 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8c9024c845a9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8481881 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
DOn't uplift until bug 1061475 is fiixed and included, please.
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Whiteboard: [screensharing-uplift]
You need to log in before you can comment on or make changes to this bug.