in order to support fullscreen mode on windows mobile, we need to implement the method MakeFullScreen using the windows mobile shell apis. There is no real change for win32 in doing so.

incorporates patch v.1. This adds a new sizemode to xul windows so that you can specify "fullscreen". It has the same semantics as maximized but tells the platform to call MakeFullScreen() on the window. The only widget that is hooked up is Windows. Btw, the reason that we can just make the nsXULWindow to call MakeFullScreen is that the widget might have to do interesting things to keep the top level window fullscreen. For example, the windows implementation needs to force the top level window to be fullscreen when the user switches back to it. I would imagine other implementation need to do similar things.

Comment on attachment 368907 [details] [diff] [review] patch v.3 This is ok, but can you make the same change to the gtk2 code for hildon? We treat Maximized as fullscreen there for now.

hey vlad, here are the follow up bugs for the tier 1 platforms: mac - 484833 gtk2 - 484832 I will implemented these such that fullscreen just equals maximized unless MakeFullScreen is implemented.

Comment on attachment 368907 [details] [diff] [review] patch v.3 > case nsSizeMode_Maximized: >+ case nsSizeMode_Fullscreen: /* do we really need a STATE_FULLSCREEN? */ > *aWindowState = nsIDOMChromeWindow::STATE_MAXIMIZED; we probably do so the xul app knows to provide extra essential window chrome. > #ifdef WINCE_WINDOWS_MOBILE >+ > // on windows mobile, dialogs and top level windows are full screen > // This is partly due to the lack of a GetWindowPlacement. nit, ws >+} >+ >+ >+NS_IMETHODIMP >+nsWindow::MakeFullScreen(PRBool aFullScreen) >+{ file style appears to be only one line between functions >+#if WINCE >+ RECT rc; >+ if (aFullScreen) { >+ SHFullScreen(mWnd, >+ SHFS_HIDETASKBAR | SHFS_HIDESTARTICON); this can be on one line >+ SHFullScreen(mWnd, >+ SHFS_SHOWTASKBAR | SHFS_SHOWSTARTICON); same

jst, can you also give this a review?

this tweaks session store so that fullscreen will behave like maximized.

How does this interact with window.fullScreen?

hey roc, this should play nice with window.fullscreen, however, it is clear that I am not setting the right bit to make window.fullScreen reflect our current state. Aside from this, do you foresee any other issues? Doug

No, it makes sense. We just need to make sure those things stay synchronized.

makes window.fullScreen return the current value.

Comment on attachment 368986 [details] [diff] [review] patch v.6 changing review. jst please slap my hand and take it back if you want.

Comment on attachment 368986 [details] [diff] [review] patch v.6 >diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp >--- a/dom/base/nsGlobalWindow.cpp >+++ b/dom/base/nsGlobalWindow.cpp >@@ -3889,16 +3889,22 @@ nsGlobalWindow::GetFullScreen(PRBool* aF > treeItem->GetRootTreeItem(getter_AddRefs(rootItem)); > if (rootItem != treeItem) { > nsCOMPtr<nsIDOMWindowInternal> window = do_GetInterface(rootItem); > if (window) > return window->GetFullScreen(aFullScreen); > } > } > >+ // Sync the fullscreen flag >+ nsCOMPtr<nsIWidget> widget = GetMainWidget(); >+ PRInt32 mode; >+ if (widget && NS_SUCCEEDED(widget->GetSizeMode(&mode))) >+ mFullScreen = mode == nsSizeMode_Fullscreen; >+ So you update mFullScreen only if someone asks the value of window.fullScreen. Yet in many places in nsGlobalWindow it is expected that mFullScreen has the right value. When window.fullScreen is set to true, we dispatch "fullscreen" event. Does that happen with the patch? "fullscreen" event is handled here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1198 The patch adds support for fullscreen persistence, right? If the event isn't dispatched, I guess Firefox chrome will look quite wrong, when starting in fullscreen mode.

@smaug thanks for the feedback. i can try dispatching the event. However, i worry about waiting for the event to resize the native window as anything that effects startup performance is bad. It might just be i can remove the code in the fullScreen getter, and instead just post a message from the nsXULWindow?

Comment on attachment 368986 [details] [diff] [review] patch v.6 >+NS_IMETHODIMP >+nsWindow::MakeFullScreen(PRBool aFullScreen) >+{ >+#if WINCE >+ RECT rc; >+ if (aFullScreen) { >+ SetForegroundWindow(mWnd); >+ SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON); >+ >+ SetRect(&rc, 0, 0, GetSystemMetrics(SM_CXSCREEN), GetSystemMetrics(SM_CYSCREEN)); >+ } >+ else { >+ SHFullScreen(mWnd, SHFS_SHOWTASKBAR | SHFS_SHOWSTARTICON); >+ SystemParametersInfo(SPI_GETWORKAREA, 0, &rc, FALSE); >+ } >+ MoveWindow(mWnd, rc.left, rc.top, rc.right-rc.left, rc.bottom-rc.top, TRUE); >+ return NS_OK; >+#else >+ return nsBaseWidget::MakeFullScreen(aFullScreen); >+#endif > } This is not right. nsBaseWidget::MakeFullScreen calls nsIFullscreen service when needed. You prevent that for wince. But, gtk2's ::MakeFullScreen is wrong too, a regression from bug 301402. Do we need nsIFullscreen for anything? Minimo used to use it, but can't find anything in mozilla-central. r- because either nsIFullScreen needs to be removed or its handling fixed. And mFullScreen handling in nsGlobalWindow doesn't look right. I think we could remove nsIFullscreen *and* mFullScreen. nsGlobalWindow::GetFullScreen could always return widget's sizemode == nsSizeMode_Fullscreen. But we still need to dispatch "fullscreen" event when we enter or leave fullscreen. Roc, any comments? And this all needs some tests! (at least some basic tests for "fullscreen" event)

See http://markmail.org/message/5sjfdbcrqmyjtql3#query:nsifullscreen+page:1+mid:fin2u6bhhlulgguo+state:results Let's remove nsIFullScreen and mFullScreen as you suggest.

try builds: https://build.mozilla.org/tryserver-builds/2009-03-29_16:50-dougt@mozilla.com-try-aa871492653/

Comment on attachment 369934 [details] [diff] [review] patch v.7 >+ // Sync the fullscreen flag Nit, you're not syncing any flag here. > NS_IMETHODIMP nsBaseWidget::MakeFullScreen(PRBool aFullScreen) > { > HideWindowChrome(aFullScreen); > >- nsCOMPtr<nsIFullScreen> fullScreen = do_GetService("@mozilla.org/browser/fullscreen;1"); >- >- if (aFullScreen) { >- if (!mOriginalBounds) >- mOriginalBounds = new nsIntRect(); >- GetScreenBounds(*mOriginalBounds); >- >- // Move to top-left corner of screen and size to the screen dimensions >- nsCOMPtr<nsIScreenManager> screenManager; >- screenManager = do_GetService("@mozilla.org/gfx/screenmanager;1"); >- NS_ASSERTION(screenManager, "Unable to grab screenManager."); >- if (screenManager) { >- nsCOMPtr<nsIScreen> screen; >- screenManager->ScreenForRect(mOriginalBounds->x, mOriginalBounds->y, >- mOriginalBounds->width, mOriginalBounds->height, >- getter_AddRefs(screen)); >- if (screen) { >- PRInt32 left, top, width, height; >- if (NS_SUCCEEDED(screen->GetRect(&left, &top, &width, &height))) { >- SetSizeMode(nsSizeMode_Normal); >- Resize(left, top, width, height, PR_TRUE); >- >- // Hide all of the OS chrome >- if (fullScreen) >- fullScreen->HideAllOSChrome(); >- } >- } >- } >- >- } else if (mOriginalBounds) { >+ if (mOriginalBounds) { > Resize(mOriginalBounds->x, mOriginalBounds->y, mOriginalBounds->width, > mOriginalBounds->height, PR_TRUE); So why you remove all that screen manager code? It was added there on purpose, see bug 127575. >+ // Dispatch fullscreen event ... Could you make some helper method for "fullscreen" and "windowZLevel" events. I wonder how "fullscreen" should work here. In nsGlobalWindow is it cancellable and dispatched before setting the widget to fullscreen mode. Could you do similar here?

Comment on attachment 369934 [details] [diff] [review] patch v.7 r- because of the missing helper method for event dispatch, and would like get at least some explanation why the screen manager handling can be removed.

1) allows for cancelable fullscreen events when using sizemode=fullscreen. 2) factors custom event dispatching code in nsXULWindow

Comment on attachment 370529 [details] [diff] [review] patch v.8 >+ // Sync the fullscreen flag You have still this. IMO, no flag is "synced" here. Seems like a leftover from earlier patch. > // the widget had better be able to deal with not becoming visible yet > mWindow->SetSizeMode(sizeMode); >+ >+ // Dispatch fullscreen event >+ if (sizeMode == nsSizeMode_Fullscreen) { >+ if (!DispatchCustomEvent(NS_LITERAL_STRING("fullscreen"), PR_TRUE)) { >+ // fullscreen event prevented the default, set the window to >+ // maximized instead of fullscreen. >+ sizeMode = nsSizeMode_Maximized; >+ mWindow->SetSizeMode(sizeMode); >+ } >+ } Hmm, actually, nsGlobalWindow dispatches the event just *before* the value of window.fullScreen will change. You should probably do the same here. Maybe you need to modify testcases a bit too. >+PRBool nsXULWindow::DispatchCustomEvent(const nsAString& eventName, PRBool cancelable) "fullscreen" should be dispatched to |window|, "windowZLevel" to |document|, so maybe this method could have 3rd parameter to indicate the target. With those, r=me

the handler to the fullscreen will now see window.fullScreen as false. This is probably expected because it is a cancelable event. smaug, could you give this one more look since I changed the event dispatching a bit.

Comment on attachment 370682 [details] [diff] [review] patch v.9 >+ if (toDocument) { >+ nsCOMPtr<nsIDOMEventTarget> targ(do_QueryInterface(doc)); >+ if (targ) { >+ PRBool defaultActionEnabled; >+ targ->DispatchEvent(event, &defaultActionEnabled); >+ return defaultActionEnabled; >+ } >+ } else { >+ nsCOMPtr<nsIDOMWindowInternal> ourWindow; >+ GetWindowDOMWindow(getter_AddRefs(ourWindow)); >+ nsCOMPtr<nsIDOMEventTarget> targ(do_QueryInterface(ourWindow)); >+ if (targ) { >+ PRBool defaultActionEnabled; >+ targ->DispatchEvent(event, &defaultActionEnabled); >+ return defaultActionEnabled; >+ } You could simplify this a bit. Something like nsCOMPtr<nsIDOMEventTarget> target; if (toDocument) { target = do_QueryInterface(doc); } else { nsCOMPtr<nsIDOMWindowInternal> ourWindow; GetWindowDOMWindow(getter_AddRefs(ourWindow)); target = do_QueryInterface(ourWindow); } if (target) { PRBool defaultActionEnabled; target->DispatchEvent(event, &defaultActionEnabled); return defaultActionEnabled; }

Comment on attachment 370682 [details] [diff] [review] patch v.9 can you sr?

jst sr ping?

Comment on attachment 370682 [details] [diff] [review] patch v.9 roc, can you sr?

I think Olli should sr, that seems most efficient.

roc, olli reviewed. i would like additional eyes on this patch.

Olli's a much better reviewer than I am in general, is there something specific you want me to look at?

not really. just want my r/sr. The big piece is removing the nsIFullScreen interface.

@@ -1558,22 +1558,32 @@ NS_METHOD nsWindow::Show(PRBool bState) #ifdef WINCE + case nsSizeMode_Fullscreen: + ::SetForegroundWindow(mWnd); + ::ShowWindow(mWnd, SW_SHOWMAXIMIZED); + MakeFullScreen(TRUE); + break; #else + case nsSizeMode_Fullscreen: + ::ShowWindow(mWnd, SW_SHOWMAXIMIZED); + break; Why do you call MakeFullScreen on wince but not on normal Windows? It seems to me that it's not necessary on wince either because it will get called on the WM_ACTIVATE event. Is that correct? @@ -1693,29 +1703,34 @@ NS_IMETHODIMP nsWindow::SetSizeMode(PRIn This method doesn't have any call to MakeFullScreen. Is it guaranteed that a WM_ACTIVATE event will follow after SetSizeMode? Or am I missing something? +nsWindow::MakeFullScreen(PRBool aFullScreen) + SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON); + SHFullScreen(mWnd, SHFS_SHOWTASKBAR | SHFS_SHOWSTARTICON); + SystemParametersInfo(SPI_GETWORKAREA, 0, &rc, FALSE); The rest of the file uses :: before these system function calls. It looks like you're reimplementing the window rect thing that nsBaseWidget::MakeFullScreen already does. Why don't you re-use that code? (Maybe because it's not guaranteed that the original window rect is the one that GETWORKAREA returns? Just asking.) @@ -4894,17 +4934,17 @@ PRBool nsWindow::ProcessMessage(UINT msg if (pl.showCmd == SW_SHOWMAXIMIZED) event.mSizeMode = nsSizeMode_Maximized; else if (pl.showCmd == SW_SHOWMINIMIZED) event.mSizeMode = nsSizeMode_Minimized; else event.mSizeMode = nsSizeMode_Normal; #else - event.mSizeMode = nsSizeMode_Normal; + event.mSizeMode = mSizeMode; #endif Was there a reason that you set it to nsSizeMode_Normal in bug 464091? And do you know why the non-wince code doesn't just use mSizeMode? diff --git a/xpfe/appshell/src/nsXULWindow.h b/xpfe/appshell/src/nsXULWindow.h PRInt32 AppUnitsPerDevPixel(); + PRBool DispatchCustomEvent(const nsAString& eventName, PRBool cancelable = PR_FALSE, PRBool toDocument = PR_TRUE); OK, 3-spaces-indent is funny, but I think you should still use the same indentation as the surrounding lines.

> Why do you call MakeFullScreen on wince but not on normal Windows? It might want to be windows mobile only here. However, the call to make the window fullscreen needs to happen at WM_SHOW to avoid reflows, iirc. > This method (SetSizeMode) doesn't have any call to MakeFullScreen. I am not sure why you think it needs to here? > Was there a reason that you set it to nsSizeMode_Normal? it may be something other than normal (like maximized in this case)

(In reply to comment #34) > > This method (SetSizeMode) doesn't have any call to MakeFullScreen. > > I am not sure why you think it needs to here? If I change the sizemode from normal to fullscreen using SetSizeMode, I'd expect the window to become fullscreen, no? > > Was there a reason that you set it to nsSizeMode_Normal? > > it may be something other than normal (like maximized in this case) Huh? I don't understand this answer.

Comment on attachment 370682 [details] [diff] [review] patch v.9 Looks fine, but fix the indentation in nsXULWindow.h: PRInt32 AppUnitsPerDevPixel(); + PRBool DispatchCustomEvent(const nsAString& eventName, PRBool cancelable = PR_FALSE, PRBool toDocument = PR_TRUE);

basically the same, however SetSizeMode needs to be called during MakeFullScreen so that the state is remember.

(In reply to comment #33) > +nsWindow::MakeFullScreen(PRBool aFullScreen) > > + SHFullScreen(mWnd, SHFS_HIDETASKBAR | SHFS_HIDESTARTICON); > > + SHFullScreen(mWnd, SHFS_SHOWTASKBAR | SHFS_SHOWSTARTICON); > + SystemParametersInfo(SPI_GETWORKAREA, 0, &rc, FALSE); > > The rest of the file uses :: before these system function calls. This hasn't been fixed yet. The indentation in nsXULWindow.h also still needs fixing.

yup markus, the whitespace needs to be tweaked.

Comment on attachment 386118 [details] [diff] [review] trunk patch w/ minor update Looks fine, I think

this caused a windows unit test failure WINNT 5.2 mozilla-central unit test on 2009/07/01 14:30:40: 6261 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/dom/tests/mochitest/chrome/test_fullscreen.xul | window.fullScreen is true.

regresssion tracked in bug 501801.

needed to set SizeMode prior to dispatching the custom event so that window.fullScreen is the right value. Also cleaned up the mochitest so that it doesn't create any errors/warnings (missing body, some mochitest warnings)

Comment on attachment 386440 [details] [diff] [review] fixes regression pushed to try. waiting results

on second thought, it can't get much worse, right. http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=cb993b4f815e

clearing 1.9.1 request flag.

This caused bug 502133

reopening. I backed out the patches and will try again.

Comment on attachment 387281 [details] [diff] [review] fixes known regressions on windows works on linux/gtk as well. There are a couple of difference between this patch and former ones: 1) if you use the fullscreen property on a xul window, the window.fullscreen will not be true during the fullscreen callback. It will however be set appropriately after the callback (assuming you don't preventdefault). 2) prior patches tried to make widget hold the state of fullscreen or not fullscreen. This seemed like the right approach, but it got pretty messy because both nsXULWindow and the global window hold state as well. I reverted this attempt and just let the windows control the state.

Comment on attachment 387281 [details] [diff] [review] fixes known regressions on windows >@@ -3836,8 +3812,6 @@ nsGlobalWindow::SetFullScreen(PRBool aFu > // dispatch a "fullscreen" DOM event so that XUL apps can > // respond visually if we are kicked into full screen mode > if (!DispatchCustomEvent("fullscreen")) { >- // event handlers can prevent us from going into full-screen mode >- > return NS_OK; > } Why you remove the comment? >-_TEST_FILES = test_domstorage.xul \ >+_TEST_FILES = \ >+ test_fullscreen.xul \ >+ fullscreen.xul \ >+ test_fullscreen_preventdefault.xul \ >+ fullscreen_preventdefault.xul \ >+ test_domstorage.xul \ > domstorage_global.xul \ > domstorage_global.js \ > test_focus.xul \ The patch misses some test files

clearing wanted flag, this is fixed.

(In reply to comment #54) > http://hg.mozilla.org/mozilla-central/rev/51ca7c35d98f This didn't remove nsIFullScreen.idl. I removed it: http://hg.mozilla.org/mozilla-central/rev/f177359d53da

Doug, curious, why did we add this little snippet of code to the WM_ACTIVATE event handler? + if (mSizeMode == nsSizeMode_Fullscreen) + MakeFullScreen(TRUE); Windows should take care of restoring our original state, including maximized windows without us having to do it manually. Plus, even if this was needed, why did we only do full screen? Also, the return value is set to non-zero signaling to windows that we did not process this message? I'm working on another activation related bug in the same area and am wondering if this code is really needed.

Hi Jim, On windows mobile, when we were deactivated then brought back to the active window, the windows chrome (the title bar specifically) was not hidden. This check ensured that we would reenter fullscreen mode (hiding the title bar). It could be that our Windows Mobile code is missing something such that when we are restored Windows doesn't have the right state information?

(In reply to comment #58) > Hi Jim, > On windows mobile, when we were deactivated then brought back to the active > window, the windows chrome (the title bar specifically) was not hidden. This > check ensured that we would reenter fullscreen mode (hiding the title bar). > > It could be that our Windows Mobile code is missing something such that when we > are restored Windows doesn't have the right state information? Alright, I could look in to that when I have a chance. I think it would be safe to move this into the ce block above though, we shouldn't need it on win.