575195 - Restart after closing app from fullscreen mode does not display fullscreen UI

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jim Mathies [:jimm]]

In 3.6.4, this bug manifests normal full sized window on the desktop. In the recent titlebar work we did, we fixed this so we actually go into full screen mode on restart. Widget sends a size mode event, but for some reason the browser down't pick up on it, so the UI is the normal browser.

STR:

1) open
2) F11, go to fullscreen
3) close
4) restart

To get out of this weird normal browser/fullscreen mode, hit F11 again.

Comment 1

[Jim Mathies [:jimm]]

Attachment: widget patch

Comment 2

[Jim Mathies [:jimm]]

Attachment: appshell patch

Comment 3

[Jim Mathies [:jimm]]

Attachment: dom patch

Comment 4

[Jim Mathies [:jimm]]

Attachment: browser patch

Comment 5

[Jim Mathies [:jimm]]

These patches fix a few bugs and improve one issue related to full screen.

Xul windows persist size modes, so when the user is in full screen mode and opens a new window, or if they shut down in fs and relaunch, the new window should be in fs mode. In 3.6 this doesn't work. On windows what you will get is a normal window that is sized to the size of the desktop. This same problem *was* in m-c builds until a patch for the new titlebar landed this weekend fixing the "big window" problem.

This didn't fix things entirely though - widget currently isn't sending a size mode event when we enter full screen mode [bug #1, widget patch]. nsXULWindow also doesn't propagate fs size mode events to the dom via calls to global window [bug #2, xpfe + dom patches]. So there's no connection on startup between widget's size mode changes and browser.js. Also, browser.js isn't hooked up the "fullscreen" event soon enough [bug #3, browser patch] to receive it on initial startup.

The last change, which is in the xpfe patch, makes it so browser windows with persisted fs mode opened on top of existing full screen windows open normally, which is a standard windows convention.

Comment 6

[[e-mail]]

Comment on attachment 455080 [details] [diff] [review]
appshell patch

>+#ifdef XP_WIN
Any reason this is Windows-specific?

>+  // If the parent is currently fullscreen, tell the child to ignore persisted
>+  // full screen states. This way new browser windows open on top of fullscreen
>+  // windows normally.
What if some other window opens a browser window?

>+      reinterpret_cast<nsXULWindow*>(window.get())->IgnoreXULSizeMode(PR_TRUE);
Doesn't nsWebShellWindow derive from nsXULWindow?

Comment 7

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

Comment on attachment 455081 [details] [diff] [review]
dom patch

This looks like something which should happen in widget level.
MakeFullScreen should just do the right thing, no?

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #6)
> (From update of attachment 455080 [details] [diff] [review])
> >+#ifdef XP_WIN
> Any reason this is Windows-specific?
> 
> >+  // If the parent is currently fullscreen, tell the child to ignore persisted
> >+  // full screen states. This way new browser windows open on top of fullscreen
> >+  // windows normally.
> What if some other window opens a browser window?

It works great, if you have a full screen browser and open a new with ctrl-n, it opens normal. Prior to this patch it would open full screen on top of the first full screen window.

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #7)
> (From update of attachment 455081 [details] [diff] [review])
> This looks like something which should happen in widget level.
> MakeFullScreen should just do the right thing, no?

There's circular call risk here, I put this in to protect against that on other platforms. win32 widget handles this. So if we're sure

 widget -> send size mode to web shell 
 web shell -> global window -> fullscreen
 global window -> widget -> makefullscreen

Is safe, we don't need it.

Comment 10

[Jim Mathies [:jimm]]

Attachment: appshell patch v.2

- nix over zealous castings

Comment 11

[[e-mail]]

(In reply to comment #8)
>(In reply to comment #6)
>>(From update of attachment 455080 [details] [diff] [review] [details])
>>>+#ifdef XP_WIN
>>Any reason this is Windows-specific?
No answer to this question?

>>>+  // If the parent is currently fullscreen, tell the child to ignore persisted
>>>+  // full screen states. This way new browser windows open on top of fullscreen
>>>+  // windows normally.
>>What if some other window opens a browser window?
>It works great, if you have a full screen browser and open a new with ctrl-n
No, I said if some *other* window opens a browser window...

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #11)
> (In reply to comment #8)
> >(In reply to comment #6)
> >>(From update of attachment 455080 [details] [diff] [review] [details] [details])
> >>>+#ifdef XP_WIN
> >>Any reason this is Windows-specific?
> No answer to this question?

Sorry, missed that. Actually I was kind of hoping that would get fleshed out through reviews or by asking around to see what the convention is on other platforms. 

> 
> >>>+  // If the parent is currently fullscreen, tell the child to ignore persisted
> >>>+  // full screen states. This way new browser windows open on top of fullscreen
> >>>+  // windows normally.
> >>What if some other window opens a browser window?
> >It works great, if you have a full screen browser and open a new with ctrl-n
> No, I said if some *other* window opens a browser window...

Hmm, good question. Bookmarks comes to mind. I'll test and see. Maybe I should  walk across a list of all open top level windows looking for one that is full screen as a test.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 455179 [details] [diff] [review]
appshell patch v.2

I can improve on this slightly, but it'll likely be after the summit before I post a new rev.

Comment 14

[Jim Mathies [:jimm]]

Attachment: appshell patch v.3

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 457505 [details] [diff] [review]
appshell patch v.3

Updated to walk the list of open windows.

Comment 16

[Benjamin Smedberg]

I didn't realize the resulting window was unusable, we need to at least restore with a usable window.

Comment 17

[[e-mail]]

Comment on attachment 457505 [details] [diff] [review]
appshell patch v.3

>+  PRBool more;
>+  do {
>+    windowList->HasMoreElements(&more);
>+    if (!more)
>+      return PR_FALSE;
This ...

>+      nsIWidget* widget = nsnull;
nsCOMPtr should work on nsIWidget, no? Saves manually releasing.

>+  } while (more);
... means that this is always true. So you can just write
for (;;) {
  PRBool more = PR_FALSE;
etc.

>+    window.get()->IgnoreXULSizeMode(PR_TRUE);
Nit: .get() shouldn't be necessary

I should be able to give this a whirl soon now that I've got a working build again.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 455082 [details] [diff] [review]
browser patch

The event callback needs the height of the toolbox, which can change during delayedStartup.

Comment 19

[Dão Gottwald [:dao]]

Attachment: browser patch

Comment 20

[Jim Mathies [:jimm]]

gavin, ping?

Comment 21

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

Comment on attachment 460219 [details] [diff] [review]
browser patch

enterFS is a bit confusing, maybe use "inFullscreen"? I know it's not exactly accurate in the transition case, but I don't really think that's a problem.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> Comment on attachment 460219 [details] [diff] [review]
> browser patch
> 
> enterFS is a bit confusing, maybe use "inFullscreen"? I know it's not exactly
> accurate in the transition case, but I don't really think that's a problem.

What's confusing, "enter" or "FS"? I actually care about the accuracy, as the vague semantics of just "fullScreen" confused me when I wrote the patch.

Comment 23

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

"enterFS" seems to describe the transition as opposed to the end-state that the controls are being set up for, which I thought was kind of odd. But I don't feel strongly - just leave it as is if you want.

Comment 24

[Brendan Eich [:brendan]]

State names are traditionally gerunds or participles/adjectives (consider TCP closing and closed), not imperative-mood verbs like "enter". Sorry to be a nosy bystander but I'm wondering whether entering or entered is best. What's really going on here?

For the ending, -FullScreen instead of -FS seems winning, but maybe the whole name is then too long.

/be

Comment 25

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

The function runs both before the transition (in the normal "toggle full screen for a given window" case) or after (in the case where a window is restored as a  fullscreen window on startup), so entered/entering can both be wrong for a given invocation. That's why I suggested "inFullScreen" - describes the final state that we're preparing the UI for.

Comment 26

[Brendan Eich [:brendan]]

The function runs in two different states. Does the distinction really not matter?

/be

Comment 27

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

I'm not sure what you're asking. It either updates the UI to match the current state in the post-transition invocation, or updates the UI to match the future state in the pre-transition invocation. In both cases the state it's using to update the UI is reflected by the boolean.

Comment 28

[Brendan Eich [:brendan]]

current state : post-transition invocation :: future state : pre-transition

It all makes sense but it's hard to name something that has two "tenses" :-|.

/be

Comment 30

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/bb9d266da12c
http://hg.mozilla.org/mozilla-central/rev/0b6d45b6c2e6
http://hg.mozilla.org/mozilla-central/rev/a179e301dbfd