484488 - nsWindow::MakeFullScreen needs to be implemented for wince

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Doug Turner (:dougt)]

Attachment: 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.

Comment 1

[Doug Turner (:dougt)]

Attachment: 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.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 4

[Doug Turner (:dougt)]

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 5

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.4

jst, can you also give this a review?

Comment 7

[Doug Turner (:dougt)]

Attachment: browser patch v.1

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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How does this interact with window.fullScreen?

Comment 9

[Doug Turner (:dougt)]

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 11

[Doug Turner (:dougt)]

Attachment: patch v.5

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.6

makes window.fullScreen return the current value.

Comment 13

[Doug Turner (:dougt)]

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

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 15

[Doug Turner (:dougt)]

@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 16

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

See http://markmail.org/message/5sjfdbcrqmyjtql3#query:nsifullscreen+page:1+mid:fin2u6bhhlulgguo+state:results

Let's remove nsIFullScreen and mFullScreen as you suggest.

Comment 18

[Doug Turner (:dougt)]

Attachment: patch v.7

Comment 19

[Doug Turner (:dougt)]

try builds: https://build.mozilla.org/tryserver-builds/2009-03-29_16:[e-mail]-try-aa871492653/

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

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 21

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 22

[Doug Turner (:dougt)]

Attachment: patch v.8

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

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 24

[Doug Turner (:dougt)]

Attachment: 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.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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 26

[Doug Turner (:dougt)]

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

can you sr?

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

jst sr ping?

Comment 28

[Doug Turner (:dougt)]

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

roc, can you sr?

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think Olli should sr, that seems most efficient.

Comment 30

[Doug Turner (:dougt)]

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

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 32

[Doug Turner (:dougt)]

not really.  just want my r/sr.

The big piece is removing the nsIFullScreen interface.

Comment 33

[Markus Stange [:mstange]]

@@ -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.

Comment 34

[Doug Turner (:dougt)]

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

Comment 35

[Markus Stange [:mstange]]

(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 36

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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);

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated to apply to trunk

Comment 38

[Doug Turner (:dougt)]

Attachment: trunk patch w/ minor update

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

Comment 39

[Markus Stange [:mstange]]

(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.

Comment 40

[Doug Turner (:dougt)]

yup markus, the whitespace needs to be tweaked.

Comment 41

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 386118 [details] [diff] [review]
trunk patch w/ minor update

Looks fine, I think

Comment 42

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/8312b1fdc851

Comment 43

[Josh Aas]

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.

Comment 44

[Doug Turner (:dougt)]

regresssion tracked in bug 501801.

Comment 45

[Doug Turner (:dougt)]

Attachment: 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)

Comment 46

[Doug Turner (:dougt)]

Comment on attachment 386440 [details] [diff] [review]
fixes regression

pushed to try. waiting results

Comment 47

[Doug Turner (:dougt)]

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

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

Comment 48

[Doug Turner (:dougt)]

clearing 1.9.1 request flag.

Comment 49

[adrian]

This caused bug 502133

Comment 50

[Doug Turner (:dougt)]

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

Comment 51

[Doug Turner (:dougt)]

Attachment: fixes known regressions on windows

Comment 52

[Doug Turner (:dougt)]

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 53

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 54

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/51ca7c35d98f

Comment 55

[Doug Turner (:dougt)]

clearing wanted flag, this is fixed.

Comment 56

[Markus Stange [:mstange]]

(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

Comment 57

[Jim Mathies [:jimm]]

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.

Comment 58

[Doug Turner (:dougt)]

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

[Jim Mathies [:jimm]]

(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.