Closed Bug 487245 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Widget: Win32, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(1 file, 6 obsolete files)

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
Attached patch windows nswindow reorg v.1 (obsolete) — Splinter Review
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.
The diff is somewhat hard to read. Do you have a list of what you're moving where and what you're simplifying?
(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.
Attached patch windows nswindow reorg v.1 (obsolete) — Splinter Review
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.
Attachment #381154 - Attachment is obsolete: true
Attachment #381325 - Flags: review?(emaijala)
Attached file nsWindow.cpp (obsolete) —
Attached file nsWindow.cpp (obsolete) —
Added another block - "Event dispatch" for separating nsSwitchToUIThread and the event stuff better.
Attachment #381326 - Attachment is obsolete: true
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 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.
Attachment #381325 - Flags: review?(emaijala) → review-
(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.
Attached patch windows nswindow reorg v.2 (obsolete) — Splinter Review
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. :)
Attachment #381325 - Attachment is obsolete: true
Attachment #381329 - Attachment is obsolete: true
Attachment #383060 - Flags: review?(emaijala)
Attached patch windows nswindow reorg v.3 (obsolete) — Splinter Review
Same patch with some added white space cleanup, primarily in the new headers.
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.
Attachment #383116 - Flags: review+
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.
unbitrot and landed patch
Attachment #383060 - Attachment is obsolete: true
Attachment #383116 - Attachment is obsolete: true
Attachment #383060 - Flags: review?(emaijala)
Depends on: 501390
Blocks: 501379
No longer depends on: 501390
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 510686
Blocks: 326661
No longer depends on: 483136
You need to log in before you can comment on or make changes to this bug.