617052 - Use ITaskbarList2::MarkFullscreenWindow for proper windows taskbar integration

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

http://msdn.microsoft.com/en-us/library/bb774640(v=VS.85).aspx

We should support this via widget. It should help cleanup some of the full screen / taskbar z-order issues we have.

Comment 1

[Brian R. Bondy [:bbondy]]

This does fix the problem with the auto hide taskbars by the way.
The thing is that we use ITaskbarList4 which requires Windows 7 but this function is available in XP which would be nice to have. 

I added Bug 680227.

I will use this bug as the autohide fix for Windows 7.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch for letting the taskbar know when we are going into full screen mode.

This fixes the auto hide taskbar problem.

I extended nsIWintaskbar for this call.

The extra version of prepareFullScreen with nsIDOMWindow was done because I think there are some tasks coming up that allows web content to control full screen in the "Web API".

Let me know if there is a way to get a nsIDOMWindow  from a nsWindow.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 554223 [details] [diff] [review]
Patch for letting the taskbar know when we are going into full screen mode.

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

::: widget/public/nsIWinTaskbar.idl
@@ +194,5 @@
> +   *
> +   * @thow NS_ERROR_INVALID_ARG if the window is not a valid top level window
> +   * @thow NS_ERROR_UNEXPECTED for general failures.
> +   */
> +  void prepareFullScreenHWND(in voidPtr aWindow, in boolean aFullScreen);

We should mark this with [noscript] for now. (With ctypes we might actually be able to do this at some point but i don't believe we support native handles yet.) Besides I don't think we want to expose this to script until we have a good reason to do so.

Also, will we run into any problems on 64-bit builds passing a HANDLE through this interface?

::: widget/src/windows/WinTaskbar.cpp
@@ +452,5 @@
> +  
> +  return PrepareFullScreenHWND(toplevelHWND, aFullScreen);
> +}
> +
> +/* void prepareFullScreen(in nsIDOMWindow aWindow, in boolean aFullScreen); */

nit - comment copy paste error

::: widget/src/windows/nsWindow.cpp
@@ +2726,5 @@
>    }
>  
> +  if (!aFullScreen) {
> +    // Notify the taskbar that we have exited full screen mode.
> +    if (taskbarInfo) {

Might as well combine these together into a single if.

Comment 5

[Brian R. Bondy [:bbondy]]

Thanks for the quick review :)

> Also, will we run into any problems on 64-bit builds passing a HANDLE through this interface?

No we wouldn't run into problems on x64 compiled apps because a HWND is a HANDLE which is a void* which would be 8 bytes on x64.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 554223 [details] [diff] [review]
Patch for letting the taskbar know when we are going into full screen mode.

cool. r+ with nits addressed.

Comment 7

[Brian R. Bondy [:bbondy]]

Found one more problem with this that I need to fix too if you start in full screen mode mTaskbar is not yet initialized. So I'll r? you again in any case.

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Patch for letting the taskbar know when we are going into full screen mode v2

Implemented review comments and fixed problem if you start in full screen.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 554395 [details] [diff] [review]
Patch for letting the taskbar know when we are going into full screen mode v2

I took a second to spot that '@thow' vs. '@throw' change. :) Thanks for cleaning that up.

Comment 10

[Brian R. Bondy [:bbondy]]

This was pushed to try, results:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=e8059c500d22

Comment 11

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6faef8a5901c

Comment 12

[Ed Morley [:emorley]]

http://hg.mozilla.org/mozilla-central/rev/6faef8a5901c

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 554395 [details] [diff] [review]
Patch for letting the taskbar know when we are going into full screen mode v2

>+NS_IMETHODIMP
>+WinTaskbar::PrepareFullScreenHWND(void *aHWND, PRBool aFullScreen) {
>+  if (!Initialize())
>+    return NS_ERROR_NOT_AVAILABLE;
>+
>+    NS_ENSURE_ARG_POINTER(aHWND);
>+
>+    if (!::IsWindow((HWND)aHWND)) 
>+      return NS_ERROR_INVALID_ARG;
>+
>+    HRESULT hr = mTaskbar->MarkFullscreenWindow((HWND)aHWND, aFullScreen);
>+    if (FAILED(hr)) {
>+      return NS_ERROR_UNEXPECTED;
>+    }
>+
>+    return NS_OK;
>+}

Weird indentation here

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Formatting fix for previous push.

No logic change.

Comment 15

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/fb5d85637013