606520 - Add an exit full screen option to the system menu

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Bug 580599 allows us to customize this menu, so it would be neat if we could add an option for exiting full screen.

Comment 1

[Manu Jain]

Hi, I would like to patch this bug. Please assist me how to go forward as I'm new to this.

Comment 2

[Anurag]

Hi, I would like to patch this bug. I am new to bug patching, so please guide me through the process.

Comment 3

[Jim Mathies [:jimm]]

Who ever takes this is free to work on it, since we have two requesters here.

We create our context menu manually based on user input - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#4277

When in fullscreen, it would be great if we could add an exit fullscren option. We'll need to investigate if this is even possible first and implement if we have a way forward.

Comment 4

[Manu Jain]

Jim, Can you tell what SC_RESTORE is used for? Thanks!!

Comment 5

[Manu Jain]

Also we need to make change in file "http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#175" for context menu. So, can you guide how to forward with this context thing. Thanks!!

Comment 6

[Jim Mathies [:jimm]]

(In reply to manu.jain13 from comment #4)
> Jim, Can you tell what SC_RESTORE is used for? Thanks!!

Google is your friend :)

http://msdn.microsoft.com/en-us/library/windows/desktop/ms646360%28v=vs.85%29.aspx

(In reply to manu.jain13 from comment #5)
> Also we need to make change in file
> "http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> BrowserApp.java#175" for context menu. So, can you guide how to forward with
> this context thing. Thanks!!

Why don't we concentrate on just fixing the Windows context menu here? I'm not even sure they need this in other browsers other than desktop.

Comment 7

[Manu Jain]

I think SC_RESTORE can also work as exit fullscreen, as it restores the window to normal size.
Also, can you tell how to reproduce the bug? Thanks!!

Comment 8

[u518207]

Attachment: 606520.patch

Hi.
I read about this bug and found it interesting.
I just modified the code and checked whether the result was correct.
No testing was performed, except the one mentioned above.

Regards.

Comment 9

[u518207]

Attachment: 606520.patch

The previous fix report wasn't formatted as needed.
Modification info was added using automatic tool.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8492748 [details] [diff] [review]
606520.patch

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

This works great, thanks!

::: widget/windows/nsWindow.cpp
@@ +5247,5 @@
> +	  
> +	  if (mSizeMode == nsSizeMode_Fullscreen && filteredWParam == SC_RESTORE) {
> +        MakeFullScreen(false);
> +        result = true;
> +	  }

Some white space or tabs snuck in here, please clean this formatting up. You can check white space in visual studio using the Edit -> Advanced -> Show whitespace option.

Comment 11

[u518207]

Attachment: 606520-indents_corrected.patch

Indents corrected.

Comment 12

[Jim Mathies [:jimm]]

(In reply to mycoolclub from comment #11)
> Created attachment 8494402 [details] [diff] [review]
> 606520-indents_corrected.patch
> 
> Indents corrected.

Thanks.

Would you also please update the commit message with a more accurate description of the bug, plus the review? Something like:

"Bug 606520 - Add support for exiting fullscreen via the Windows command menu. r=jimm"

Comment 13

[u518207]

Attachment: 606520.patch

I signed patch to be checked in, because I am not familiar with the procedure.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 8494422 [details] [diff] [review]
606520.patch

don't think we use this flag anymore.

Comment 15

[u518207]

Thanks.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Can we run this through Try first please to make sure there aren't any hidden surprises lurking :)? A Windows build + mochitests should suffice.

Comment 17

[u518207]

Ok, you mean the whole mochitest or some partial one with parameter(s); with only this single patch active?

Comment 18

[Jim Mathies [:jimm]]

we don't have any tests for the system menu.

Comment 19

[Jim Mathies [:jimm]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fb9ebeb5cc55

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bb15cbc8e7

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5bb15cbc8e7

Comment 22

[Jim Mathies [:jimm]]

Woot, thanks [e-mail]!

Comment 23

[u518207]

My pleasure.