Closed Bug 783333 Opened 8 years ago Closed 7 years ago

nsWindow should do all move/size processing in `WM_WINDOWPOSCHANGING` and `WM_WINDOWPOSCHANGED`. Do not handle `WM_MOVE`, `WM_SIZE`, and `WM_SHOWWINDOW`

Categories

(Core :: Widget: Win32, defect)

All
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: TimAbraldes, Assigned: poiru)

Details

(Whiteboard: [mentor=tabraldes@mozilla.com][lang=c++])

Attachments

(1 file, 1 obsolete file)

Because Windows 3.1 was a long time ago [1].

Also, since we handle the `WM_WINDOWPOSCHANGING` and `WM_WINDOWPOSCHANGED` messages already, we should be returning `true` from our processing of those messages.  Otherwise, the default window proc additionally sends us the old-school messages.

[1] https://blogs.msdn.com/b/oldnewthing/archive/2008/01/15/7113860.aspx?Redirected=true
This bug involves working in win32-specific code so you'll need a Windows machine with a working build environment.

I think it should be pretty self-explanatory, but MSDN has some useful documentation [1][2][3][4], and Raymond Chen has an informative blog post [5].

From what I can tell, we only still process WM_MOVE, and we do that at [6].

[1] WM_SIZE - http://msdn.microsoft.com/en-us/library/windows/desktop/ms632646%28v=vs.85%29.aspx
[2] WM_MOVE - http://msdn.microsoft.com/en-us/library/windows/desktop/ms632631%28v=vs.85%29.aspx
[3] WM_SHOWWINDOW - http://msdn.microsoft.com/en-us/library/windows/desktop/ms632645%28v=vs.85%29.aspx
[4] WM_WINDOWPOSCHANGED - http://msdn.microsoft.com/en-us/library/windows/desktop/ms632652%28v=vs.85%29.aspx
[5] https://blogs.msdn.com/b/oldnewthing/archive/2008/01/15/7113860.aspx?Redirected=true
[6] https://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp?rev=c63a1c48560b#4730
Assignee: tabraldes → nobody
Status: ASSIGNED → NEW
Whiteboard: [mentor=tabraldes@mozilla.com][lang=c++]
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4431512811c6
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #808696 - Flags: review?(tabraldes)
Sorry about the delay! I've been out of town until today.

I'll take a look at this shortly.
Comment on attachment 808696 [details] [diff] [review]
Use WM_WINDOWPOSCHANGING instead of WM_MOVE to handle window moves in nsWindow

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

This looks great to me! I can't actually r+ this since I'm not a widget/windows peer, so I'll hand it off to jimm
Attachment #808696 - Flags: review?(tabraldes)
Attachment #808696 - Flags: review?(jmathies)
Attachment #808696 - Flags: feedback+
Comment on attachment 808696 [details] [diff] [review]
Use WM_WINDOWPOSCHANGING instead of WM_MOVE to handle window moves in nsWindow

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

::: widget/windows/nsWindow.cpp
@@ +5202,1 @@
>        OnWindowPosChanged(wp, result);

Do we need to pass result here? Looks like we could get rid of that in the handler.

@@ +5887,5 @@
> +  if (!(wp->flags & SWP_NOMOVE)) {
> +    mBounds.x = wp->x;
> +    mBounds.y = wp->y;
> +
> +    if (mWidgetListener)

nit - brackets around the if block
(In reply to Jim Mathies [:jimm] from comment #5)
> Comment on attachment 808696 [details] [diff] [review]

Thanks, fixed those and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=0848d73cc0b9
Attachment #808696 - Attachment is obsolete: true
Attachment #808696 - Flags: review?(jmathies)
Attachment #810727 - Flags: review?(jmathies)
Comment on attachment 810727 [details] [diff] [review]
Use WM_WINDOWPOSCHANGING instead of WM_MOVE to handle window moves in nsWindow

Took this for a spin, didn't see any issues with window resizing.
Attachment #810727 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a39f82ca67b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Returning "result = true" in case of WM_WINDOWPOSCHANGING breaks limit of window size.
Window size can be larger than desktop size after this fix. Anybody care?
(In reply to Redo Stuff from comment #10)
> Returning "result = true" in case of WM_WINDOWPOSCHANGING breaks limit of
> window size.
> Window size can be larger than desktop size after this fix. Anybody care?

Sorry, I'm having trouble understanding the issue here. Are you saying that before the patch we restricted our windows to being smaller than the desktop, and after the patch we no longer have that restriction?
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #11)
> Are you saying that before the patch we restricted our windows to being smaller than the
> desktop, and after the patch we no longer have that restriction?
Yes, at least it didn't tried to bypass general limits of Windows: 
>    case WM_GETMINMAXINFO:
>    {
>      MINMAXINFO* mmi = (MINMAXINFO*)lParam;
>      // Set the constraints. The minimum size should also be constrained to the
>      // default window maximum size so that it fits on screen.
>      mmi->ptMinTrackSize.x =
>        std::min((int32_t)mmi->ptMaxTrackSize.x,
>               std::max((int32_t)mmi->ptMinTrackSize.x, mSizeConstraints.mMinSize.width));
>      mmi->ptMinTrackSize.y =
>        std::min((int32_t)mmi->ptMaxTrackSize.y,
>        std::max((int32_t)mmi->ptMinTrackSize.y, mSizeConstraints.mMinSize.height));
>      mmi->ptMaxTrackSize.x = std::min((int32_t)mmi->ptMaxTrackSize.x, >mSizeConstraints.mMaxSize.width);
>      mmi->ptMaxTrackSize.y = std::min((int32_t)mmi->ptMaxTrackSize.y, >mSizeConstraints.mMaxSize.height);
>    }
>    break;
This code now useless.
You need to log in before you can comment on or make changes to this bug.