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

RESOLVED FIXED

Status

()

Core
Widget: Win32
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Depends on: 1 bug)

Trunk
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

9 years ago
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
(Assignee)

Comment 1

9 years ago
Created attachment 381154 [details] [diff] [review]
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.
The diff is somewhat hard to read. Do you have a list of what you're moving where and what you're simplifying?
(Assignee)

Comment 3

9 years ago
(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.
(Assignee)

Comment 4

9 years ago
Created attachment 381325 [details] [diff] [review]
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.
Attachment #381154 - Attachment is obsolete: true
Attachment #381325 - Flags: review?(emaijala)
(Assignee)

Comment 5

9 years ago
Created attachment 381326 [details]
nsWindow.cpp
(Assignee)

Comment 6

9 years ago
Created attachment 381329 [details]
nsWindow.cpp

Added another block - "Event dispatch" for separating nsSwitchToUIThread and the event stuff better.
Attachment #381326 - Attachment is obsolete: true
(Assignee)

Comment 7

9 years ago
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

9 years ago
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-
(Assignee)

Comment 9

9 years ago
(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.
(Assignee)

Comment 10

9 years ago
Created attachment 383060 [details] [diff] [review]
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. :)
Attachment #381325 - Attachment is obsolete: true
Attachment #381329 - Attachment is obsolete: true
Attachment #383060 - Flags: review?(emaijala)
(Assignee)

Comment 11

9 years ago
Created attachment 383116 [details] [diff] [review]
windows nswindow reorg v.3

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

Comment 12

9 years ago
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+
(Assignee)

Comment 13

9 years ago
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.
(Assignee)

Comment 14

9 years ago
Created attachment 385867 [details] [diff] [review]
windows nswindow reorg v.4

unbitrot and landed patch
Attachment #383060 - Attachment is obsolete: true
Attachment #383116 - Attachment is obsolete: true
Attachment #383060 - Flags: review?(emaijala)
(Assignee)

Updated

9 years ago
Depends on: 501390
(Assignee)

Updated

9 years ago
Blocks: 501379
(Assignee)

Updated

9 years ago
No longer depends on: 501390
(Assignee)

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 510686

Updated

9 years ago
Blocks: 326661
(Assignee)

Updated

9 years ago
No longer depends on: 483136
You need to log in before you can comment on or make changes to this bug.