487245 - Cleanup and better organize code in widget/src/windows/nsWindow

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

General goal: clean up and simplify code for debugging and improve commenting

- split off static functions into utility library
- split off platform specific code into separate files
- split off debug code into separate file
- split off closely related chunks of code into separate files

Comment 1

[Jim Mathies [:jimm]]

Attachment: windows nswindow reorg v.1

Splitting off large code chunks ended up not being needed after I got things well organized. I will probably split off paint into a separate file nsWindowGrx in prep for the opengl stuff we'll be doing. This seemed like a good starting point though before making any further changes.

Comment 2

[Rob Arnold [:robarnold]]

The diff is somewhat hard to read. Do you have a list of what you're moving where and what you're simplifying?

Comment 3

[Jim Mathies [:jimm]]

(In reply to comment #2)
> The diff is somewhat hard to read. Do you have a list of what you're moving
> where and what you're simplifying?

It's broken down into major blocks and sub sections. The comment at the top has the outline that I followed - 

Includes
Variables
Misc. utilities
nsIWidget impl.
    nsIWidget methods
nsSwitchToUIThread impl.
    nsSwitchToUIThread methods
Native message handling (main event loop)
    Message processing methods
    OnXXX event handlers
Paint handling
IME management and accessibility
Transparency
Popup hook handling
Child window impl.

In addition to the nsWindow reorg, I've split code out into other files - 
nsWindowDefs.h
nsWindowDbg.h/.cpp
nsWindowCE.h/.cpp

I wasn't able to split out as much CE code as I had hoped, but maybe over time we can work on that. I avoided duplicate blocks of code in two files. That sort of restricted what I could move over to the ce src, but I was able to pull out about 500 unique lines.

Once I finish up I can post the raw src, that patch doesn't really do it justice IMHO.

Comment 4

[Jim Mathies [:jimm]]

Attachment: windows nswindow reorg v.1

Ok, this passed on try builds, including ce. I'll attach the nsWindow.cpp as well.

This patch looks a little scary, but 99.9% of it just code re-org and commenting cleanup.

Comment 5

[Jim Mathies [:jimm]]

Attachment: nsWindow.cpp

Comment 6

[Jim Mathies [:jimm]]

Attachment: nsWindow.cpp

Added another block - "Event dispatch" for separating nsSwitchToUIThread and the event stuff better.

Comment 7

[Jim Mathies [:jimm]]

Here is the current layout of the code in nsWindow.cpp:

 * Includes
 * Variables
 * Misc. utilities
 * nsIWidget impl.
 *     nsIWidget methods
 * nsSwitchToUIThread impl.
 *     nsSwitchToUIThread methods
 * Event init and dispatch
 * Native message handling
 *     Wnd proc
 *     Message processing
 *     OnXXX event handlers
 * Paint handling
 * IME management and accessibility
 * Transparency
 * Popup hook handling
 * Child window impl.

Comment 8

[Ere Maijala (slow)]

Comment on attachment 381325 [details] [diff] [review]
windows nswindow reorg v.1

> \ No newline at end of file

Please fix these. 

> * nsWindow has been reorganized into a set of major blocks

Make it "organized".

> * Blocks should be split out into seperate files if they

*separate*

I like it, but how about also putting the utility and WinCE functions into classes as statics so they wouldn't pollute the global namespace anymore? And while at it, why not put the global variables into nsWindow as statics? Currently we have a mess with both used arbitrarily.

Comment 9

[Jim Mathies [:jimm]]

(In reply to comment #8)
> (From update of attachment 381325 [details] [diff] [review])
> > \ No newline at end of file
> 
> Please fix these. 
> 
> > * nsWindow has been reorganized into a set of major blocks
> 
> Make it "organized".
> 
> > * Blocks should be split out into seperate files if they
> 
> *separate*
> 
> I like it, but how about also putting the utility and WinCE functions into
> classes as statics so they wouldn't pollute the global namespace anymore? And
> while at it, why not put the global variables into nsWindow as statics?
> Currently we have a mess with both used arbitrarily.

Good ideas, will do.

Comment 10

[Jim Mathies [:jimm]]

Attachment: windows nswindow reorg v.2

ok, here's an update -  

- more cleanup in nswindow.h/.cpp
- globals converted to statics
- standardized on sXXX and gXXX variable conventions
- split gfx related code out into nsWindowGfx
- wrapped ce and gfx utilities with classes
- misc. additional cleanup and commenting

Ere, this bit rots really often so if you could toss this on the top of your review queue I'd greatly appreciate it. :)

Comment 11

[Jim Mathies [:jimm]]

Attachment: windows nswindow reorg v.3

Same patch with some added white space cleanup, primarily in the new headers.

Comment 12

[Ere Maijala (slow)]

Comment on attachment 383116 [details] [diff] [review]
windows nswindow reorg v.3

Yeahm, rots quickly and I'm too slow. Please unrot and get it in. We can continue work on it after that if necessary.

Comment 13

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/1aecdc720018

I'm going to keep this bug open as I want to do some additional work on nsWindowGfx this week.

Comment 14

[Jim Mathies [:jimm]]

Attachment: windows nswindow reorg v.4

unbitrot and landed patch