Closed Bug 1159659 Opened 9 years ago Closed 9 years ago

Allow browser sharing gUM requests on Windows XP and OSX 10.6

Categories

(Core :: WebRTC, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- verified

People

(Reporter: mikedeboer, Assigned: mreavy)

References

Details

Attachments

(1 file, 1 obsolete file)

Randell mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1146921#c24 that modifications in `MediaManager::GetUserMedia` are necessary to allow browser sharing on older platforms.

Sharing of type 'browser' is possible on older platforms, whereas screen and window sharing are not.

This feature is available in Loop/ Hello, so it'd be nice for our users on those older platforms to be able to have one sharing option available.e
Flags: qe-verify+
QA Contact: bogdan.maris
Sounds like this is a matter of putting the OSX 10.6/Windows XP check outside of the "dom::MediaSourceEnum::Browser" case in MediaManager::GetUserMedia.

That kind of change seems containable for 38.0.5, I think, assuming no other related issues? Maire, do you agree, and if so can someone on your team pick this up on that timeline?
Flags: needinfo?(mreavy)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #1)
> Sounds like this is a matter of putting the OSX 10.6/Windows XP check
> outside of the "dom::MediaSourceEnum::Browser" case in
> MediaManager::GetUserMedia.
> 
> That kind of change seems containable for 38.0.5, I think, assuming no other
> related issues? Maire, do you agree, and if so can someone on your team pick
> this up on that timeline?

From a technical perspective, this doable for 38.0.5.  We could have a patch up for review by the end of this week.  (It should be a small patch -- almost a one-liner.)

From a non-technical perspective, it may be confusing to those users that they can do one and only one type of sharing (that would need to be explained somewhere), and I don't know how important it is to invest time and energy supporting those older platforms (and their limitations).  Further, after the patch landed, we'd need someone from QA to manually verify that sharing is working as expected and that the performance is reasonable on those platforms. NOTE: This feature would be available to all WebRTC applications on XP and OSX 10.6 (not just Hello).

Gavin and I chatted in irc, and he said we're going to need either a patch for Platform to allow tab sharing or a patch for the client to hide it.

Since a platform patch should be small and quick, I'll start the ball rolling on getting one up for review.  In the meantime, I'm doing a needinfo to RT to ask if he has a strong opinion either way to enable tab sharing for XP/OSX 10.6 (which includes QA support and testing for tab sharing on those older platforms) or simply hide it for those platforms.
Flags: needinfo?(mreavy) → needinfo?(rtestard)
> 
> From a non-technical perspective, it may be confusing to those users that
> they can do one and only one type of sharing (that would need to be
> explained somewhere),

Agreed we need the SUMO page to clarify that (currently the draft says that tab sharing only is allowed on XP).
I cc Joni on that bug so she's aware of where we end-up WRT tab sharing availability in XP.

 and I don't know how important it is to invest time
> and energy supporting those older platforms (and their limitations). 

Per netmarketshare XP is 19% and per statcounter it is 12% of all desktop traffic so the truth is probably somewhere in-between - it's probably significant enough.
Also marketing is considering a snippet campaign to promote the use of sharing (I am trying to understand if snippets could only target non XP users) and having a bug for 12 to 19% of the user base would become apparent.

> Further, after the patch landed, we'd need someone from QA to manually
> verify that sharing is working as expected and that the performance is
> reasonable on those platforms. NOTE: This feature would be available to all
> WebRTC applications on XP and OSX 10.6 (not just Hello).
> 
> Gavin and I chatted in irc, and he said we're going to need either a patch
> for Platform to allow tab sharing or a patch for the client to hide it.
> 
> Since a platform patch should be small and quick, I'll start the ball
> rolling on getting one up for review.  In the meantime, I'm doing a needinfo
> to RT to ask if he has a strong opinion either way to enable tab sharing for
> XP/OSX 10.6 (which includes QA support and testing for tab sharing on those
> older platforms) or simply hide it for those platforms.

I'd much rather we enable tab sharing on XP machines if the performance is acceptable given the market share of XP and the fact that it's our first step towards a product which will be mostly about sharing.
Flags: needinfo?(rtestard)
Assignee: nobody → mreavy
Status: NEW → ASSIGNED
Comment on attachment 8600085 [details] [diff] [review]
Allow tab sharing on XP and OSX 10.6

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

Pkerr, Jib -- I'll take the first review I can get.  We want to enable tab sharing for XP and OSX 10.6. Loop needs it for Fx 38.0.5. There are no automated tests in Loop for tab sharing that I could find -- so I'm going to be testing this manually on an OSX 10.6 machine that I have and asking the reporter to test on his XP machine.  I'll also file a bug to add automated tests for tab sharing.

Feel free to ping me on irc if you have any questions.  Thanks!
Attachment #8600085 - Flags: review?(pkerr)
Attachment #8600085 - Flags: review?(jib)
Attachment #8600085 - Flags: review?(pkerr) → review+
Attachment #8600085 - Flags: review?(jib)
Comment on attachment 8600085 [details] [diff] [review]
Allow tab sharing on XP and OSX 10.6

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

::: dom/media/MediaManager.cpp
@@ +1850,5 @@
>                                  false) ||
>  #if defined(XP_MACOSX) || defined(XP_WIN)
>            (
> +           // Allow tab sharing for all platforms including XP and OSX 10.6
> +           (src != dom::MediaSourceEnum::Browser) && 

nits: need one more character indent on these two, parens are not needed, and trailing whitespace.
Attachment #8600085 - Attachment is obsolete: true
Comment on attachment 8600288 [details] [diff] [review]
Allow tab sharing on XP and OSX 10.6

Carry forward r=pkerr.  Addressed nits from jib (kept parentheses because I felt they made the change clearer).
Attachment #8600288 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=56af0cc4fdd0

Tested manually and verified to work on OSX 10.6 with e10s disabled (tab sharing in Loop won't work on any platform with e10s enabled). I'll be asking the reporter to test on XP.
Whiteboard: checkin-needed
Bogdan -- Do you have an XP machine?  If so, can you please test this patch for me on XP?  I need to verify that tab sharing works -- specifically on XP.  I don't believe there are manual automated tests for tab sharing in Hello, so manually testing for this is very important. Thanks!
Flags: needinfo?(bogdan.maris)
Filed Bug 1160482 for adding automated tests for Hello tab sharing.
See Also: → 1160482
Note that checkin-needed is a bug keyword (and bug marking tools Just Work when it's used) :)
Whiteboard: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/950723ac69a4
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
I confirm that tab sharing works as expected on Windows XP SP3 32-bit and Windows XP SP2 64-bit using latest Nightly and the try build from comment 9.
Flags: needinfo?(bogdan.maris)
Also works on Mac OS X 10.6.8.
Status: RESOLVED → VERIFIED
Comment on attachment 8600288 [details] [diff] [review]
Allow tab sharing on XP and OSX 10.6

Approval Request Comment
[Feature/regressing bug #]: Hello tab sharing for Win XP and OSX 10.6.  We need to get this into 38.0.5.
[User impact if declined]: Tab sharing for XP and Mac 10.6.x users would appear to be available but would not work for those users
[Describe test coverage new/current, TreeHerder]: Testing coverage is currently manual.  Manually tested by me on OSX 10.6 and manually tested and verified by QA to work on XP and OSX 10.6
[Risks and why]: Risk is very, very low to XP and OSX 10.6 users, and there is no risk to other users.  This is a one-line patch to turn on the tab sharing pref for XP and 10.6 users so that they don't have to go into about:config and flip the pref themselves.  If we don't take this patch, then someone on the Hello client team will have to create and uplift a Hello UI patch to hide the tab sharing feature for XP and 10.6 users.  Likely that patch will be more complicated and riskier than this one AND Product prefers to enable tab sharing for XP and 10.6 users since sharing will be highlighted as a big product feature for Hello.
[String/UUID change made/needed]: No strings
Attachment #8600288 - Flags: approval-mozilla-release?
Attachment #8600288 - Flags: approval-mozilla-beta?
Attachment #8600288 - Flags: approval-mozilla-aurora?
Maire, there is still some discussion about what will happen with 38.0.5 but I think we can go ahead and take this patch for now.
Comment on attachment 8600288 [details] [diff] [review]
Allow tab sharing on XP and OSX 10.6

Approved for uplift to aurora, and to the beta channel which is 38.0.5.   But please don't uplift it to release/38.
Attachment #8600288 - Flags: approval-mozilla-release?
Attachment #8600288 - Flags: approval-mozilla-release-
Attachment #8600288 - Flags: approval-mozilla-beta?
Attachment #8600288 - Flags: approval-mozilla-beta+
Attachment #8600288 - Flags: approval-mozilla-aurora?
Attachment #8600288 - Flags: approval-mozilla-aurora+
FYI, 38.0.5 merged to release on Monday. At this point, beta's a dead branch until 39 merges to it next week.
Flags: needinfo?(lhenry)
Augh. Ryan I can't believe I got that backwards. I'm glad you knew what I meant.
Flags: needinfo?(lhenry)
Verified fixed on 38.0.5 beta 2 (Build ID: 20150514163436), across the following platforms: Mac OS X 10.6.8 and Windows XP - SP2 64 bit and SP3 32 bit.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.