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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 2

[Biru [:poiru]]

Attachment: Use WM_WINDOWPOSCHANGING instead of WM_MOVE to handle window moves in nsWindow

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=4431512811c6

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Sorry about the delay! I've been out of town until today.

I'll take a look at this shortly.

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 5

[Jim Mathies [:jimm]]

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

Comment 6

[Biru [:poiru]]

Attachment: Use WM_WINDOWPOSCHANGING instead of WM_MOVE to handle window moves in nsWindow

(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

Comment 7

[Jim Mathies [:jimm]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a39f82ca67b

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0a39f82ca67b

Comment 10

[Redo Stuff]

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?

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

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

Comment 12

[Redo Stuff]

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