574778 - Enabling Full Screen Video is broken

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(mostly gone) XtC4UaLL [:xtc4uall]]

STR:
* open URL
* hit the Play Button
* right click in the Video + "Full Screen"

Expected:
Watching the Video in Full Screen Mode.

Actual:
* You get hit by Bug 574690.
* the Video is played in a small Content Area at the top left.

Using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a6pre) Gecko/20100625 Minefield/3.7a6pre

Range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=522df66198cf&tochange=51bd519736c4

AFIK, this isn't yet covered by other filed Regression Reports, no?

Comment 1

[[not reading bugmail]]

Did you test with turning off OOPP,  It maybe helpful to see if there any connected code path there.

Comment 2

[Jim Mathies [:jimm]]

(In reply to comment #1)
> Did you test with turning off OOPP,  It maybe helpful to see if there any
> connected code path there.

This video is playing in mozilla. no oopp!

Comment 3

[[not reading bugmail]]

(In reply to comment #2)
> (In reply to comment #1)
> > Did you test with turning off OOPP,  It maybe helpful to see if there any
> > connected code path there.
> 
> This video is playing in mozilla. no oopp!

Oh, duh, I missed that link to the video.. good thing I didn't ask where was either, thanks.

Comment 4

[Bas Schouten (:bas.schouten)]

Fixed by fix http://hg.mozilla.org/mozilla-central/rev/7ad36412775e to bug 574690.

Comment 5

[(mostly gone) XtC4UaLL [:xtc4uall]]

If i understand correctly, Bug 574690 got (partially?) backed-out and thus this Issue regressed again. Will this get fixed by Bug 579421 again?

Comment 6

[[not reading bugmail]]

(In reply to comment #5)
> If i understand correctly, Bug 574690 got (partially?) backed-out and thus this
> Issue regressed again. Will this get fixed by Bug 579421 again?

Confirming I see it regressed with 7-16 nightly pointing to the backout; 7-15 was fine even after Retain Layers landed.

Comment 7

[Jim Mathies [:jimm]]

Attachment: fix v.1

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 458413 [details] [diff] [review]
fix v.1

Now that we have base windows that are chromed os windows, we need to touch up things like this. nsXULWindow will resize the window after it loads unless intrinsic resizing is disabled. Since we create the video dialog, then set it to fullscreen before it finishes loading, we need to disable this.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 458413 [details] [diff] [review]
fix v.1

>+  // Prevent chrome documents which are still loading from resizing
>+  // the window after we set fullscreen mode.
>+  nsCOMPtr<nsIBaseWindow> treeOwnerAsWin;
>+  GetTreeOwner(getter_AddRefs(treeOwnerAsWin));
>+  nsCOMPtr<nsIXULWindow> xulWin(do_GetInterface(treeOwnerAsWin));
>+  if (xulWin) {
>+    xulWin->SetIntrinsicallySized(PR_FALSE);
>+  }
>+
Should this happen only if aFullScreen is PR_TRUE?

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #9)
> Comment on attachment 458413 [details] [diff] [review]
> fix v.1
> 
> >+  // Prevent chrome documents which are still loading from resizing
> >+  // the window after we set fullscreen mode.
> >+  nsCOMPtr<nsIBaseWindow> treeOwnerAsWin;
> >+  GetTreeOwner(getter_AddRefs(treeOwnerAsWin));
> >+  nsCOMPtr<nsIXULWindow> xulWin(do_GetInterface(treeOwnerAsWin));
> >+  if (xulWin) {
> >+    xulWin->SetIntrinsicallySized(PR_FALSE);
> >+  }
> >+
> Should this happen only if aFullScreen is PR_TRUE?

Sure, I'll add:

if (aFullScreen && xulWin) {

I doubt the reverse situation ever happens, but better safe than sorry.

Comment 11

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/4291098c5736

Thanks for the quick turn around on reviews smaug!

Comment 12

[[not reading bugmail]]

Jim, looks like we didn't quite make it into full screen.. it looks maximized instead.. my windows taskbar is still there and there is not a close X in the upper right, I don't get a context menu, or what have you.

Comment 13

[Jim Mathies [:jimm]]

Attachment: ConstrainPosition fix v.1

ConstrainPosition in widget has a bug where we constrain full screen windows to the work area. This updates that. (ConstrainPosition is really only called from a couple places in nsXULWindow, for centering and for initial positioning.)

Comment 14

[Rob Arnold [:robarnold]]

Comment on attachment 458550 [details] [diff] [review]
ConstrainPosition fix v.1

Well this looks right to me. Can't help but feel like the last chunk is dead code.

Comment 15

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/274a672f9acf