Closed Bug 639705 Opened 13 years ago Closed 12 years ago

[10.7] add fullscreen support for Mac OS X 10.7 Lion

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla14

People

(Reporter: sgreenlay, Assigned: zpao)

References

(Depends on 4 open bugs, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-safari])

Attachments

(7 files, 15 obsolete files)

Safari in 10.7, Full Screen
209.52 KB, image/png
Details
Safari in 10.7, Full Screen with Menu Bar
260.08 KB, image/png
Details
Screen recording of interaction
1.46 MB, video/webm
Details
Patch v1.0 (Part 1: xp support for fullscreenbutton="true")
4.18 KB, patch
Details | Diff | Splinter Review
Patch v1.0 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)
9.86 KB, patch
Details | Diff | Splinter Review
Patch v1.0 (Part 3: turn it on & handle in browser)
5.75 KB, patch
Details | Diff | Splinter Review
Patch v1.2 (Part 4: fix content tests)
16.04 KB, patch
Details | Diff | Splinter Review 
      No description provided.
Blocks: lion-compatibility
Here is an initial patch that adds support for Lion's new full screen mode. 

Tested on the 10.7 11A390 release. (The first public seed from February)
Here is a more complete patch. This also implements the behaviour of Safari on Lion: if you move the pointer to the edge of the screen, the cursor changes to a little arrow. You can then click and drag to resize the content view. This is specially useful on large screens where you do want the full screen focus but not the content to be actually full screen.

Note that this currently only works if you enter full screen mode through View->FullScreen and *not* via the new button in the window.

This is because using the button does not yet send the "fullscreen" event that we catch in browser.js. I don't know how to do that.

I have added two delegate methods to WindowDelegate in nsCocoaWindow.mm that listen to the full screen notifications but I do not know how to turn those into DOM events.
Attachment #520533 - Attachment is obsolete: true
Whiteboard: [parity-safari]
Today's Lion update changes Safari's behaviour a bit.

Full screen now shows just a very lean location/search bar.

When you hover the mouse over it, it will show the menu bar, bookmarks bar and tabs bar. (And probably other bars if you have configured those)

This works really well on a small device like the 11" MacBook Air.

I wonder if we should clone this and turn the current full screen mode, where we do not show any UI, into a 'Presentation Mode'.

