Closed
Bug 742178
Opened 13 years ago
Closed 2 years ago
[10.7] MakeFullScreen calls do nothing if in transition
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
INCOMPLETE
Tracking | Status | |
---|---|---|
firefox14 | - | --- |
People
(Reporter: zpao, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
This is working, but I may need to address some edge cases (hence the XXX comments).
Assignee: nobody → paul
Attachment #612396 -
Flags: feedback?(mstange)
Comment 2•13 years ago
|
||
Tracking for FF14 since 10.7 FS is a new feature and this bug may cause an unexpected state.
Reporter | ||
Updated•12 years ago
|
Attachment #612396 -
Flags: feedback?(smichaud)
Comment 3•12 years ago
|
||
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•12 years ago
|
||
[Triage Comment]
Re-assigning to Steven in response to comment 3.
Assignee: paul → smichaud
Comment 5•12 years ago
|
||
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•12 years ago
|
||
(In reply to comment #5)
I frankly don't know. Paul?
Comment 7•12 years ago
|
||
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)
Reporter | ||
Comment 8•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
(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.
Updated•12 years ago
|
Blocks: lion-compatibility
Comment 11•11 years ago
|
||
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)
Comment 12•2 years ago
|
||
This is left over from when I was a Mozilla employee. I'll never get back to it.
Assignee: smichaud → nobody
Comment 13•2 years ago
|
||
We're working on switching to native fullscreen in bug 1631735.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•