Open Bug 742178 Opened 9 years ago Updated 7 years ago

[10.7] MakeFullScreen calls do nothing if in transition

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set
normal

Tracking

()

Tracking Status
firefox14 - ---

People

(Reporter: zpao, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attached patch Patch v0.1 (WIP)Splinter Review
This is working, but I may need to address some edge cases (hence the XXX comments).
Assignee: nobody → paul
Attachment #612396 - Flags: feedback?(mstange)
Tracking for FF14 since 10.7 FS is a new feature and this bug may cause an unexpected state.
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 :-)
[Triage Comment]
Re-assigning to Steven in response to comment 3.
Assignee: paul → smichaud
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?
(In reply to comment #5)

I frankly don't know.  Paul?
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?
Attachment #612396 - Flags: feedback?(mstange)
(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.
(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).
(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 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.
Attachment #612396 - Flags: feedback?(smichaud)
You need to log in before you can comment on or make changes to this bug.