Closed Bug 630276 Opened 12 years ago Closed 12 years ago
Neither exiting full-screen video nor closing full-screen window restore menubar/dock
If you full-screen a video, then exit full-screen by pushing escape, the OS X menubar and dock remain hidden. Switching windows from Firefox to another application will restore them; getting them back permanently involves switching windows "several times" (haven't quite figured that part out yet, though the dock and menubar do eventually come back).
Does this happen on trunk too?
Sorry I didn't specify - this *is* trunk.
This is probably a softblocker.
blocking2.0: --- → ?
blocking2.0: ? → final+
issue also seems to happen if you close a window while in full screen mode.
taking it over for now. tested it on nightlies, looks like it breaks in 2011-01-19 but is good in 2011-01-18, this is the corresponding window: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b521854c006d&tochange=809aded51aad
Assignee: nobody → mars.martian+bugmail
It looks like http://hg.mozilla.org/mozilla-central/rev/acb48b922e91 causes the problem, not sure why though. CC'ing Olli to see if he can shed some light on the problem.
Summary: Exiting full-screen video doesn't restore menubar/dock → Neither exiting full-screen video nor closing full-screen window restore menubar/dock
Component: Video/Audio Controls → General
Product: Toolkit → Core
QA Contact: video.audio → general
blocking2.0: final+ → ---
OS: Mac OS X → Windows 7
Gavin, why did this become a non-blocking Windows bug?
huh, I have no idea how Bug 625311 could have caused that problem. We really can't back that out. How does fullscreen video work? Do we create a new widget for it? What keeps that widget alive? Are we not calling nsBaseWidget::OnDestroy() ?
(In reply to comment #8) > huh, I have no idea how Bug 625311 could have caused that problem. > We really can't back that out. ...or we'd need to fix the issue in some other way.
Hmm, is it possible that something in fullscreen video UI relies on widget to be deleted almost immediately after hiding the window?
blocking2.0: --- → final+
OS: Windows 7 → Mac OS X
From debugging this morning, it seems like something holds a reference to the nsThebesDeviceContext which isn't released normally but is later GC'ed and destroyed. After this, the widget is finally destroyed and consequently the menubar and dock are restored. Instead of solving that pointer problem, we could move the |if (mFullscreen)| block from ~nsCocoaWindow to nsCocoaWindow::Destroy. Not sure how risky that would be though. CC'ing Josh because it seems like he works on that section.
Moving it to Destroy sounds like a good idea.
Patch works in both cases, when closing a full screen window and when closing a full screen video. (In the second case, the video and tab are still around) Running this through on try to make sure it doesn't break anything.
Attachment #511203 - Flags: review?
Attachment #511203 - Flags: review? → review?(mstange)
Comment on attachment 511203 [details] [diff] [review] Patch v1, moves isFullscreen check to Destroy. Looks good, thanks! At some point we should make sure that we also handle the window being hidden and re-shown during its lifetime, and only hide the OS chrome while the fullscreen window is visible, but that can wait. Fullscreenable windows are currently guaranteed to be destroyed right after being hidden, so this will work for now.
(In reply to comment #14) > Comment on attachment 511203 [details] [diff] [review] > Patch v1, moves isFullscreen check to Destroy. > > Looks good, thanks! > > At some point we should make sure that we also handle the window being hidden > and re-shown during its lifetime, and only hide the OS chrome while the > fullscreen window is visible, but that can wait. Fullscreenable windows are > currently guaranteed to be destroyed right after being hidden, so this will > work for now. Sweet! Is your review enough to check this in, or should I wait for Josh to give it a look?
Better wait for Josh to give it another rubber stamp; he's the overlord here.
Comment on attachment 511203 [details] [diff] [review] Patch v1, moves isFullscreen check to Destroy. Thanks!
Attachment #511203 - Flags: review?(joshmoz) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.