742178 - [10.7] MakeFullScreen calls do nothing if in transition

Status: RESOLVED INCOMPLETE

(Core :: Widget: Cocoa, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

This came up in bug 741864. There's a test case there which at the end, should result in the window exiting full screen mode.

The details: We don't set mFullScreen synchronously (we wait for OS X to tell us the window is fullscreen), but the navigation happens meanwhile and we call MakeFullScreen(false) & mFullScreen is already false so we just bail.

We're now tracking if we're in transition, so we can take advantage of that. If we get a call to MakeFullScreen while transitioning, we can reissue that call when the transition finishes. A quick test shows calling toggleFullScreen again doesn't work, we just print "not in fullscreen state" (not us, coming from OS X). Otherwise it doesn't seem like there's anything to call to cancel the transition.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

This is working, but I may need to address some edge cases (hence the XXX comments).

Comment 2

[Alex Keybl [:akeybl]]

Tracking for FF14 since 10.7 FS is a new feature and this bug may cause an unexpected state.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

For this and bug 740923 I'm going to do a test build with logging on all the fullscreen activity.  I'll play with this and see what I turn up.

It's time I learned this code anyway :-)

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Re-assigning to Steven in response to comment 3.

Comment 5

[Alex Keybl [:akeybl]]

This appears to be an issue in our test cases, but it's not clear to me that it's in a common user scenario. Can somebody provide clarity here?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #5)

I frankly don't know.  Paul?

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 612396 [details] [diff] [review]
Patch v0.1 (WIP)


 NS_METHOD nsCocoaWindow::MakeFullScreen(bool aFullScreen)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
 
+  if (mInFullScreenTransition) {

Here I'd add the following comment:
We're already in a fullscreen transition. If we're transitioning to the state that this request wants us to transition to, do nothing. If we're transitioning away from the requested state, we need to remember to transition back after the transition has completed; Cocoa doesn't give us an API to stop a fullscreen transition mid-way, so we need to wait for it to complete first.

(The last sentence is the most important one; I didn't get that at first.)

+    // We need to do this as a runnable because calls to toggleFullScreen fail
+    // while windowDid(Enter|Exit)FullScreen is on the stack.
+    nsCOMPtr<nsIRunnable> ev = NS_NewRunnableMethod(this, &nsCocoaWindow::CancelFullScreen);
+    NS_DispatchToCurrentThread(ev);

What happens if the nsCocoaWindow instance is destroyed before the event fires? Or does the event keep it alive?
Also, what happens if another MakeFullScreen call is issued before the event fires?

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to Alex Keybl [:akeybl] from comment #5)
> This appears to be an issue in our test cases, but it's not clear to me that
> it's in a common user scenario. Can somebody provide clarity here?

No it's not a common user scenario. The only way to really get here (afaik) is for a web page to request fullscreen and then immediately navigate. Given how rarely web pages request fullscreen it should be quite rare (not a showstopper)

(In reply to Markus Stange from comment #7)
> +    // We need to do this as a runnable because calls to toggleFullScreen
> fail
> +    // while windowDid(Enter|Exit)FullScreen is on the stack.
> +    nsCOMPtr<nsIRunnable> ev = NS_NewRunnableMethod(this,
> &nsCocoaWindow::CancelFullScreen);
> +    NS_DispatchToCurrentThread(ev);
> 
> What happens if the nsCocoaWindow instance is destroyed before the event
> fires? Or does the event keep it alive?

That's a good question. I'm not sure. Chris, since you used nsIRunnable in DOM FS, do you know?

> Also, what happens if another MakeFullScreen call is issued before the event
> fires?

I think that's ok since we check mInFullScreenTransition in CancelFullScreen. It gets set to false, and it would get set back to false if MakeFullScreen is called before Cancel gets dispatched.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Paul O'Shannessy [:zpao] from comment #8)
> (In reply to Markus Stange from comment #7)
> > +    nsCOMPtr<nsIRunnable> ev = NS_NewRunnableMethod(this,
> > &nsCocoaWindow::CancelFullScreen);
> > +    NS_DispatchToCurrentThread(ev);
> > 
> > What happens if the nsCocoaWindow instance is destroyed before the event
> > fires? Or does the event keep it alive?
> 
> That's a good question. I'm not sure. Chris, since you used nsIRunnable in
> DOM FS, do you know?

RunnableMethod keeps a reference to its target, so the runnable will keep nsCocoaWindow alive (though it won't necessarily keep it sane).

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Paul O'Shannessy [:zpao] (no longer moco, slower to respond) from comment #8)
> (In reply to Alex Keybl [:akeybl] from comment #5)
> > This appears to be an issue in our test cases, but it's not clear to me that
> > it's in a common user scenario. Can somebody provide clarity here?
> 
> No it's not a common user scenario. The only way to really get here (afaik)
> is for a web page to request fullscreen and then immediately navigate. Given
> how rarely web pages request fullscreen it should be quite rare (not a
> showstopper)

Thanks Paul. Untracking in that case.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 612396 [details] [diff] [review]
Patch v0.1 (WIP)

I'm not going to get to this anytime soon.

But our fullscreen support does need reworking at some point -- probably both on the Cocoa widget side and the Gecko side.  For example, I'm pretty sure that the biggest reason we have so much trouble with automated tests of fullscreen mode is that they tend to assume we've finished the transition into or out of fullscreen mode before we've really done so.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

This is left over from when I was a Mozilla employee. I'll never get back to it.

Comment 13

[Stephen A Pohl [:spohl]]

We're working on switching to native fullscreen in bug 1631735.