Closed Bug 606520 Opened 9 years ago Closed 5 years ago

Add an exit full screen option to the system menu

Categories

(Core :: Widget: Win32, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jimm, Assigned: u518207, Mentored)

References

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 file, 3 obsolete files)

Bug 580599 allows us to customize this menu, so it would be neat if we could add an option for exiting full screen.
Depends on: 580599
Mentor: jmathies
Whiteboard: [good first bug][lang=c++]
Hi, I would like to patch this bug. Please assist me how to go forward as I'm new to this.
Hi, I would like to patch this bug. I am new to bug patching, so please guide me through the process.
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.
Jim, Can you tell what SC_RESTORE is used for? Thanks!!
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!!
(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.
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!!
Attached file 606520.patch (obsolete) —
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.
Attachment #8492636 - Flags: review?(jmathies)
Attachment #8492636 - Attachment is obsolete: true
Attachment #8492636 - Attachment is patch: false
Attachment #8492636 - Flags: review?(jmathies)
Attached patch 606520.patch (obsolete) — Splinter Review
The previous fix report wasn't formatted as needed.
Modification info was added using automatic tool.
Attachment #8492748 - Flags: review?(jmathies)
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.
Attachment #8492748 - Flags: review?(jmathies) → review+
Assignee: nobody → mycoolclub
Attachment #8492748 - Attachment is obsolete: true
Attached patch 606520-indents_corrected.patch (obsolete) — Splinter Review
Indents corrected.
Attachment #8494402 - Flags: review+
(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"
Attachment #8494402 - Attachment is obsolete: true
Attached patch 606520.patchSplinter Review
I signed patch to be checked in, because I am not familiar with the procedure.
Attachment #8494422 - Flags: review+
Attachment #8494422 - Flags: checkin?
Comment on attachment 8494422 [details] [diff] [review]
606520.patch

don't think we use this flag anymore.
Attachment #8494422 - Flags: checkin?
Keywords: checkin-needed
Thanks.
Can we run this through Try first please to make sure there aren't any hidden surprises lurking :)? A Windows build + mochitests should suffice.
Keywords: checkin-needed
Ok, you mean the whole mochitest or some partial one with parameter(s); with only this single patch active?
we don't have any tests for the system menu.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5bb15cbc8e7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Woot, thanks mycoolclub@yahoo.com!
My pleasure.
You need to log in before you can comment on or make changes to this bug.