821073 - full screen support for video chats

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

need to look into supporting moving chat windows into fullscreen mode

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: exploratory patch

this is just a quick hack patch to move into full screen if you're not a browser tab, lots of stuff still doesn't work with a video tag in a chat window.

Comment 2

[Jason Smith [:jsmith]]

Not directly related to the determination for if we ship webrtc or not.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: fullscreen patch

full screen patch allows content in chat windows to go full screen.  relies on css for full screen mode.  also replaces previous work on hiding social when browser tabs go full screen by relying on css, resulting in a better fullscreen transition.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 755005 [details] [diff] [review]
fullscreen patch

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

::: browser/base/content/browser-fullScreen.js
@@ +107,5 @@
>      // However, if we receive a "MozEnteredDomFullScreen" event for a document
>      // which is not a subdocument of the currently selected tab, we know that
>      // we've switched tabs since the request to enter full-screen was made,
>      // so we should exit full-screen since the "full-screen document" isn't
>      // acutally visible.

just need to update this comment with something like
s/which is not a subdocument of the currently selected tab/which is not active/

@@ +110,5 @@
>      // so we should exit full-screen since the "full-screen document" isn't
>      // acutally visible.
> +    if (!event.target.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIWebNavigation)
> +                                 .QueryInterface(Ci.nsIDocShell).isActive) {

I wonder here if a subdocument can be QI'ed to webNav/docShell and isActive works, or if you'll need to use .top

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Felipe Gomes from comment #4)
> >      // so we should exit full-screen since the "full-screen document" isn't
> >      // acutally visible.
> > +    if (!event.target.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                 .getInterface(Ci.nsIWebNavigation)
> > +                                 .QueryInterface(Ci.nsIDocShell).isActive) {
> 
> I wonder here if a subdocument can be QI'ed to webNav/docShell and isActive
> works, or if you'll need to use .top

isActive state is propagated from parent to child docshells: http://hg.mozilla.org/mozilla-central/annotate/4755d50e2402/docshell/base/nsDocShell.cpp#l5236

Comment 6

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=69339a2292b4

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: fullscreen patch

patch with fixed comment

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

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d70b4807df13

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 757014 [details] [diff] [review]
fullscreen patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): chat windows
User impact if declined: webrtc video chats do not work well in the small confines of the text chat window, video is basically unusable at that size.  this allows for video calls to go full screen
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been on central for a couple of weeks but adding verifyme to make sure this works as expected before we get to Beta on Monday.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43bf22a00669

Comment 12

[Samvedana (:Samvedana)]

I have tested this using demo provider from http://mixedpuppy.github.io/socialapi-demo/
on Windows 8.

User Agent:Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
user Id ID: 20130625125232

Video works in full screen.

Is it sufficient to verify using the video embedded in the chat panel of the demo provider?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Samvedana from comment #12)
> I have tested this using demo provider from
> http://mixedpuppy.github.io/socialapi-demo/
> on Windows 8.
> 
> User Agent:Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20100101
> Firefox/23.0
> user Id ID: 20130625125232
> 
> Video works in full screen.
> 
> Is it sufficient to verify using the video embedded in the chat panel of the
> demo provider?

For this bug, yes.  fullscreen for tear-off chat windows (bug 880911) is slightly different testing but can also be tested with the demo provider.

Comment 14

[Samvedana (:Samvedana)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> (In reply to Samvedana from comment #12)
> > I have tested this using demo provider from
> > http://mixedpuppy.github.io/socialapi-demo/
> > on Windows 8.
> > 
> > User Agent:Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20100101
> > Firefox/23.0
> > user Id ID: 20130625125232
> > 
> > Video works in full screen.
> > 
> > Is it sufficient to verify using the video embedded in the chat panel of the
> > demo provider?
> 
> For this bug, yes.  fullscreen for tear-off chat windows (bug 880911) is
> slightly different testing but can also be tested with the demo provider.

Thanks Shane!

Comment 15

[u279076]

Samvedana, can you please retest this against Firefox 24?

Comment 16

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20130723 Firefox/24.0
Build ID: 20130723004004

I have tested this using demo provider from http://mixedpuppy.github.io/socialapi-demo/
on Windows 8.

Video works in full screen.