Adding screenshots.
It was noted in a crash comment today that Chrome has support for full screen as well. Not advocating, just noting.
(In reply to comment #7)
> It was noted in a crash comment today that Chrome has support for full
> screen as well. Not advocating, just noting.

It doesn't actually. They show the standard Lion 'go fullscreen' button but then they don't go full screen 'natively' but simply resize the window full screen.
Regarding what to display in full screen mode, I like the way Apple did it in iOS but not Lion.  I suggest we attach the address and tab bars to the top of the page so when you scroll down all you see is content, but pop them up on top of the content when you mvoe your cursor to the top of the screen (unless they're already visible as you've scrolled to the top of the page).
(In reply to comment #2)
> Created attachment 520694 [details] [diff] [review] [review]
> Work in Progress / Proof of Concept v2

This patch builds and works fine with the 10.7 SDK. But it doesn't currently build with 10.6 SDK.
(In reply to comment #10)
> (In reply to comment #2)
> > Created attachment 520694 [details] [diff] [review] [review] [review]
> > Work in Progress / Proof of Concept v2
> 
> This patch builds and works fine with the 10.7 SDK. But it doesn't currently
> build with 10.6 SDK.

Can you post the error here?
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #2)
> > > Created attachment 520694 [details] [diff] [review] [review] [review] [review]
> > > Work in Progress / Proof of Concept v2
> > 
> > This patch builds and works fine with the 10.7 SDK. But it doesn't currently
> > build with 10.6 SDK.
> 
> Can you post the error here?

g++-4.2 -o nsMacCursor.o -c  -fvisibility=hidden -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DMOZ_THUNDERBIRD=1 -DOSTYPE=\"Darwin11.0.0\" -DOSARCH=Darwin -DNO_X11 -I/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/../xpwidgets  -I/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa -I. -I../../../dist/include -I../../../dist/include/nsprpub  -I/Users/polysom/Documents/Developer/temp-6/src/obj-x86_64-apple-darwin11.0.0/mozilla/dist/include/nspr -I/Users/polysom/Documents/Developer/temp-6/src/obj-x86_64-apple-darwin11.0.0/mozilla/dist/include/nss       -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -isysroot /Developer/SDKs/MacOSX10.6.sdk -fno-strict-aliasing -fno-common -fshort-wchar -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -MD -MF .deps/nsMacCursor.pp -fobjc-exceptions /Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsMacCursor.mm
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm: In member function ‘nsresult nsCocoaWindow::CreateNativeWindow(const NSRect&, nsBorderStyle, PRBool)’:
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm:424: error: ‘NSWindowCollectionBehaviorFullScreenPrimary’ was not declared in this scope
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm: In member function ‘virtual nsresult nsCocoaWindow::MakeFullScreen(PRBool)’:
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm:1139: warning: ‘BaseWindow’ may not respond to ‘-toggleFullScreen:’
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm:1139: warning: (Messages without a matching method signature
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm:1139: warning: will be assumed to return ‘id’ and accept
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaWindow.mm:1139: warning: ‘...’ as arguments.)
make[7]: *** [nsCocoaWindow.o] Error 1
make[7]: *** Waiting for unfinished jobs....
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaUtils.h: In constructor ‘nsAutoRetainCocoaObject::nsAutoRetainCocoaObject(objc_object*)’:
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsCocoaUtils.h:58: warning: ‘_tmp’ may be used uninitialized in this function
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsChildView.mm: In function ‘NSRect -[ChildView firstRectForCharacterRange:](ChildView*, objc_selector*, NSRange)’:
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsChildView.mm:4975: warning: ‘rect.CGRect::size.CGSize::height’ is used uninitialized in this function
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsChildView.mm:4975: warning: ‘rect.CGRect::size.CGSize::width’ is used uninitialized in this function
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsChildView.mm:4975: warning: ‘rect.CGRect::origin.CGPoint::y’ is used uninitialized in this function
/Users/polysom/Documents/Developer/temp-6/src/mozilla/widget/src/cocoa/nsChildView.mm:4975: warning: ‘rect.CGRect::origin.CGPoint::x’ is used uninitialized in this function
make[6]: *** [libs] Error 2
make[5]: *** [libs] Error 2
make[4]: *** [libs_tier_platform] Error 2
make[3]: *** [tier_platform] Error 2
make[2]: *** [default] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2
Not sure how to deal with that. NSWindowCollectionBehaviorFullScreenPrimary is a simple enum value so you could simply add that. The toggleFullscreen selector on NSWindow can also be hidden by using using something like performSelector.

Doesn't Apple guarantee backward compatibility with the 10.7 SDK though? If you compile against 10.7 and make sure not to use any 10.7 features if you don't run on it then the app should be fine on 10.6 and 10.5?
(In reply to comment #13)
> Not sure how to deal with that. NSWindowCollectionBehaviorFullScreenPrimary
> is a simple enum value so you could simply add that.

That's what we usually do. With a different name (e.g. prefixed with "moz") so that building with the 10.7 SDK doesn't break.

> The toggleFullscreen
> selector on NSWindow can also be hidden by using using something like
> performSelector.

In some places we define the method in a category, for example here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/cocoa/nsCocoaUtils.h#99

> Doesn't Apple guarantee backward compatibility with the 10.7 SDK though? If
> you compile against 10.7 and make sure not to use any 10.7 features if you
> don't run on it then the app should be fine on 10.6 and 10.5?

Indeed. http://developer.apple.com/library/ios/#documentation/DeveloperTools/Conceptual/cross_development/Configuring/configuring.html#//apple_ref/doc/uid/10000163i-CH1-SW1

But as far as I know, we've historically always kept the minimum deployment target and the SDK in sync. I don't know the exact reasons for that, though.
There is a SIMBL hack that enables the full screen mode for all applications:

http://chpwn.com/apps/maximizer.html

I gave it a try and it works perfectly fine for Firefox as well (behavior like Safari, the tab bar stays there all the time).

Maybe that helps, I think the source code of this is available as well.

By the way: I believe there are 2 other things missing that the new Safari brings.
1. "rubber band" scrolling
2. swipe to go back/forward in history incl. the "card" animation
(In reply to comment #14)
> But as far as I know, we've historically always kept the minimum deployment
> target and the SDK in sync. I don't know the exact reasons for that, though.

Possibly just because it makes it much easier to accidentally link directly to new SDK features and make the app not work on older OS versions.
(In reply to comment #15)
> By the way: I believe there are 2 other things missing that the new Safari
> brings.
> 1. "rubber band" scrolling
> 2. swipe to go back/forward in history incl. the "card" animation

1. There is bug 636564
2. To go travel back/forth, change settings in System Preferences in the Trackpad section, More Gestures tab.
(In reply to comment #17)
> (In reply to comment #15)
> > By the way: I believe there are 2 other things missing that the new Safari
> > brings.
> > 1. "rubber band" scrolling
> > 2. swipe to go back/forward in history incl. the "card" animation
> 
> 1. There is bug 636564
> 2. To go travel back/forth, change settings in System Preferences in the
> Trackpad section, More Gestures tab.

Actually for Num 2 you should check the animation etc in Safari, this is something different (I just looked myself in Safari). It appears it could add the additional functionality of being able to REALLY easily compare two pages with each other, and in a conventional feeling way. I think developers in particular would like to see this in Firefox along with all the other excellent web dev tools.

True, the simple back/foward functionality can be duplicated, but that's not the whole picture (and I /really/ don't care for Safari).
Comment on attachment 520694 [details] [diff] [review]
Work in Progress / Proof of Concept v2

>+// Lion sends these notifications when the window changes full screen state. We need
>+// to fire an event here so that in browser.js we can do some additional work that
>+// is required when entering/leaving full screen mode.
>+
>+- (void)windowDidEnterFullScreen:(NSNotification *)notification
>+{
>+  printf("Window did enter full screen\n");
>+
>+  if (!mGeckoWindow)
>+    return;
>+
>+  // I had hoped that this would work. Searching for the right way to do this.
>+  //mGeckoWindow->DispatchCustomEvent("windowDidEnterFullScreen");
>+}
>+
>+- (void)windowDidExitFullScreen:(NSNotification *)notification
>+{
>+  printf("Window did exit full screen\n");
>+
>+  if (!mGeckoWindow)
>+    return;
>+
>+  // I had hoped that this would work. Searching for the right way to do this.
>+  //mGeckoWindow->DispatchCustomEvent("windowDidExitFullScreen");
>+}
>+
> - (void)windowDidBecomeMain:(NSNotification *)aNotification
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;

I think that you should use nsCommandEvent internally. It's being used for back/forward/home/stop button of mouse or multimedia keyboard. So, it's being used for executing the specified action at chrome.
http://mxr.mozilla.org/mozilla-central/source/widget/public/nsGUIEvent.h#1521
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#3036
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#3184
Depends on: 677573
> Possibly just because it makes it much easier to accidentally link directly
> to new SDK features and make the app not work on older OS versions.

There was also the problem of depending on libcryto, but that has been fixed.

Looks like the fear of accidentally depending on new features is why chromium is still using the 10.5 sdk but forward declaring the methods/constants they need.

How confident would we be that our testing (which would still include 10.5) would catch these problems?
This is an important feature and we need an owner for this.  Who owns it?
I am not experienced on these APIs, but I will try to take a look.
Rafael let me know if i can help.
I played with it a bit, and the tasks I see are

* Make it build with the 10.6 SDK. Blocking on us having 10.7 SDKs on the bots would take way too much time.
* Make it build with the 10.5 SDK if that is not automatic once it builds with 10.6.
* Put the runtime checks in place, test on 10.6, 10.5.
* Watch the full screen api talk from wwdc :-)
* Fix some differences with safari/FIXMEs:
  * Probably only the main window should have a full screen mode. Any other?
  * Bringing the menu down should take the tabs and address bar down with it.
  * Relabel the current full screen mode to presentation as suggested by stefan.

I think the last two items can probably be done in a followup patch.
This patch builds with 10.6 and 10.7. I added a try to see if it builds on 10.5:

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=0f4220edb0a9

I used MAC_OS_X_VERSION_MAX_ALLOWED to make it more likely that we will clean this up once we are using the 10.7 SDK.
Attachment #520694 - Attachment is obsolete: true
I believe just renaming the current full screen mode isn’t the perfect solution. It’s two things that are basically the same, just in slightly different flavors.

It would be (I think) a much more elegant and comprehensive solution to completely remove the current full screen mode instead of renaming it and adding an option/button in the toolbar/titlebar to hide the UI completely (hence “presentation mode”).

Coincidentally this is also what Chrome does. Opera (of course) has a presentation mode, but as Firefox is moving against becoming less complex and more streamlined I don’t think that’s the right solution.
Chrome did replace its fullscreen mode with the "Mac" fullscreen mode...sort of.  It actually just moved and renamed the same function.  Chrome on Mac does *not* have a Lion-style fullscreen mode.  Instead, it just expands to fill up the full screen (like Firefox's fullscreen mode).  This is significant because it doesn't push itself to a different desktop and it doesn't play nicely with other things in the window manager (modal dialogs in other apps on the same desktop being a good example).

My opinion: when running on Lion, Firefox should disable its own fullscreen functionality and use the Lion fullscreen mode.  I believe this is what Dominic is suggesting.
The current developer build of Chrome actually does move to a different desktop and all that.

Your opinion is my opinion, Daniel. I was just suggesting to add a button to the lion fullscreen mode to hide the toolbar to make up for otherwise lost functionality.
I agree, seem silly to have two fullscreen modes.  A button or menu option to hide the toolbars in fullscreen mode sounds like the best way (with the ability to set the default option for entering fullscreen mode).
(In reply to Sam Illingworth from comment #29)
> I agree, seem silly to have two fullscreen modes.  A button or menu option
> to hide the toolbars in fullscreen mode sounds like the best way (with the
> ability to set the default option for entering fullscreen mode).

I disagree, the new lion feature causes a second display to become effectively disabled when one application is in Lion-full-screen mode.  A few links to the issue:

http://apple.stackexchange.com/questions/17941/how-do-i-run-an-app-in-full-screen-mode-on-os-x-lion-on-my-second-monitor
http://forums.macrumors.com/showthread.php?t=1174757
http://www.macgasm.net/2011/06/14/lion-fullscreen-apps-render-monitor-useless/
http://osxdaily.com/2011/06/16/os-x-lion-full-screen-app-mode-doesnt-play-well-with-external-displays/
(In reply to Stacey Marshall from comment #30)
>A few links to the issue:
This isn't a real "issue", this is by design. See "Multiple Monitors and Full Screen" at: http://developer.apple.com/library/mac/#releasenotes/Cocoa/AppKit.html
(In reply to Nomis101 from comment #31)
> This isn't a real "issue", this is by design. 
Thanks for the pointer.  I would very much like to see firefox incorporate its excellent original design too.
I'd like to see both. Keep the current as Kiosk mode, and slap on the OS X Lion Full-screen mode to maintain consistency for those who adopt it.
Safari seems to implement a reasonable compromise. Show the back buttons, URL bar and search bar always. If you need to get at the toolbars, move your mouse to the top of the screen (like you're trying to get at the menubar) and appear.

Let me know if you need any help with the full screen APIs.
(In reply to Colin Barrett [:cbarrett] from comment #34)
> move your mouse to the top of the screen (like you're trying to get at the menubar)
> and appear.

Oops, should be "move your mouse to the top of the screen (like you're trying to get at the menubar) _they_ and appear."
(In reply to Colin Barrett [:cbarrett] from comment #35)
> (In reply to Colin Barrett [:cbarrett] from comment #34)
> > move your mouse to the top of the screen (like you're trying to get at the menubar)
> > and appear.
> 
> Oops, should be "move your mouse to the top of the screen (like you're
> trying to get at the menubar) _they_ and appear."

Wow, "and _they_ appear". Sorry about all the extra bugmail. Brain is not engaged today.
+1 vote to preserving the existing full screen mode somehow when the Lion full screen support is also added.

One use case that the Lion Full Screen mode stops is a multi monitor system and a full screen browser on each monitor.  The Lion API blanks extra monitors intentionally.

Chrome 14 brought in proper Lion API usage for it's two modes, full screen and presentation.  This change rendered Chrome unusable on a Lion system attached to three TVs.  Chrome has no current plans to also allow the old full screen mode, prompting an investigation into other browsers for the TV setup.  Chrome was initially picked due to the combination of a full screen mode that didn't use the Lion API, and Applescript compatibility.
Is this related to Bug 423014? In current Firefox fullscreen mode, whenever you mouse to the upper screen edge so the address bar and tab bar move down, the page content moves along. Like in Safari, Chrome or Opera, the mouse-triggered movement of the menubar and other bars should not move the page content.
Depends on: 703724
Comment on attachment 558845 [details] [diff] [review]
patch that builds with the 10.6 sdk

I just applied this and it seems to be working.

Some things:
* Our fullscreen button appears on the toolbar when entering through the menu, but not through the os button (our button should never appear)
* We should change our menu text to match Safari/Chrome "Enter/Exit Full Screen"
* Our keyboard shortcut doesn't match (I'm not sure if it ever did & don't have 10.6 installed to check) - ctrl+cmd+f seems to be pretty standard now, we do shift+cmd+f
* We should do like Safari does and move the top chrome down when the menubar gets shown (I don't really like what Chrome does). In both cases content isn't moved which is the right thing.

I understand the frustrations around Lion's fullscreen mode, but we've strived for OS-compatability so I think we should go down the path this starts & just support this mode on Lion & not add a second kiosk/whatever mode (let an addon do that?)
DOM full screen [1] also makes this a bit more confusing. Chrome ships this and makes the element (video is easiest example [2]) full screen and leaves it there unless the esc key is pressed or tab is switched. We exit full screen on "deactivate" (ie, the window loses focus for whatever reason including app switch). That whole experience is jarring on Lion because of the full screen animation. I kind of want our current full screen behavior there. Since that's not likely, maybe we just don't listen for "deactivate" on Lion - it's less important there anyway since we haven't hidden the dock & menubar ourselves.

[1]: https://developer.mozilla.org/en/DOM/Using_full-screen_mode
[2]: https://developer.mozilla.org/samples/domref/fullscreen.html
(In reply to Paul O'Shannessy [:zpao] from comment #41)
> DOM full screen [1] also makes this a bit more confusing. Chrome ships this

And as of Firefox 9 so do we: bug 545812.
(In reply to Philipp von Weitershausen [:philikon] from comment #42)
> (In reply to Paul O'Shannessy [:zpao] from comment #41)
> > DOM full screen [1] also makes this a bit more confusing. Chrome ships this
> 
> And as of Firefox 9 so do we: bug 545812.

I know :) I meant it as a data point for working references. Safari ends up being the "canonical" reference most of the time, but that doesn't exist so all we have is Chrome right now.
Ok, I have a patch that "works" & overcomes the problems people had with browser.js & the "fullscreen" event. I have bug 539601 with some changes applied, so I might make this dependent on that bug or just integrate that here - it's all the same ball game. I still need to cleanup the browser.js side of this & make sure we do the right thing with domfullscreen.

I'm still new to this OS X API mess - I know for a fact that I'm using at least one thing added in 10.7 (NSFullScreenWindowMask) but I think that can just be defined somewhere. I'm guessing if Raphael's patch built on 10.6 it should build on 10.5 while we support that. I think I can look at nsCocoaFeatures::OnLionOrLater & use respondsToSelector to success.

I don't have a 10.6 machine handy to test/build so I'll need some testing help. I'll post here with builds.

Limi: I'll post a screencast when I clean up the edges here so you can give feedback on the UX to make sure it's good to go.
Blocks: 711750
Attached patch Patch v0.4 (WIP) (obsolete) — — Splinter Review 
Thoughts:
 * not sure I actually needed the "nativeFullscreen" feature in window.open
 * this should be broken up, but for ease right now, here's the whole thing
 * not sure what happens on <10.7 with this patch, particularly around fullscreen events (more particularly when leaving)

Problems:
 * popups aren't getting the fullscreen button, but they should (for parity-safari, chrome doesn't have the fullscreen button for popups)
 * domfullscreen is triggering more "fullscreen" events than it should
 * leaving domfullscreen doesn't re-enable toolbars
 * toolbar fullscreen button still shows
Assignee: nobody → paul
Attachment #583668 - Flags: feedback?(smichaud)
Attachment #583668 - Flags: feedback?(mstange)
(In reply to Paul O'Shannessy [:zpao] from comment #45)
>  * popups aren't getting the fullscreen button, but they should (for
> parity-safari, chrome doesn't have the fullscreen button for popups)

This is going to be because I'm not turning "nativeFullscreen" on if it's not specified in window.open. Not sure I want to do that.... I'm also not sure that it should be exposed to content (or if we can even really control that, though I guess we must with "chrome" et al).
(In reply to Paul O'Shannessy [:zpao] from comment #45)
>  * domfullscreen is triggering more "fullscreen" events than it should

This is because exiting domfullscreen goes through nsDocument::ExitFullScreen which calls nsGlobalWindow::SetFullScreen(false) directly which  triggered a cascading effecting of wrongness.

nsGlobalWindow::SetFullScreen(false) -- dispatches "fullscreen" event
-->nsCocoaWindow::MakeFullScreen
   -->windowWillExitFullScreen
      -->DispatchSizeModeEvent -- nsSizeMode_Fullscreen
         -->nsWebShellWindow::HandleEvent
            -->nsGlobalWindow::SetFullScreen(true) -- dispatches "fullscreen" event

   -->windowDidResize
      -->DispatchSizeModeEvent -- nsSizeMode_Normal
         -->nsWebShellWindow::HandleEvent -- previous sizemode was nsSizeMode_Fullscreen
            -->nsGlobalWindow::SetFullScreen(false) -- dispatches "fullscreen" event

Anyway, the fix for that seems to be to also check mFullScreen in GetWindowSizeMode. I'll probably want to do something similar in nsCocoaWindow::SetSizeMode
Comment on attachment 583668 [details] [diff] [review]
Patch v0.4 (WIP)

I'm very grateful for your work on this.  But I'm just about to go away on vacation, and won't be back until early January.  Once I'm back I intend to do both a code review and a "user's review".
Attached patch Patch v0.5 (WIP) (obsolete) — — Splinter Review 
This has the opposite problem v0.4 had: now all windows get the fullscreen button, even those that shouldn't. I think the path I want to go there is default on for content, default off for chrome? Maybe that means a "-moz-native-fullscreen" option a la "-moz-internal-modal" (bug 414291). That also means that anywhere that has "nativeFullscreen" in its feature string isn't actually doing anything right now...

I went back & simplified some things in browser.js's FullScreen.toggle so it's much closer to what it was. We don't need to make it too complicated yet (a lot of that was done to add in content window resizing which we won't do here).

It looks like we're getting the right number of fullscreen events, so yay.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b6655cd08694
(which means there should be builds to play with at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/poshannessy@mozilla.com-b6655cd08694 at some point).
Attachment #583668 - Attachment is obsolete: true
Attachment #583668 - Flags: feedback?(smichaud)
Attachment #583668 - Flags: feedback?(mstange)
Attachment #584660 - Flags: feedback?(smichaud)
Attachment #584660 - Flags: feedback?(mstange)
Attachment #584660 - Flags: feedback?(joshmoz)
Fun problem that will need to be fixed: If the site identity panel hasn't been opened before going into fullscreen, then opening it while in fullscreen will result in it being opened in a new space. However if it was opened before going FS, then it works just fine. I'm not sure if we need to do something cocoa side or not (I remember seeing something about dependent windows in the fullscreen docs) but I'll look into it.
Comment on attachment 584660 [details] [diff] [review]
Patch v0.5 (WIP)

Looks good to me. You can use an attribute like fullscreenbutton="true" on those windows that should be fullscreenable and implement it like the toggletoolbar attribute.
Attachment #584660 - Flags: feedback?(mstange) → feedback+
(In reply to Markus Stange from comment #51)
> Comment on attachment 584660 [details] [diff] [review]
> Patch v0.5 (WIP)
> 
> Looks good to me. You can use an attribute like fullscreenbutton="true" on
> those windows that should be fullscreenable and implement it like the
> toggletoolbar attribute.

That was my original plan, I just couldn't figure out how to make that work - I really didn't want to mess around with window.open(). I'll look into that as it gives much finer control. Thanks for the pointer about toggletoolbar!
Attached patch Patch v0.6 (obsolete) — — Splinter Review 
Main changes:
* some sizemode stuff pulled back out into bug 539601 (new patch there)
* uses fullscreenbutton="true" to turn on (much better!)
* using nsToolkit instead of nsCocoaFeatures
* tracking if we're using native fullscreen so other windows can still go fullscreen (was an issue fixing test_bug665540.html - it worked with bug 539601, then failed again here). This also lets us quickly turn off fullscreen support in browser
* took out a bunch of debugging
* actually revved the iid in nsIWidget

I intend to pull this into separate patches to make it easier for review (and because it makes sense)
* core changes to support fullscreenbutton="true" in xul
* os x implementation of SetShowsFullScreenButton (core Lion fullscreen support)
* browser changes (what people really mean with this bug - making the browser window actually show the button & handle it semi-properly). we might actually want to pull this out into a different bug

TODO:
* make easy to turn off - I was going to put this behind a pref, but I think adding minimal additional checking in browser.js (look for window.fullscreenbutton) is all we'd really need. then we can quickly "turn it off" if need be by just removing that attribute.

Most other things I'm pushing off to followups & marking dependent on this as I go. It's all in https://etherpad.mozilla.org/osx-lion-fullscreen if not up here.
Attachment #585623 - Flags: feedback?(smichaud)
Attachment #585623 - Flags: feedback?(joshmoz)
Attachment #584660 - Flags: feedback?(smichaud)
Attachment #584660 - Flags: feedback?(joshmoz)
Comment on attachment 585623 [details] [diff] [review]
Patch v0.6

I don't understand how the fullscreen mode is useful at this point. Sigh... I hate OS X.

-    if (enterFS) {
+    if (!this.isOSXLion && enterFS) {

It looks like you'll enter the else branch (that one for leaving FS) with isOSXLion=true and enterFS=true; we only want this for enterFS=false.

Requesting feedback from cpearce for not listening to the deactivate event in DOM fullscreen mode.
Attachment #585623 - Flags: feedback?(dao)
Attachment #585623 - Flags: feedback?(chris)
Attachment #585623 - Flags: feedback+
Comment on attachment 585623 [details] [diff] [review]
Patch v0.6

Review of attachment 585623 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looping me in Dão.

::: browser/base/content/browser.js
@@ +3875,2 @@
>      // show/hide all menubars, toolbars (except the full screen toolbar)
> +    if (!this.isOSXLion)

So these changes are going to prevent DOM full-screen mode from hiding all toolbars etc on OSX Lion. We *need* to still do all the things we're currently doing if document.mozFullScreen is true. One way to achieve that would be by changing isOSXLion to return:
  !document.mozFullScreen && /^11\./.test(Services.sysinfo.getProperty("version"));

You should then probably rename isOSXLion to something like isOSXLionFullScreenMode so it reflects its altered meaning.

Note you can test how your patch interacts with DOM full-screen mode on my test page at http://pearce.org.nz/full-screen/
Behaviour of DOM full-screen mode should not change with your patch applied.

@@ +3978,5 @@
>      gBrowser.tabContainer.addEventListener("TabSelect", this.exitDomFullScreen);
>  
>      // Exit DOM full-screen mode when the browser window loses focus (ALT+TAB, etc).
> +    if (!this.isOSXLion) {
> +      window.addEventListener("deactivate", this.exitDomFullScreen, true);

We should do this in DOM full-screen mode, else we add a security hole.

Not doing this in F11/Cmd+Shift+F/Lion fullscreen mode is probably ok, it's what we used to do.
Attachment #585623 - Flags: feedback?(chris) → feedback-
(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #55)
> Comment on attachment 585623 [details] [diff] [review]
> Patch v0.6
> 
> Review of attachment 585623 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for looping me in Dão.
> 
> ::: browser/base/content/browser.js
> @@ +3875,2 @@
> >      // show/hide all menubars, toolbars (except the full screen toolbar)
> > +    if (!this.isOSXLion)
> 
> So these changes are going to prevent DOM full-screen mode from hiding all
> toolbars etc on OSX Lion. We *need* to still do all the things we're
> currently doing if document.mozFullScreen is true. One way to achieve that
> would be by changing isOSXLion to return:
>   !document.mozFullScreen &&
> /^11\./.test(Services.sysinfo.getProperty("version"));

So that's not entirely true, enterDomFullScreen calls mouseoverToggle, which does hide the top chrome (gNavToolbox). But yea showXULChrome goes through other toolbars (as far as shipping toolbars... that leaves the addon bar).

> You should then probably rename isOSXLion to something like
> isOSXLionFullScreenMode so it reflects its altered meaning.

Yea, I had changed it already locally to something similar (after adding a check for fullscreenbutton to turn it off)

> Note you can test how your patch interacts with DOM full-screen mode on my
> test page at http://pearce.org.nz/full-screen/

I've been using https://developer.mozilla.org/samples/domref/fullscreen.html

> Behaviour of DOM full-screen mode should not change with your patch applied.

I want to disagree but I'm not sure what I'm going to disagree with, so can you clarify?

I'm picturing dom fullscreen making firefox go into a space just like normal fullscreen would + hide UI, with the exception that most things will pop us out of fullscreen (and apart from the addon bar that's how it works). See how Chrome behaves.

> @@ +3978,5 @@
> >      gBrowser.tabContainer.addEventListener("TabSelect", this.exitDomFullScreen);
> >  
> >      // Exit DOM full-screen mode when the browser window loses focus (ALT+TAB, etc).
> > +    if (!this.isOSXLion) {
> > +      window.addEventListener("deactivate", this.exitDomFullScreen, true);
> 
> We should do this in DOM full-screen mode, else we add a security hole.

I think this is the part I disagree with. If I pop a video fullscreen, then cmd+tab to check my mail for a second, I don't want to go back to Firefox and put that video fullscreen again.

I'll admit naivety here, but what's the security hole? Spoofing UI doesn't seem like _that_ big of a risk (and I think it's even less of an issue on Lion). 

> Not doing this in F11/Cmd+Shift+F/Lion fullscreen mode is probably ok, it's
> what we used to do.

That sentence makes my head hurt :) But yes, we currently don't exit normal fullscreen on deactivate.
(In reply to Paul O'Shannessy [:zpao] from comment #56)
> So that's not entirely true, enterDomFullScreen calls mouseoverToggle, which
> does hide the top chrome (gNavToolbox). But yea showXULChrome goes through
> other toolbars (as far as shipping toolbars... that leaves the addon bar).

I'm not sure what you mean by this. I was merely proposing a way to make your changes not interfere with the existing DOM full-screen API's behaviour.


> > Behaviour of DOM full-screen mode should not change with your patch applied.
> 
> I want to disagree but I'm not sure what I'm going to disagree with, so can
> you clarify?

The behaviour of DOM full-screen mode should not be affected by your patch, i.e. when script calls HTMLElement.mozRequestFullScreen() and is granted full-screen, the requesting HTML element should occupy the entire screen; there should be no chrome showing and chrome should not pop up.

When the user on OSX Lion opts to make Firefox enter Lion full-screen mode (for want of a better name) then I think it's perfectly reasonable to do whatever is appropriate on that platform (e.g. leave some chrome visible like Safari or whatever).

These two full-screen modes are distinct and different, and should be treated as such.


> > @@ +3978,5 @@
> > >      gBrowser.tabContainer.addEventListener("TabSelect", this.exitDomFullScreen);
> > >  
> > >      // Exit DOM full-screen mode when the browser window loses focus (ALT+TAB, etc).
> > > +    if (!this.isOSXLion) {
> > > +      window.addEventListener("deactivate", this.exitDomFullScreen, true);
> > 
> > We should do this in DOM full-screen mode, else we add a security hole.
> 
> I think this is the part I disagree with. If I pop a video fullscreen, then
> cmd+tab to check my mail for a second, I don't want to go back to Firefox
> and put that video fullscreen again.
> 
> I'll admit naivety here, but what's the security hole? Spoofing UI doesn't
> seem like _that_ big of a risk (and I think it's even less of an issue on
> Lion). 

Spoofing a non-full-screen Firefox with a paypal.com login page isn't a big deal?

You'd be best to take this up with the security guys, they wanted this.
> When the user on OSX Lion opts to make Firefox enter Lion full-screen mode
> (for want of a better name) then I think it's perfectly reasonable to do
> whatever is appropriate on that platform (e.g. leave some chrome visible
> like Safari or whatever).
> 
> These two full-screen modes are distinct and different, and should be
> treated as such.

I'm not a fan of distinct full-screen modes, but DOM full-screen semantics are certainly defined differently from Lion's, so it seems unavoidable.  The one thing I would *strongly* request is that the DOM full-screen be implemented under Lion to still take advantage of the Lion full-screen API -- i.e. move onto a separate and isolated space.  This is very important.  Without this, full-screen interacts very poorly with Cocoa's focus management, not to mention surprising users who are used to "native" application behavior.

I would look to the QuickTime application as an example of how the DOM full-screen mode should behave under Lion.  This seems to be a relevant model, since the most immediate application of DOM full-screen is for video playback in the browser.

> Spoofing a non-full-screen Firefox with a paypal.com login page isn't a big
> deal?
> 
> You'd be best to take this up with the security guys, they wanted this.

Given the semantics of the Lion full-screen (isolated on a dedicated space, system-controlled menu bar, system-controlled background), this would seem to be a *very* difficult attack vector to exploit.  It would certainly be doable on any other platform, but OS X has too much WS-global stuff going on to allow attacks of this nature.  I'm not saying it's impossible, just very difficult.

This again is an area where using the Lion full-screen APIs for the DOM full-screen mode kills two birds with one stone: it makes it much harder to spoof a non-full-screen browser, and it behaves exactly how a user would expect (and want!) w.r.t. focus management.
(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #57)
> (In reply to Paul O'Shannessy [:zpao] from comment #56)
> > So that's not entirely true, enterDomFullScreen calls mouseoverToggle, which
> > does hide the top chrome (gNavToolbox). But yea showXULChrome goes through
> > other toolbars (as far as shipping toolbars... that leaves the addon bar).
> 
> I'm not sure what you mean by this. I was merely proposing a way to make
> your changes not interfere with the existing DOM full-screen API's behaviour.

But I want it to interfere with the existing DOM full screen behavior.

> > > Behaviour of DOM full-screen mode should not change with your patch applied.
> > 
> > I want to disagree but I'm not sure what I'm going to disagree with, so can
> > you clarify?
> 
> The behaviour of DOM full-screen mode should not be affected by your patch,
> i.e. when script calls HTMLElement.mozRequestFullScreen() and is granted
> full-screen, the requesting HTML element should occupy the entire screen;
> there should be no chrome showing and chrome should not pop up.
> 
> When the user on OSX Lion opts to make Firefox enter Lion full-screen mode
> (for want of a better name) then I think it's perfectly reasonable to do
> whatever is appropriate on that platform (e.g. leave some chrome visible
> like Safari or whatever).
> 
> These two full-screen modes are distinct and different, and should be
> treated as such.

I don't think I see them as distinct & different as you do.

I agree that dom fullscreen should hide all UI (and it's a bug that mine doesn't completely). But I want it to use Lion's full screen go to a space behavior, not just fill the screen.

> Spoofing a non-full-screen Firefox with a paypal.com login page isn't a big
> deal?
> 
> You'd be best to take this up with the security guys, they wanted this.

How does exiting on deactivate help much? They can still spoof the UI and by then it's too late. The chance of somebody deactivating the window at that point doesn't seem that likely.

(In reply to Daniel Spiewak from comment #58)
> I'm not a fan of distinct full-screen modes, but DOM full-screen semantics
> are certainly defined differently from Lion's, so it seems unavoidable.  The
> one thing I would *strongly* request is that the DOM full-screen be
> implemented under Lion to still take advantage of the Lion full-screen API
> -- i.e. move onto a separate and isolated space.  This is very important. 
> Without this, full-screen interacts very poorly with Cocoa's focus
> management, not to mention surprising users who are used to "native"
> application behavior.

It looks like we agree, and that's actually how I have it implemented right now.
Just to make it clear as to how I think we should behave.

This is as of patch v0.6 (+fixed Dao's comment) and bug 714172 (menu changes).
(In reply to Paul O'Shannessy [:zpao] from comment #60)
> Created attachment 586227 [details]
> Screen recording of interaction
> 
> Just to make it clear as to how I think we should behave.

This is exactly the behavior I would expect (and very much desire).  The only exception I take is the fact that the chrome doesn't move down with the menu bar, which has already been noted.
After playing on my colleague's dual monitor Lion setup, I think I understand Lion full-screen mode better...

It seems like what we should aim for is, with regards to the DOM API is:
* DOM full-screen mode should use the Lion API, so on Lion we get a "native" user experience when using the DOM full-screen API.
* If DOM full-screen mode uses the Lion API, it's OK to not listen on the "deactivate" event, since on Lion you have a clear animation switching between the Lion full-screen "space" and non full-screen apps.
* In DOM full-screen mode we should still hide the chrome, but it's OK if it reappears upon hover at the top of screen (same behaviour as Google Chrome, Safari), provided this doesn't cause the full-screen element to resize (i.e. the UI at the top needs to be an overlay).

So what you propose in your animation in comment 60 looks fine.

Non DOM full-screen related comments:
* We should definitely provide a keyboard shortcut for the Lion full-screen mode.
* I'm not sure we should even keep the old CMD+SHIFT+F full-screen mode around on Lion, that would be a decision for the UX team.
(In reply to Daniel Spiewak from comment #61)
> (In reply to Paul O'Shannessy [:zpao] from comment #60)
> > Created attachment 586227 [details]
> > Screen recording of interaction
> > 
> > Just to make it clear as to how I think we should behave.
> 
> This is exactly the behavior I would expect (and very much desire).  The
> only exception I take is the fact that the chrome doesn't move down with the
> menu bar, which has already been noted.

I think that should be done in a followup. It's the same thing Chris mentions as far as hovering UI in DOM full screen - it would need to be done such that we don't cause the content to resize.

The first stopgap solution is to just add more space at the top, which I filed bug 714186 for. It's just so very cramped.

(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #62)
> After playing on my colleague's dual monitor Lion setup, I think I
> understand Lion full-screen mode better...

:)

> It seems like what we should aim for is, with regards to the DOM API is:
> * DOM full-screen mode should use the Lion API, so on Lion we get a "native"
> user experience when using the DOM full-screen API.
> * If DOM full-screen mode uses the Lion API, it's OK to not listen on the
> "deactivate" event, since on Lion you have a clear animation switching
> between the Lion full-screen "space" and non full-screen apps.
> * In DOM full-screen mode we should still hide the chrome, but it's OK if it
> reappears upon hover at the top of screen (same behaviour as Google Chrome,
> Safari), provided this doesn't cause the full-screen element to resize (i.e.
> the UI at the top needs to be an overlay).

I think for a first pass at least, we should skip that. As I have it now we won't have any UI reappear while in DOM fullscreen. I'll file a followup bug & get UX input there.

> So what you propose in your animation in comment 60 looks fine.

Great!

> Non DOM full-screen related comments:
> * We should definitely provide a keyboard shortcut for the Lion full-screen
> mode.
> * I'm not sure we should even keep the old CMD+SHIFT+F full-screen mode
> around on Lion, that would be a decision for the UX team.

Lion full screen mode is going to replace the other full screen mode where available, so we're just going to use the same keyboard shortcut. In bug 714172 the shortcut & menu strings will change to be consistent with other OS X apps.
This just adds the cross platform part of supporting fullscreenbutton="true"
Attachment #587868 - Flags: review?(enndeakin)
Attachment #587868 - Attachment is patch: true
There are still a few XXX comments in there, but they're all minor things that can be answered/addressed in review.
Attachment #587873 - Flags: review?(joshmoz)
Comment on attachment 587873 [details] [diff] [review]
Patch v0.7 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)

>+void nsCocoaWindow::SetShowsFullScreenButton(bool aShow)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>+
>+  //XXXzpao What if this gets called while already in non-native fullscreen?
>+  //        Technically it's possible, though we won't currently do it.
>+  //        mUsesFullScreen will lead to the wrong path taken in MakeFullScreen(false);
>+  if (nsToolkit::OnLionOrLater()) {
>+    NSWindowCollectionBehavior newBehavior = [mWindow collectionBehavior];
>+    if (aShow)
>+      newBehavior |= NSWindowCollectionBehaviorFullScreenPrimary;
>+    else
>+      newBehavior &= ~NSWindowCollectionBehaviorFullScreenPrimary;
>+    [mWindow setCollectionBehavior: newBehavior];
>+    mUsesNativeFullScreen = aShow;
>+  }
>+
>+  NS_OBJC_END_TRY_ABORT_BLOCK;
>+}

Note: I haven't actually checked that this works when aShow == false...
This sets fullscreenbutton="true" in browser.xul and then handles that appropriately for the browser window.
Attachment #587879 - Flags: review?(dao)
Attachment #587868 - Flags: review?(enndeakin) → review+
Attachment #585623 - Flags: feedback?(smichaud)
Attachment #585623 - Flags: feedback?(joshmoz)
Without bug 714911 applied, there's actually a problem at startup. Since we're persisting sizemode=fullscreen, if you quit with a window fullscreen we try to go into fullscreen at startup.

So we call nsCocoaWindow::MakeFullScreen, get past NSDisableScreenUpdates, but then toggleFullScreen asserts several layers deep in Cocoa. We catch it just fine, but we never call NSEnableScreenUpdates so the window gets super laggy as you resize & drag. Everything else is fine.

The assertion:
2012-01-13 12:18:52.171 firefox-bin[34335:507] *** Assertion failure in -[_NSWindowFullScreenTransition makeAndSetupOverlayWindow], /SourceCache/AppKit/AppKit-1138.23/AppKit.subproj/NSWindowFullScreenTransition.m:671
2012-01-13 12:18:52.172 firefox-bin[34335:507] Mozilla has caught an Obj-C exception [NSInternalInconsistencyException: Invalid parameter not satisfying: _transitionedWindowBeforeContents != nil]

(and then a stack trace of numbers, nothing actually useful, not looking up symbols?)

I'm going back now to see if we can fix it w/o the no-persist patch. Based on the error, we're calling MakeFullScreen before CreateNativeWindow is done...

I tried to handle it by calling NSEnableScreenUpdates from windowDidFailToEnterFullScreen, but it looks like we never get there. So we could try+catch ourselves and not use the NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT macro, calling enablescreenupdates regardless...
Depends on: 714911
Comment on attachment 587873 [details] [diff] [review]
Patch v0.7 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)

Review of attachment 587873 [details] [diff] [review]:
-----------------------------------------------------------------

I made a build with your patch queue, the fullscreen functionality looks pretty good. Thanks for working on this!

::: widget/cocoa/nsCocoaWindow.mm
@@ +1098,5 @@
>      // We will reach here on a call to MakeFullScreen(true), so we need to
>      // check that we're not already in full screen mode before calling.
> +    // XXXzpao should we amend the fullscreen check on Lion?
> +    // if (!mFullScreen && (!mUsesNativeFullScreen ||
> +    //                     !([mWindow styleMask] & NSFullScreenWindowMask)))

Lets not leave this an open question.

@@ +1188,5 @@
> +  // On Lion we're going to dispatch a size mode event from SetFullScreen, which
> +  // nsAppShell handles & forwards to nsGlobalWindow to here. We're already in
> +  // full mode by this point, so handle it gracefully.
> +  if (mFullScreen == aFullScreen)
> +    return NS_OK;

Please use curly braces for all "if" blocks, even if they are one line. Rule for Cocoa widgets.

@@ +1194,5 @@
>    NSDisableScreenUpdates();
> +  nsresult rv = NS_OK;
> +
> +  if (mUsesNativeFullScreen) {
> +    [mWindow toggleFullScreen: nil];

No space between ":" and the argument in Obj-C messages in Cocoa widgets.

@@ +1431,5 @@
>  static nsSizeMode
>  GetWindowSizeMode(NSWindow* aWindow, bool aFullScreen) {
> +  //XXXzpao should we amend the fullscreen check on Lion?
> +  // if (aFullScreen && (!mUsesNativeFullScreen ||
> +  //                     [aWindow styleMask] & NSFullScreenWindowMask))

This needs to be resolved.

@@ +1665,5 @@
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +
> +  //XXXzpao What if this gets called while already in non-native fullscreen?
> +  //        Technically it's possible, though we won't currently do it.
> +  //        mUsesFullScreen will lead to the wrong path taken in MakeFullScreen(false);

This needs to be resolved.

@@ +1914,5 @@
> +// because our "fullscreen" events are supposed to fire before the transition.
> +- (void)windowWillEnterFullScreen:(NSNotification *)notification
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +  printf("Window will enter full screen\n");

This should be removed.

@@ +1920,5 @@
> +  if (!mGeckoWindow)
> +    return;
> +
> +  mGeckoWindow->SetFullScreen(true);
> +  NS_OBJC_END_TRY_ABORT_BLOCK;

Put an empty line before end exception handling macros.

@@ +1926,5 @@
> +
> +- (void)windowWillExitFullScreen:(NSNotification *)notification
> +{
> +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> +  printf("Window will exit full screen\n");

This should be removed.
Attachment #587873 - Flags: review?(joshmoz) → review-
(In reply to Josh Aas (Mozilla Corporation) from comment #69)
> I made a build with your patch queue, the fullscreen functionality looks
> pretty good. Thanks for working on this!

Sorry for making you jump through hoops to get it going!

> ::: widget/cocoa/nsCocoaWindow.mm
> @@ +1098,5 @@
> >      // We will reach here on a call to MakeFullScreen(true), so we need to
> >      // check that we're not already in full screen mode before calling.
> > +    // XXXzpao should we amend the fullscreen check on Lion?
> > +    // if (!mFullScreen && (!mUsesNativeFullScreen ||
> > +    //                     !([mWindow styleMask] & NSFullScreenWindowMask)))
> 
> Lets not leave this an open question.

I agree. I was hoping you might have an opinion :) I haven't noticed any issues without amending the check so I think we should just leave it as is.

> @@ +1431,5 @@
> >  static nsSizeMode
> >  GetWindowSizeMode(NSWindow* aWindow, bool aFullScreen) {
> > +  //XXXzpao should we amend the fullscreen check on Lion?
> > +  // if (aFullScreen && (!mUsesNativeFullScreen ||
> > +  //                     [aWindow styleMask] & NSFullScreenWindowMask))
> 
> This needs to be resolved.

Same.

> @@ +1665,5 @@
> > +  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> > +
> > +  //XXXzpao What if this gets called while already in non-native fullscreen?
> > +  //        Technically it's possible, though we won't currently do it.
> > +  //        mUsesFullScreen will lead to the wrong path taken in MakeFullScreen(false);
> 
> This needs to be resolved.

This one is going to be a bit trickier to resolve. We may want to track if we're in native full screen and return NS_ERROR? (would need to change the signature which I'm not wild about, it'd be nice to match setShowsToolbarButton).

Another option is to actually track if we're in native fullscreen mode & adjust accordingly where ever we're using mUsesNativeFullScreen (ie, mUsesNativeFullScreen || mInNativeFullScreen). That's not too bad actually...

Or maybe the best thing is to just correct. If we're in non-native fullscreen & SetShowsFullScreenButton(true) is called, we exit fullscreen, change our collection behavior, then enter fullscreen again.

I'm leaning towards the 3rd option - it keeps us from having to track yet another thing. What do you think?

The other remaining problem with this is browser side, where as I have it, I'm using a lazy getter to check fullscreenbutton. I can make that less lazy, but as is, we don't remove the attribute so it doesn't actually matter.

Then there's the problem from comment 68...
I addressed the review comments & also removed the definition of NSFullScreenWindowMask because I'm not using it anymore (was a part of the amended full screen check for Lion).

Regarding the options I presented for fixing SetShowsFullScreenButton, I went with option 3 (transition out, update, transition back in) and played with it - it works just fine & really isn't that bad. It's not a case that we _should_ ever really see, but it's workable.

And then the exception being thrown & putting us into a weird state - Neil gave me f+ in bug 714911 to not persist sizemode=fullscreen, so that should take care of that.
Attachment #558845 - Attachment is obsolete: true
Attachment #584660 - Attachment is obsolete: true
Attachment #585623 - Attachment is obsolete: true
Attachment #587873 - Attachment is obsolete: true
Attachment #591247 - Flags: review?(joshmoz)
This is pretty awesome Paul. Thank you so much.

In my original Proof of Concept patch I also had the ability to resize the content area like Safari allows you to do. Do you think we can have that too? That feature is super useful on big screens.
(In reply to Stefan Arentz [:st3fan] from comment #72)
> This is pretty awesome Paul. Thank you so much.

:)

> In my original Proof of Concept patch I also had the ability to resize the
> content area like Safari allows you to do. Do you think we can have that
> too? That feature is super useful on big screens.

I took that out here because it became just another hurdle that could prevent this from landing - I wanted to boil it down to the minimum level of support.

Bug 703724 is on file for the resizable content area. Some of the other followup work is higher priority, but I would like to go back and get that in. I'll go through your patch again and see what I can salvage (ideally we could do it without having to set up our own listeners to fake resizing)
I'm not completely sure if I had missed this before or if something about Markus's recent sizemode event changes broke this, but exiting fullscreen via the native button was no longer resulting in nsGlobalWindow::SetFullScreen(false) being called. That's fixed now using a slightly different approach.

Josh, I know you've looked at this already but I'm also tagging Markus here in case you can't get to it soon. I'd like to land this ASAP so we can catch as much followup work as possible before the next merge.
Attachment #591247 - Attachment is obsolete: true
Attachment #591247 - Flags: review?(joshmoz)
Attachment #594868 - Flags: review?(mstange)
Attachment #594868 - Flags: review?(joshmoz)
Comment on attachment 594868 [details] [diff] [review]
Patch v0.9 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)

>diff --git a/widget/cocoa/nsCocoaWindow.mm b/widget/cocoa/nsCocoaWindow.mm

>+NS_METHOD nsCocoaWindow::SetFullScreen(bool aFullScreen)
>+{
>+  // This method should only be called if we're using native fullscreen mode.
>+  if (mUsesNativeFullScreen && mFullScreen != aFullScreen) {
>+    // Since this isn't going through nsGlobalWindow::SetFullScreen, we need to
>+    // set mFullScreen ourselves.
>+    mFullScreen = aFullScreen;
>+
>+    DispatchSizeModeEvent();

Just to check if I've understood correctly: The call to DispatchSizeModeEvent will eventually call MakeFullScreen (through nsGlobalWindow::SetFullScreen), but that won't do anything because mFullScreen already has the new value. Right?

>+  }
>+  return NS_OK;
>+}
> 
> NS_METHOD nsCocoaWindow::MakeFullScreen(bool aFullScreen)
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> 
>-  NS_ASSERTION(mFullScreen != aFullScreen, "Unnecessary MakeFullScreen call");
>+  // When using native fullscreen, when we enter/exit fullscreen via the toolbar
>+  // button, we will bypass our usual checks. We will set mFullScreen and dispatch
>+  // the sizemode event from there. nsGlobalWindow will handle that event as a
>+  // normal sizemode event, so it will then call MakeFullScreen. Since we
>+  // dispatched the event we don't need to handle it again, but we also don't
>+  // need to ASSERT. We'll handle it gracefully instead.
>+  if (mFullScreen == aFullScreen) {
>+    return NS_OK;
>+  }
> 
>   NSDisableScreenUpdates();

NSDisableScreenUpdates and NSEnableScreenUpdates should only be used in the non-native fullscreen case. Their purpose is to hide the multiple window resizes and content area shifts that happen during the transition.
In the native fullscreen case, I'm surprised they don't suppress the animation.

>-  // The order here matters. When we exit full screen mode, we need to show the
>-  // Dock first, otherwise the newly-created window won't have its minimize
>-  // button enabled. See bug 526282.
>-  nsCocoaUtils::HideOSChromeOnScreen(aFullScreen, [mWindow screen]);
>-  nsresult rv = nsBaseWidget::MakeFullScreen(aFullScreen);
>+  nsresult rv = NS_OK;
>+
>+  // If we're using native fullscreen, then we will set mFullScreen and dispatch
>+  // the sizemode event from SetFullScreen.
>+  if (mUsesNativeFullScreen) {
>+    [mWindow toggleFullScreen:nil];
>+  } else {
>+    // The order here matters. When we exit full screen mode, we need to show the
>+    // Dock first, otherwise the newly-created window won't have its minimize
>+    // button enabled. See bug 526282.
>+    nsCocoaUtils::HideOSChromeOnScreen(aFullScreen, [mWindow screen]);
>+    rv = nsBaseWidget::MakeFullScreen(aFullScreen);
>+
>+    mFullScreen = aFullScreen;
>+    DispatchSizeModeEvent();

How about calling SetFullScreen from here? Then you could also remove all checks from SetFullScreen; the mFullScreen != aFullScreen isn't necessary because DispatchSizeModeEvent is already guarded against firing duplicate events.
That would reduce the differences between native and non-native fullscreen.

>+  }
>   NSEnableScreenUpdates();
>   NS_ENSURE_SUCCESS(rv, rv);

The NS_ENSURE_SUCCESS call can go into the non-native case, too (together with NSEnableScreenUpdates). Then you can also remove the nsresult rv; declaration at the top again.

> 
>-  mFullScreen = aFullScreen;
>-  DispatchSizeModeEvent();
>-
>-  return NS_OK;
>+  return rv;
> 
>   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
> }
> 
> 
> NS_IMETHODIMP nsCocoaWindow::Resize(PRInt32 aX, PRInt32 aY, PRInt32 aWidth, PRInt32 aHeight, bool aRepaint)
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
>@@ -1613,16 +1645,51 @@ void nsCocoaWindow::SetShowsToolbarButto
> {
>   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> 
>   [mWindow setShowsToolbarButton:aShow];
> 
>   NS_OBJC_END_TRY_ABORT_BLOCK;
> }
> 
>+void nsCocoaWindow::SetShowsFullScreenButton(bool aShow)
>+{
>+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>+
>+  if (!nsToolkit::OnLionOrLater() || mUsesNativeFullScreen == aShow) {
>+    return;
>+  }

Instead of nsToolkit::OnLionOrLater(), I'd like to have [mWindow respondsToSelector:@selector(toggleFullScreen:)] here. It won't make a difference in practice, but it's the basic principle of preferring feature testing over version testing.

>+
>+  // If the window is currently in fullscreen mode, then we're going to
>+  // transition out first, then set the collection behavior & toggle
>+  // mUsesNativeFullScreen, then transtion back into fullscreen mode. This
>+  // prevents us from getting into a conflicting state with MakeFullScreen
>+  // where mUsesNativeFullScreen would lead us down the wrong path.
>+  bool transition = mFullScreen;

How about "wasFullScreen" instead of "transition"?

>+// Lion's full screen mode will bypass our internal fullscreen tracking, so
>+// we need to catch it before we transition and call our own stuff, which in
>+// turn will fire "fullscreen" events. We catch the transition at "will" events
>+// because our "fullscreen" events are supposed to fire before the transition.

Where does it say that, and what difference does it make?

>diff --git a/xpfe/appshell/src/nsWebShellWindow.cpp b/xpfe/appshell/src/nsWebShellWindow.cpp

>           if (modeEvent->mSizeMode == nsSizeMode_Fullscreen) {
>             ourWindow->SetFullScreen(true);
>           }
>+          else if (inFullScreen) {
>+            ourWindow->SetFullScreen(false);
>+          }

Why wasn't this necessary before? Or was it, and nobody noticed?

I have another idea that would make the implementation between native and non-native more parallel. You could implement your own toggleFullScreen method in BaseWindow (our common subclass of NSWindow) which would in non-native fullscreen execute the "else" branch from MakeFullScreen (and in native fullscreen simply call [super toggleFullScreen:aSender]). In effect, we'd have the old implementation with the new API. You could even call windowWillEnter/ExitFullScreen from there (instead of calling SetFullScreen directly). Not sure if it's worth the effort, just an idea.
(In reply to Markus Stange from comment #75)
> Comment on attachment 594868 [details] [diff] [review]
> Patch v0.9 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)
> 
> >diff --git a/widget/cocoa/nsCocoaWindow.mm b/widget/cocoa/nsCocoaWindow.mm
> 
> >+NS_METHOD nsCocoaWindow::SetFullScreen(bool aFullScreen)
> >+{
> >+  // This method should only be called if we're using native fullscreen mode.
> >+  if (mUsesNativeFullScreen && mFullScreen != aFullScreen) {
> >+    // Since this isn't going through nsGlobalWindow::SetFullScreen, we need to
> >+    // set mFullScreen ourselves.
> >+    mFullScreen = aFullScreen;
> >+
> >+    DispatchSizeModeEvent();
> 
> Just to check if I've understood correctly: The call to
> DispatchSizeModeEvent will eventually call MakeFullScreen (through
> nsGlobalWindow::SetFullScreen), but that won't do anything because
> mFullScreen already has the new value. Right?

Right. That's why I swapped out the assert in MakeFullScreen.

> >+  }
> >+  return NS_OK;
> >+}
> > 
> > NS_METHOD nsCocoaWindow::MakeFullScreen(bool aFullScreen)
> > {
> >   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
> > 
> >-  NS_ASSERTION(mFullScreen != aFullScreen, "Unnecessary MakeFullScreen call");
> >+  // When using native fullscreen, when we enter/exit fullscreen via the toolbar
> >+  // button, we will bypass our usual checks. We will set mFullScreen and dispatch
> >+  // the sizemode event from there. nsGlobalWindow will handle that event as a
> >+  // normal sizemode event, so it will then call MakeFullScreen. Since we
> >+  // dispatched the event we don't need to handle it again, but we also don't
> >+  // need to ASSERT. We'll handle it gracefully instead.
> >+  if (mFullScreen == aFullScreen) {
> >+    return NS_OK;
> >+  }
> > 
> >   NSDisableScreenUpdates();
> 
> NSDisableScreenUpdates and NSEnableScreenUpdates should only be used in the
> non-native fullscreen case. Their purpose is to hide the multiple window
> resizes and content area shifts that happen during the transition.
> In the native fullscreen case, I'm surprised they don't suppress the
> animation.

That makes sense. They weren't breaking anything by leaving them in, so I did.

> >-  // The order here matters. When we exit full screen mode, we need to show the
> >-  // Dock first, otherwise the newly-created window won't have its minimize
> >-  // button enabled. See bug 526282.
> >-  nsCocoaUtils::HideOSChromeOnScreen(aFullScreen, [mWindow screen]);
> >-  nsresult rv = nsBaseWidget::MakeFullScreen(aFullScreen);
> >+  nsresult rv = NS_OK;
> >+
> >+  // If we're using native fullscreen, then we will set mFullScreen and dispatch
> >+  // the sizemode event from SetFullScreen.
> >+  if (mUsesNativeFullScreen) {
> >+    [mWindow toggleFullScreen:nil];
> >+  } else {
> >+    // The order here matters. When we exit full screen mode, we need to show the
> >+    // Dock first, otherwise the newly-created window won't have its minimize
> >+    // button enabled. See bug 526282.
> >+    nsCocoaUtils::HideOSChromeOnScreen(aFullScreen, [mWindow screen]);
> >+    rv = nsBaseWidget::MakeFullScreen(aFullScreen);
> >+
> >+    mFullScreen = aFullScreen;
> >+    DispatchSizeModeEvent();
> 
> How about calling SetFullScreen from here? Then you could also remove all
> checks from SetFullScreen; the mFullScreen != aFullScreen isn't necessary
> because DispatchSizeModeEvent is already guarded against firing duplicate
> events.
> That would reduce the differences between native and non-native fullscreen.

True... good idea.

> >+  }
> >   NSEnableScreenUpdates();
> >   NS_ENSURE_SUCCESS(rv, rv);
> 
> The NS_ENSURE_SUCCESS call can go into the non-native case, too (together
> with NSEnableScreenUpdates). Then you can also remove the nsresult rv;
> declaration at the top again.

Makes sense.

> >+void nsCocoaWindow::SetShowsFullScreenButton(bool aShow)
> >+{
> >+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
> >+
> >+  if (!nsToolkit::OnLionOrLater() || mUsesNativeFullScreen == aShow) {
> >+    return;
> >+  }
> 
> Instead of nsToolkit::OnLionOrLater(), I'd like to have [mWindow
> respondsToSelector:@selector(toggleFullScreen:)] here. It won't make a
> difference in practice, but it's the basic principle of preferring feature
> testing over version testing.

I guess this is the only place I ended up adding that, so it makes sense. Previously I was doing detection multiple times and OnLionOrLater was just more succinct. I'll change that.

> >+
> >+  // If the window is currently in fullscreen mode, then we're going to
> >+  // transition out first, then set the collection behavior & toggle
> >+  // mUsesNativeFullScreen, then transtion back into fullscreen mode. This
> >+  // prevents us from getting into a conflicting state with MakeFullScreen
> >+  // where mUsesNativeFullScreen would lead us down the wrong path.
> >+  bool transition = mFullScreen;
> 
> How about "wasFullScreen" instead of "transition"?

Sure.

> >+// Lion's full screen mode will bypass our internal fullscreen tracking, so
> >+// we need to catch it before we transition and call our own stuff, which in
> >+// turn will fire "fullscreen" events. We catch the transition at "will" events
> >+// because our "fullscreen" events are supposed to fire before the transition.
> 
> Where does it say that, and what difference does it make?

I noticed it in browser.js: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3924 Specifically, it takes !window.fullScreen to determine the direction we're transitioning.

Also, in nsGlobalWindow::SetFullScreen, the event is dispatched before mFullScreen is set.

Now that I'm looking at all that again, I guess it doesn't really matter that we use "will" instead of "did" events. Since we'll still end up dispatching the sizemode event triggering the fullscreen event, the flow is still the same. I'll leave it as will (and adjust the comment to be more accurate) unless there are objections.

> >diff --git a/xpfe/appshell/src/nsWebShellWindow.cpp b/xpfe/appshell/src/nsWebShellWindow.cpp
> 
> >           if (modeEvent->mSizeMode == nsSizeMode_Fullscreen) {
> >             ourWindow->SetFullScreen(true);
> >           }
> >+          else if (inFullScreen) {
> >+            ourWindow->SetFullScreen(false);
> >+          }
> 
> Why wasn't this necessary before? Or was it, and nobody noticed?

I don't think it was necessary before because the only way to enter/exit fullscreen mode was via our own code, which always called SetFullScreen(false). Now we're getting an event from OS X and (afaict) can't call nsGlobalWindow::SetFullScreen(false) directly.

> I have another idea that would make the implementation between native and
> non-native more parallel. You could implement your own toggleFullScreen
> method in BaseWindow (our common subclass of NSWindow) which would in
> non-native fullscreen execute the "else" branch from MakeFullScreen (and in
> native fullscreen simply call [super toggleFullScreen:aSender]). In effect,
> we'd have the old implementation with the new API. You could even call
> windowWillEnter/ExitFullScreen from there (instead of calling SetFullScreen
> directly). Not sure if it's worth the effort, just an idea.

That's not too unreasonable, but it's pretty much the same amount of misdirection. Also, then we would need to version check in SetShowsFullScreenButton since all windows would respond to toggleFullScreen, right? I think I'll leave it as is unless you're vehement about changing.
(In reply to Paul O'Shannessy [:zpao] from comment #76)
> Now that I'm looking at all that again, I guess it doesn't really matter
> that we use "will" instead of "did" events. Since we'll still end up
> dispatching the sizemode event triggering the fullscreen event, the flow is
> still the same.

Good, that's what I was wondering.

> I'll leave it as will (and adjust the comment to be more
> accurate) unless there are objections.

I thought I had an objection (or an opportunity for simplification), but now I realize that my idea won't work with non-native fullscreen. Specifically, I wanted to get rid of the argument to SetFullScreen and get the new state from the window's state mask (NSFullScreenWindowMask). Then we would have been able to move the whole SetFullScreen code into DispatchSizeModeEvent, and windowDidEnter/ExitFullScreen could have looked just like windowDidMiniaturize, for example (just another sizemode change source).
But nevermind.

> > >diff --git a/xpfe/appshell/src/nsWebShellWindow.cpp b/xpfe/appshell/src/nsWebShellWindow.cpp
> > 
> > >           if (modeEvent->mSizeMode == nsSizeMode_Fullscreen) {
> > >             ourWindow->SetFullScreen(true);
> > >           }
> > >+          else if (inFullScreen) {
> > >+            ourWindow->SetFullScreen(false);
> > >+          }
> > 
> > Why wasn't this necessary before? Or was it, and nobody noticed?
> 
> I don't think it was necessary before because the only way to enter/exit
> fullscreen mode was via our own code, which always called
> SetFullScreen(false). Now we're getting an event from OS X and (afaict)
> can't call nsGlobalWindow::SetFullScreen(false) directly.

Makes sense.

> > I have another idea that would make the implementation between native and
> > non-native more parallel. You could implement your own toggleFullScreen
> > method in BaseWindow (our common subclass of NSWindow) which would in
> > non-native fullscreen execute the "else" branch from MakeFullScreen (and in
> > native fullscreen simply call [super toggleFullScreen:aSender]). In effect,
> > we'd have the old implementation with the new API. You could even call
> > windowWillEnter/ExitFullScreen from there (instead of calling SetFullScreen
> > directly). Not sure if it's worth the effort, just an idea.
> 
> That's not too unreasonable, but it's pretty much the same amount of
> misdirection.

Yeah, I agree.

> Also, then we would need to version check in
> SetShowsFullScreenButton since all windows would respond to
> toggleFullScreen, right?

Right. (Unless we used the class version of the check, i.e. [NSWindow respondsToSelector:...].)

> I think I'll leave it as is unless you're vehement
> about changing.

OK.
nsCocoaWindow::SetFullScreen is a void now - I was just returning NS_OK from it all the time anyway & not using the result.(In reply to Markus Stange from comment #77)
> (In reply to Paul O'Shannessy [:zpao] from comment #76)
> > Now that I'm looking at all that again, I guess it doesn't really matter
> > that we use "will" instead of "did" events. Since we'll still end up
> > dispatching the sizemode event triggering the fullscreen event, the flow is
> > still the same.
> 
> Good, that's what I was wondering.

So the only potential problem I can see is if the window fails to transition into fullscreen mode. I haven't been able to make that happen in practice, but according to Apple's docs, it is possible. I guess in that case we'd be left in a bad state. IIRC, "did" methods are only called on successful transitions (there are "failedTo" methods we can implement too, but gross). So maybe using "did" methods is best... what do you think?
(In reply to Paul O'Shannessy [:zpao] from comment #78)
> > > Now that I'm looking at all that again, I guess it doesn't really matter
> > > that we use "will" instead of "did" events. Since we'll still end up
> > > dispatching the sizemode event triggering the fullscreen event, the flow is
> > > still the same.
> > 
> > Good, that's what I was wondering.
> 
> So the only potential problem I can see is if the window fails to transition
> into fullscreen mode. I haven't been able to make that happen in practice,
> but according to Apple's docs, it is possible. I guess in that case we'd be
> left in a bad state. IIRC, "did" methods are only called on successful
> transitions (there are "failedTo" methods we can implement too, but gross).
> So maybe using "did" methods is best... what do you think?

Good points. If it doesn't make a difference in your tests, using the "did" methods is probably better. Does it affect the transition in any way? The fullscreen event hides the toolbars, so are they already hidden during the transition when you use the "will" methods?
The Whiteboard should be edited to include [parity-safari;chrome;opera]
You're lagging, ff.
Comment on attachment 594868 [details] [diff] [review]
Patch v0.9 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)

Review of attachment 594868 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsCocoaWindow.mm
@@ +1159,5 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
>  }
>  
> +NS_METHOD nsCocoaWindow::SetFullScreen(bool aFullScreen)

Maybe call this "SetNativeFullScreen" to clarify the context?

@@ +1192,5 @@
> +
> +  // If we're using native fullscreen, then we will set mFullScreen and dispatch
> +  // the sizemode event from SetFullScreen.
> +  if (mUsesNativeFullScreen) {
> +    [mWindow toggleFullScreen:nil];

I assume that we'll get a call from the OS about entering fullscreen, and we'll set mFullScreen properly when that happens. You might want to note that here so it doesn't look like an oversight.
Attachment #594868 - Flags: review?(joshmoz) → review+
(In reply to Markus Stange from comment #79)
> (In reply to Paul O'Shannessy [:zpao] from comment #78)
> > > > Now that I'm looking at all that again, I guess it doesn't really matter
> > > > that we use "will" instead of "did" events. Since we'll still end up
> > > > dispatching the sizemode event triggering the fullscreen event, the flow is
> > > > still the same.
> > > 
> > > Good, that's what I was wondering.
> > 
> > So the only potential problem I can see is if the window fails to transition
> > into fullscreen mode. I haven't been able to make that happen in practice,
> > but according to Apple's docs, it is possible. I guess in that case we'd be
> > left in a bad state. IIRC, "did" methods are only called on successful
> > transitions (there are "failedTo" methods we can implement too, but gross).
> > So maybe using "did" methods is best... what do you think?
> 
> Good points. If it doesn't make a difference in your tests, using the "did"
> methods is probably better. Does it affect the transition in any way? The
> fullscreen event hides the toolbars, so are they already hidden during the
> transition when you use the "will" methods?

We're not going to hide toolbars on Lion except for domfullscreen (see part 3), but without that applied there's a noticeable difference - they aren't hidden yet, but it does seem like the transition is in progress. Waiting for "did" is more consistent with what do now I think (resize, then timeout for animation starts). DOM fullscreen is a little jarring since we wait for the transition, then hide the chrome bits (but it seems like that's happening both ways, so meh)

Also, it looks like my changes to nsWebShellWindow.cpp aren't going to be necessary since somebody else just did essentially the same thing (for handling naive fullscreen events from GTK in bug 512529)

(In reply to Josh Aas (Mozilla Corporation) from comment #81)
> > +NS_METHOD nsCocoaWindow::SetFullScreen(bool aFullScreen)
> 
> Maybe call this "SetNativeFullScreen" to clarify the context?

Yea, the name had been bugging me too, though SetNativeFullScreen doesn't feel quite right either. SetFullScreen actually feels a lot better now that it's the only place where mFullScreen is set (following Markus's comments above). I think I'll leave it as is now.

> > +  // If we're using native fullscreen, then we will set mFullScreen and dispatch
> > +  // the sizemode event from SetFullScreen.
> > +  if (mUsesNativeFullScreen) {
> > +    [mWindow toggleFullScreen:nil];
> 
> I assume that we'll get a call from the OS about entering fullscreen, and
> we'll set mFullScreen properly when that happens. You might want to note
> that here so it doesn't look like an oversight.

I'll add more to that comment to clarify.

There's a new patch coming today, though the changes are not too major. I'll ask for review again just to make sure that r+ holds
quick breakdown of changes:
* simplified SetFullScreen
* using SetFullScreen from MakeFullScreen (keeping it DRY)
* SetFullScreen is a void now - I was just returning NS_OK from it all the time anyway & never using the result
* Moved more code from general case in MakeFullScreen to apply specifically to non-native fullscreen
* using "did" methods instead of "will"
  * also not using OBJC_TRY blocks because I shouldn't need them (and other similar methods don't use them)
* nsWebShellWindow changes were made elsewhere (see bug 512529)
* adjusted some comments
Attachment #594868 - Attachment is obsolete: true
Attachment #594868 - Flags: review?(mstange)
Attachment #596812 - Flags: review?(joshmoz)
Comment on attachment 596812 [details] [diff] [review]
Patch v0.10 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)

Another change I forgot to mention: I'm now using respondsToSelector instead of version checking in SetShowsFullScreenButton
Attachment #596812 - Flags: review?(mstange)
Comment on attachment 596812 [details] [diff] [review]
Patch v0.10 (Part 2: Support Lion Fullscreen when fullscreenbutton=true)

Review of attachment 596812 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsCocoaWindow.mm
@@ +1159,5 @@
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
>  }
>  
> +void nsCocoaWindow::SetFullScreen(bool aFullScreen)

This method name makes it sound like if you call it you'll cause us to enter fullscreen mode. Tough to get these names as clear as possible, how about "EnteredFullScreen"? That makes it sound more like you're giving the object the opportunity to update state, which is what you're doing.
Attachment #596812 - Flags: review?(joshmoz) → review+
Attachment #596812 - Flags: review?(mstange) → review+
Comment on attachment 587879 [details] [diff] [review]
Patch v0.7 (Part 3: turn it on & handle in browser)

Could you put "if (this.useLionFullScreen) { ...; return; }" before the existing if/else and leave the latter alone? Should make it easier to comprehend this code.
Attachment #587879 - Flags: review?(dao) → review-
I like that request. It keeps the changes here to a minimum.
Attachment #587879 - Attachment is obsolete: true
Attachment #597558 - Flags: review?(dao)
Blocks: 703724
No longer depends on: 703724
Comment on attachment 597558 [details] [diff] [review]
Patch v0.8 (Part 3: turn it on & handle in browser)

>+      if (document.mozFullScreen)
>+        this.showXULChrome("toolbar", false);

You also need to call this.mouseoverToggle(false), don't you?
(In reply to Dão Gottwald [:dao] from comment #88)
> Comment on attachment 597558 [details] [diff] [review]
> Patch v0.8 (Part 3: turn it on & handle in browser)
> 
> >+      if (document.mozFullScreen)
> >+        this.showXULChrome("toolbar", false);
> 
> You also need to call this.mouseoverToggle(false), don't you?

mouseoverToggle(false) will get called from enterDomFullScreen (which gets called after toggle
But a few lines below, toggle does call mouseoverToggle for DOM FS. Should it not do that?
(In reply to Dão Gottwald [:dao] from comment #90)
> But a few lines below, toggle does call mouseoverToggle for DOM FS. Should
> it not do that?

The DOM FS call is probably extraneous - I guess it'll hide chrome slightly sooner. We could move mouseoverToggle into the if(!DOM FS) block, but for after first fullscreen enter, we would re-add a listener (default: animateUp == 1), though we could just add that above the if block and it would get removed in mouseoverToggle.

All-in-all, I think this would benefit from some cleanup/refactoring but maybe we should just file a new bug for that.
(In reply to Paul O'Shannessy [:zpao] from comment #91)
> All-in-all, I think this would benefit from some cleanup/refactoring but
> maybe we should just file a new bug for that.

Alright, could you please file that bug then?
Attachment #597558 - Flags: review?(dao) → review+
Woot! Awesome work Paul, can't wait to see this in Nightly :)
(In reply to Dão Gottwald [:dao] from comment #92)
> (In reply to Paul O'Shannessy [:zpao] from comment #91)
> > All-in-all, I think this would benefit from some cleanup/refactoring but
> > maybe we should just file a new bug for that.
> 
> Alright, could you please file that bug then?

Filed bug 729638.

I haven't pushed this yet because there were some test failures in the domfullscreen tests on 10.7 only (99.9% sure it was due to timing with the transition). I missed them when I'd pushed to try previously. I've fixed them with a setTimeout to give the animation time to run (executeSoon isn't good enough because it's not our event loop we're waiting on).
Like I mentioned in comment #94, there were some test failures on Lion. This has fixed it locally for me (apart from when the tests crash, but that's a known orange - bug 728962). The length of the added timeout was pretty arbitrary but I figured long enough to ensure we wouldn't run into issues with the animation.

Try results eventually at https://tbpl.mozilla.org/?tree=Try&rev=b80cf65567d7
Attachment #600541 - Flags: review?(cpearce)
Attachment #600541 - Attachment is patch: true
Comment on attachment 600541 [details] [diff] [review]
Patch v1.0 (Part 4: fix content tests)

This is going to fail randomly. Is there a way we can delay sending the mozfullscreenchange event until after the animation has finished, and use a mozfullscreenchange listener instead of the timeout here.

Delaying mozfullscreenchange until after the animation is desirable anyway, as it means the event won't be received until the transition is complete, and we're ready for script to start rendering. Situations like this is the reason why requestFullScreen() is asynchronous.
(In reply to Chris Pearce (:cpearce) from comment #99)
> Is there a way we can delay sending the
> mozfullscreenchange event until after the animation has finished, and use a
> mozfullscreenchange listener instead of the timeout here.
> 
> Delaying mozfullscreenchange until after the animation is desirable anyway,
> as it means the event won't be received until the transition is complete,
> and we're ready for script to start rendering. Situations like this is the
> reason why requestFullScreen() is asynchronous.

The comment at
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8976 makes me think that it's not really doable (not without a very careful balancing act).

Instead I reworked how the dom fullscreen tests ran. It's not a major change but it seems to have done the trick without having to resort to setTimeout. It may still cause orange (I remember some funkiness with waitForFocus), but I think I may have fixed some orange in the process (less setTimeout!).
Attachment #601464 - Flags: review?(cpearce)
Comment on attachment 601464 [details] [diff] [review]
Patch v1.1 (Part 4: fix content tests)

Review of attachment 601464 [details] [diff] [review]:
-----------------------------------------------------------------

If you haven't already, make sure you do a full try run with these changes before landing, I recall Linux in particular being quite fussy about focus while running these tests.

::: content/html/content/test/file_fullscreen-navigation.html
@@ +37,3 @@
>    frameWin = document.getElementById("f").contentWindow;
>    e1 = frameWin.document.body;
> +  document.addEventListener("mozfullscreenchange", function onfullscreen() {

No need to name the function onfullscreen.
Attachment #601464 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #101)
> Comment on attachment 601464 [details] [diff] [review]
> Patch v1.1 (Part 4: fix content tests)
> 
> Review of attachment 601464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you haven't already, make sure you do a full try run with these changes
> before landing, I recall Linux in particular being quite fussy about focus
> while running these tests.

Yea, Linux & focus are still not 100% there. It's been a while since I've hit issues but bug 521233 was... fun. And it's already in the queue :) https://tbpl.mozilla.org/?tree=Try&rev=3f7416c92d79

> ::: content/html/content/test/file_fullscreen-navigation.html
> @@ +37,3 @@
> >    frameWin = document.getElementById("f").contentWindow;
> >    e1 = frameWin.document.body;
> > +  document.addEventListener("mozfullscreenchange", function onfullscreen() {
> 
> No need to name the function onfullscreen.

It could have been named whatever, it just makes it easy to remove the event listener from within - These days I prefer it over arguments.callee.
Comment on attachment 601464 [details] [diff] [review]
Patch v1.1 (Part 4: fix content tests)

Bah, this did not go completely as planned. Linux wasn't happy so I'm going to have to play with this a bit more. 10.7 wasn't entirely happy either :/
(In reply to Paul O'Shannessy [:zpao] from comment #40)
> * Our keyboard shortcut doesn't match (I'm not sure if it ever did & don't
> have 10.6 installed to check) - ctrl+cmd+f seems to be pretty standard now,
> we do shift+cmd+f
Sorry, maybe a bit offtopic, but where in the code is this shortcut defined. I've searched the code but couldn't find it...
(In reply to Nomis101 from comment #104)
> Sorry, maybe a bit offtopic, but where in the code is this shortcut defined.
> I've searched the code but couldn't find it...

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#329

To find it I searched for "Full Screen" on MXR, found the fullScreenCmd entity in the browser dtd file, then searched for that.
(In reply to Nomis101 from comment #104)
> (In reply to Paul O'Shannessy [:zpao] from comment #40)
> > * Our keyboard shortcut doesn't match (I'm not sure if it ever did & don't
> > have 10.6 installed to check) - ctrl+cmd+f seems to be pretty standard now,
> > we do shift+cmd+f
> Sorry, maybe a bit offtopic, but where in the code is this shortcut defined.
> I've searched the code but couldn't find it...

The patch in bug 714172 has it all (and more), but the short answer is: the key & label are defined in browser.dtd and the modifiers are defined in browser-menubar.inc.
Thanks!
Attachment #600541 - Attachment is obsolete: true
Attachment #600541 - Flags: review?(cpearce)
After too many attempts at this, I tweaked this a bit further, but try still wouldn't go green. So I added the timeout back in _for Lion only_. The other option was disabling the test on Lion, and I really didn't want to do that.
Attachment #601464 - Attachment is obsolete: true
Flags: in-litmus?
Depends on: 737752
Whiteboard: [parity-safari][inbound] → [parity-safari]
Depends on: 738335
dev-doc-needed: the fullscreenbutton attribute.
Keywords: dev-doc-needed
Documented:

https://developer.mozilla.org/en/XUL/Attribute/fullscreenbutton

And mentioned on Firefox 14 for developers.
Keywords: dev-doc-needed → dev-doc-complete
I found a minor annoyance: Firefox doesn't maintain fullscreen mode when you quit Firefox in fullscreen mode and open it again. Instead it opens in a fully maximized mode even though I have Firefox set so that it doesn't take up all of the screen.
Blocks: 738335
No longer depends on: 738335
Blocks: 738993
Blocks: 738995
Depends on: 739012
Paul: Can we listen for the NSWindowDidEnterFullScreenNotification notification on OSX to determine when the transition to fullscreen animation is finished (in order to fix the orange we have)? i.e. does NSWindowDidEnterFullScreenNotification fire after the fullscreen animation has finished?

(Just put aside the fact we'd have to refactor how to do fullscreen in order to take advantage of this, I just want to know if it's possible on the platform side.)
Paul: Or can we fix the orange by changing your timeouts to check window.innerWidth and window.innerHeight, and reset the timeout if the window dimensions don't correspond to the anticipated fullscreen/normal mode screen sizes?
(In reply to Chris Pearce (:cpearce) from comment #114)
> Paul: Can we listen for the NSWindowDidEnterFullScreenNotification
> notification on OSX to determine when the transition to fullscreen animation
> is finished (in order to fix the orange we have)? i.e. does
> NSWindowDidEnterFullScreenNotification fire after the fullscreen animation
> has finished?

We're implementing windowDid(Enter|Exit)FullScreen right now, so that's exactly what we're doing. I'm not 100% certain, but I think that's called after the animation is complete (Apple docs don't say that explicitly - "The window just entered fullscreen mode").

So we may be able to cleanup the test by waiting for "fullscreen" to fire instead of domfullscreen. I think I tried a variation of that for cleaning up at the end of a test though and it wasn't as successful as I'd hoped. I didn't make that change globally to the tests but doing that might make a difference.

(In reply to Chris Pearce (:cpearce) from comment #115)
> Paul: Or can we fix the orange by changing your timeouts to check
> window.innerWidth and window.innerHeight, and reset the timeout if the
> window dimensions don't correspond to the anticipated fullscreen/normal mode
> screen sizes?

The orange is only happening with a single test so it doesn't seem to (directly) be a failure of the timeout I added. The timeout is following a window close anyhow. But we could probably do something like you're suggesting.

The failing test uses executeSoon(executeSoon(... to wait for plugins to load, which I guess isn't happening consistently. Could be that the animation is disrupting something there. Perhaps there are events we can wait for before starting instead of that timeout?
Keywords: relnote
Blocks: 740148
No longer blocks: 740148
Depends on: 740148
No longer blocks: 738993
Hmm this also opens popups, like for example BrowserID, in a new full screen window. In safari those open in a new tab and in Chrome they actually open in a new window that floats on top of the full screen parent. Not sure which I prefer (I *think* the Chrome behaviour) but what Firefox does now is certainly not the best solution I think.
(In reply to Stefan Arentz [:st3fan] from comment #117)
> Hmm this also opens popups, like for example BrowserID, in a new full screen
> window. In safari those open in a new tab and in Chrome they actually open
> in a new window that floats on top of the full screen parent. Not sure which
> I prefer (I *think* the Chrome behaviour) but what Firefox does now is
> certainly not the best solution I think.

Let's get this into a new bug to discuss. I agree that our behavior right now isn't really ideal.
did anyone noticed that using personas keeps an extra space (I guess it's the titlebar) when using Fullscreen mode?
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120404 Firefox/14.0a1
attached screenshots

personas applied fullscreen https://skitch.com/worg/8xhmd/fullscreen-persona

default theme (no persona applied) fullscreen https://skitch.com/worg/8xhmx/fullscreen-default
(In reply to Hiram J. Perez from comment #119)
> did anyone noticed that using personas keeps an extra space (I guess it's
> the titlebar) when using Fullscreen mode?

Thanks for letting us know Hiram. I filed that as bug 716450 a little while back and it may end up getting fixed by bug 714186 since the work is so similar.
I knew it just after my comment, I forgot to use the search option, thanks.
No longer blocks: 714172
Blocks: 714172
I just updated to FF13 under Lion 10.7.3, and now when I hit Cmd-Shift-F, instead of the FF window becoming larger to fill the full screen, it disappears for an instant, all other app windows are visible for about 200ms, then it animates a zoom from the center of the screen.

I find this behavior quite distracting and annoying -- I'm used to hitting Cmd-Shift-F regularly when in FF and I would prefer not to have the visiual distraction.

This this a result of this bug? Is it deliberate? I'm having a hard time telling.

Thanks.
> This this a result of this bug?

No, this bug is not included in Firefox 13, but Firefox 14. Your issue is unrelated to this bug.
Depends on: 773375
Depends on: 774677
In fullscreen, we don't see personnal bar (for favorites)...bug ?
> In fullscreen, we don't see personnal bar (for favorites)...bug ?

I don't understand.

Is what you don't see listed in View : Toolbars?
I chechked View: Toolbars: "Personnal ToolBar" (not sur of the traduction because in french it's "Barre personnelle")

But in fullscreen mode i don't see this toolbar
In my en_US copy of Firefox 14.0.1, running without extensions, I see the following three toolbars under View : Toolbars:

Navigation Toolbar
Bookmarks Toolbar
Add-on Toolbar

Only the first (the Navigation Toolbar) is selected by default.  I see it in fullscreen mode.  But I also see the others in fullscreen mode if I've selected them (under View : Toolbars).

I suspect your "Barre personnelle" comes from an extension.  It'd help if you could tell us which extension.
Now autohide is enabled, this bug 594681 has appeared making fullscreen useless for me.
(In reply to comment #130)

I frankly don't understand what you're saying.  Please be more specific.
(Following up comment #129)

Jean-François, are you running on OS X 10.7.X (or above)?

If not, then this is the wrong bug.  This bug is about support for the Lion-style fullscreen mode on OS X 10.7 (and up).
Bookmarks Toolbar does not work in fullscreen any more.
(In reply to Simon Howes from comment #133)
> Bookmarks Toolbar does not work in fullscreen any more.

I can't reproduce this problem. I still have a Bookmarks Toolbar in fullscreen.
No longer depends on: 774677
Jean-François and Simon, are you by any chance using OS X 10.8 (Mountain Lion)?

Thanks to a stupid little bug, the Bookmarks Toolbar and Add-on Toolbar *aren't* visible on OS X 10.8:  On Mountain Lion we use an odd combination of the Lion fullscreen mode and the "old" fullscreen mode (aka presentation mode).  Neither toolbar is visible in the "old" fullscreen mode, even when you make them visible in View : Toolbars.
> Neither toolbar is visible in the "old" fullscreen mode, even when
> you make them visible in View : Toolbars.

I've opened bug 775314 on this.
Depends on: 774685
(Following up comment #135)

This is bug 775325, which is already fixed in current trunk code, and will be fixed in FF 15, but isn't yet fixed in FF 14.0.1.
Blocks: 839888
Depends on: 910144
Depends on: 854743
Keywords: relnote
> Disabled for 10.10

You mean mochitests for this are now disabled on OS X 10.10.  They were already disabled on 10.7 and 10.8.  Apparently, though, they aren't disabled on 10.9.  Do you know why, Phil?
Flags: needinfo?(philringnalda)
I was puzzled by that, too, since according to http://mxr.mozilla.org/mozilla-central/search?string=isosxmavericks I did do a couple of others. My best guess is that for 10.9, I pretended that developers would deal with greening up tests, and I only did the ones where I... knew there was no hope? couldn't see other failures behind their killing of their respective suites? Not sure, it's been more than a year.

At any rate, the 10.9 plan clearly didn't work out very well, so I think the 10.10 plan is likely to be more of a "file and almost immediately disable" so if there are tests failing in https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&filter-machine_name=yosemite that you don't want to completely lose coverage for on > 10.6, diving in to fixing them would be a good idea.
Flags: needinfo?(philringnalda)
(In reply to comment #142)

Thanks for the info.

I've known for some time that our OS X native fullscreen support needs a full rewrite.  But I doubt I'll ever have the time for it.  It helps that its performance on mochitests is *much* worse that it is in real life.  The reason is that mochitests need to happen quickly, and wrt native fullscreen mode they generally go on to the next step before the previous step is actually finished.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: