nsWindow::MakeFullScreen needs to be implemented for wince

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

unspecified
x86
Windows CE
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec1.0a2-wm+)

Details

Attachments

(1 attachment, 13 obsolete attachments)

16.53 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
Created attachment 368609 [details] [diff] [review]
patch v.1

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.
Attachment #368609 - Flags: superreview?(pavlov)
Attachment #368609 - Flags: review?(bugmail)
(Assignee)

Updated

10 years ago
tracking-fennec: --- → ?
tracking-fennec: ? → 1.0a2-wm+
(Assignee)

Comment 1

10 years ago
Created attachment 368906 [details] [diff] [review]
patch v.2

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.
Attachment #368609 - Attachment is obsolete: true
Attachment #368906 - Flags: superreview?(pavlov)
Attachment #368906 - Flags: review?(bugmail)
Attachment #368609 - Flags: superreview?(pavlov)
Attachment #368609 - Flags: review?(bugmail)
(Assignee)

Comment 2

10 years ago
Created attachment 368907 [details] [diff] [review]
patch v.3
Attachment #368906 - Attachment is obsolete: true
Attachment #368907 - Flags: superreview?(pavlov)
Attachment #368907 - Flags: review?(bugmail)
Attachment #368906 - Flags: superreview?(pavlov)
Attachment #368906 - Flags: review?(bugmail)
(Assignee)

Updated

10 years ago
Attachment #368907 - Flags: superreview?(pavlov) → superreview?(vladimir)
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.
Attachment #368907 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 4

10 years ago
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
Attachment #368907 - Flags: review?(bugmail) → review+
(Assignee)

Comment 6

10 years ago
Created attachment 368940 [details] [diff] [review]
patch v.4

jst, can you also give this a review?
Attachment #368907 - Attachment is obsolete: true
Attachment #368940 - Flags: superreview?(jst)
(Assignee)

Comment 7

10 years ago
Created attachment 368941 [details] [diff] [review]
browser patch v.1

this tweaks session store so that fullscreen will behave like maximized.
Attachment #368941 - Flags: superreview?
Attachment #368941 - Flags: review?
How does this interact with window.fullScreen?
(Assignee)

Comment 9

10 years ago
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.
(Assignee)

Comment 11

10 years ago
Created attachment 368960 [details] [diff] [review]
patch v.5
Attachment #368940 - Attachment is obsolete: true
Attachment #368941 - Attachment is obsolete: true
Attachment #368941 - Flags: superreview?
Attachment #368941 - Flags: review?
Attachment #368940 - Flags: superreview?(jst)
(Assignee)

Updated

10 years ago
Attachment #368960 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
Created attachment 368986 [details] [diff] [review]
patch v.6

makes window.fullScreen return the current value.
Attachment #368986 - Flags: superreview?(jst)
Attachment #368986 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Attachment #368986 - Flags: superreview?(jst)
Attachment #368986 - Flags: superreview?(Olli.Pettay)
Attachment #368986 - Flags: review?(jst)
Attachment #368986 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 13

10 years ago
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 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
@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?

Updated

10 years ago
Attachment #368986 - Flags: superreview?(Olli.Pettay)
Attachment #368986 - Flags: superreview-
Attachment #368986 - Flags: review?(Olli.Pettay)
Attachment #368986 - Flags: review-

Comment 16

10 years ago
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)
(Assignee)

Comment 18

10 years ago
Created attachment 369934 [details] [diff] [review]
patch v.7
Attachment #368986 - Attachment is obsolete: true
Attachment #369934 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #369934 - Flags: review? → review?(Olli.Pettay)

Comment 20

10 years ago
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?

Updated

10 years ago
Attachment #369934 - Flags: review?(Olli.Pettay) → review-

Comment 21

10 years ago
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.
(Assignee)

Comment 22

10 years ago
Created attachment 370529 [details] [diff] [review]
patch v.8

1) allows for cancelable fullscreen events when using sizemode=fullscreen.
2) factors custom event dispatching code in nsXULWindow
Attachment #369934 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #370529 - Flags: review?(Olli.Pettay)

Comment 23

10 years ago
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
Attachment #370529 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 24

10 years ago
Created attachment 370682 [details] [diff] [review]
patch v.9

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.
Attachment #370529 - Attachment is obsolete: true
Attachment #370682 - Flags: review?(Olli.Pettay)

Comment 25

10 years ago
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;
}
Attachment #370682 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 26

10 years ago
Comment on attachment 370682 [details] [diff] [review]
patch v.9

can you sr?
Attachment #370682 - Flags: superreview?(jst)
(Assignee)

Updated

10 years ago
Flags: wanted1.9.1?
(Assignee)

Updated

9 years ago
Attachment #370682 - Flags: superreview?(jst) → superreview?(roc)
(Assignee)

Comment 28

9 years ago
Comment on attachment 370682 [details] [diff] [review]
patch v.9

roc, can you sr?
I think Olli should sr, that seems most efficient.
(Assignee)

Comment 30

9 years ago
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?
(Assignee)

Comment 32

9 years ago
not really.  just want my r/sr.

The big piece is removing the nsIFullScreen interface.
(Assignee)

Updated

9 years ago
Attachment #370682 - Flags: superreview?(roc) → superreview?(vladimir)
@@ -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.
(Assignee)

Comment 34

9 years ago
> 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.
Attachment #370682 - Flags: superreview?(vladimir) → superreview+
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);
Created attachment 386037 [details] [diff] [review]
updated to apply to trunk
Attachment #370682 - Attachment is obsolete: true
Attachment #386037 - Flags: superreview+
Attachment #386037 - Flags: review+
(Assignee)

Comment 38

9 years ago
Created attachment 386118 [details] [diff] [review]
trunk patch w/ minor update

basically the same, however SetSizeMode needs to be called during MakeFullScreen so that the state is remember.
Attachment #386037 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #386118 - Flags: superreview?(vladimir)
Attachment #386118 - Flags: review?(vladimir)
(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.
(Assignee)

Comment 40

9 years ago
yup markus, the whitespace needs to be tweaked.
Attachment #386118 - Flags: superreview?(vladimir)
Attachment #386118 - Flags: superreview+
Attachment #386118 - Flags: review?(vladimir)
Attachment #386118 - Flags: review+
Comment on attachment 386118 [details] [diff] [review]
trunk patch w/ minor update

Looks fine, I think
(Assignee)

Comment 42

9 years ago
http://hg.mozilla.org/mozilla-central/rev/8312b1fdc851
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 43

9 years ago
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.
(Assignee)

Comment 44

9 years ago
regresssion tracked in bug 501801.
(Assignee)

Comment 45

9 years ago
Created attachment 386440 [details] [diff] [review]
fixes regression

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)
(Assignee)

Comment 46

9 years ago
Comment on attachment 386440 [details] [diff] [review]
fixes regression

pushed to try. waiting results
Attachment #386440 - Flags: review?(vladimir)
Attachment #386440 - Flags: review?(vladimir) → review+
(Assignee)

Comment 47

9 years ago
on second thought, it can't get much worse, right.

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=cb993b4f815e
(Assignee)

Comment 48

9 years ago
clearing 1.9.1 request flag.
Flags: wanted1.9.1? → wanted-fennec1.0?

Updated

9 years ago
Blocks: 502133

Comment 49

9 years ago
This caused bug 502133
(Assignee)

Comment 50

9 years ago
reopening.  I backed out the patches and will try again.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 51

9 years ago
Created attachment 387281 [details] [diff] [review]
fixes known regressions on windows
Attachment #386118 - Attachment is obsolete: true
Attachment #386440 - Attachment is obsolete: true
(Assignee)

Comment 52

9 years ago
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.
Attachment #387281 - Flags: review?(Olli.Pettay)
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
Attachment #387281 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 54

9 years ago
http://hg.mozilla.org/mozilla-central/rev/51ca7c35d98f
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 55

9 years ago
clearing wanted flag, this is fixed.
Flags: wanted-fennec1.0?
Depends on: 504499

Updated

9 years ago
Blocks: 251599

Comment 57

9 years ago
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.
(Assignee)

Comment 58

9 years ago
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?

Comment 59

9 years ago
(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.
Blocks: 502232

Updated

8 years ago
Depends on: 580599
You need to log in before you can comment on or make changes to this bug.