1756621 - Cleanup Win32 nsWindow size mode logic

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P3)

Description

[Chris Martin [:cmartin]]

The logic of the Win32 nsWindow mSizeMode is rather difficult to follow, as it goes through a bit of a maze of code, is touched by Win32 API callbacks after it is set, etc. As a result, there are likely subtle bugs and other hard-to-reason-about behavior, and maintainability also suffers.

Let's try and refactor this code a bit to pull mSizeMode and others into some objects and try to enforce some variants around them so we can more easily reason about this code.

Comment 1

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Cleanup Win32 nsWindow.h

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83586d05e4b9
Cleanup Win32 nsWindow.h

Comment 3

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Allow nsBaseWidget::mBorderStyle to be set via constructor

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a5554703a48
Allow nsBaseWidget::mBorderStyle to be set via constructor r=handyman

Comment 5

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Remove nsWindowBase

It looks like it was added to abstract commonalities between Win32 and
WinRT. But we dropped support for WinRT a long time ago, and there hasn't been
any work on this area of code in 8 years. In the meantime, it just adds an
extra layer of indirection that doesn't need to exist.

Comment 6

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/83586d05e4b9
https://hg.mozilla.org/mozilla-central/rev/7a5554703a48

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70df6518a020
Remove nsWindowBase r=handyman

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/70df6518a020

Comment 9

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Encapsulate Win32 nsWindow size mode members

As a first step to making this code easier to understand, we pull the members
into a smaller class with a limited API to reduce their scope.

I will move all these new methods together in the next changelist -- It just
makes review difficult if everything is moving and changing at the same time.

Comment 10

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Move new Win32 nsWindow::FrameState functions

This is just a copy/paste change

Comment 11

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Cleanup nsWindow::FrameState code

Now that the code has been restructured, let's simplify some of the methods
by propagating constants and removing redundant logic

Comment 12

[Chris Martin [:cmartin]]

Attachment: Bug 1756621 - Add CheckInvariantWrapper to nsWindow and use for FrameState

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53be97731374
Encapsulate Win32 nsWindow size mode members r=handyman
https://hg.mozilla.org/integration/autoland/rev/e08c65b89343
Move new Win32 nsWindow::FrameState functions r=handyman
https://hg.mozilla.org/integration/autoland/rev/832276c12c9c
Cleanup nsWindow::FrameState code r=handyman
https://hg.mozilla.org/integration/autoland/rev/c9dad72c910f
Add CheckInvariantWrapper to nsWindow and use for FrameState r=handyman

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/53be97731374
https://hg.mozilla.org/mozilla-central/rev/e08c65b89343
https://hg.mozilla.org/mozilla-central/rev/832276c12c9c
https://hg.mozilla.org/mozilla-central/rev/c9dad72c910f

Comment 15

[Stephen A Pohl [:spohl]]

Chris, do you intend to land more patches as part of this bug, or can the leave-open keyword be removed and this bug be closed?