522896 - OS/2 Code Maintenance

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Rich Walsh]

OS/2-specific code, particularly in widget, hasn't had any systematic maintenance in years.  Beside being hard to work with, its current state also reflects badly on our efforts to maintain OS/2 as a viable & credible platform.

The purpose of this bug is to eliminate dead & obsolete code, pointless debugging statements, and other accumulated cruft.  I also plan to reformat 
the code using current standards and to reorganize it in a more logical sequence.

Each patch will be limited in scope, and will be named & numbered using this format:  "cm##-purpose-of-patch.diff".  The first 3 will follow shortly...

Comment 1

[Rich Walsh]

Attachment: cm00 -eliminate gcc 4.x warnings [checkin comment 24]

This eliminates the warnings I get when compiling with gcc 4.3.3, either by casting or by changing datatypes.

Comment 2

[Rich Walsh]

Attachment: cm01 - remove nsdefs.h & nsWidgetDefs.h

Most of the items in these files aren't used anymore.  Of those that are, only one is referenced by more than one file.  Worse, both include os2.h but with different INCL_* macros defined.

I moved the useful stuff into the appropriate files (mostly nsWindow.h), added os2.h to the few files that needed it, and made a few trivial code changes to replace a macro with the actual value.

Comment 3

[Rich Walsh]

Attachment: cm02 - rewrite widget creation

Of all the incomprehensible, redundant, largely-obsolete, badly-formatted code we have, nothing beats nsWindow::Create(), DoCreate(), & RealDoCreate()!

I've consolidated all of the generic widget initialization into Create() and have moved the class-specific tasks into a new virtual function, CreateWindow().  Code that was obsolete or no longer necessary has been removed, reducing the number of lines by over 40%.

I tested this very extensively using Firefox.  I'd appreciate it if someone who builds Seamonkey and/or Thunderbird from the trunk could apply these patches and report on the results.

Comment 4

[Dave Yeo]

I've built Seamonkey from the trunk ( Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a1pre) Gecko/20091018 SeaMonkey/2.1a1pre ) with these patches and after an hour of testing it seems to be performing as well as ever. This is built with GCC 4.4.1 second edition.
I'll build Thunderbird next and report if there are any issues.

Comment 5

[Rich Walsh]

(In reply to comment #4)
> I've built Seamonkey from the trunk [...]with these patches and after
> an hour of testing it seems to be performing as well as ever.

Great!  Something to look for:  do dialogs come up in the right place?  In particular, I'm concerned about dialogs spawned by other dialogs like Preferences.  Can modal dialogs be hidden by their parents?  Can any dialog be created partly or completely offscreen?  What about the popup lists for comboboxes?  For really long lists (like fonts), do they display scrollbars or do they extend off the screen?

Comment 6

[Dave Yeo]

(In reply to comment #4)
>Great!  Something to look for:  do dialogs come up in the right place?

They all seem to be coming up in their normal places
  
>In particular, I'm concerned about dialogs spawned by other dialogs like
>Preferences.

Tested with clicking the cookie manager from preferences, comes up where expected, can be hidden by the preferences and unhidden.

>Can modal dialogs be hidden by their parents?
>Can any dialog be created partly or completely offscreen?

When moving the preferences partially offscreen then closing, when reopened it is at the edge to the screen, so totally visible.

>What about the popup lists for comboboxes?

Not sure what you mean unless what I mention below.

>For really long lists (like fonts), do they display scrollbars or
>do they extend off the screen?

The fonts list has scroll bars as expected and a couple of other things like the headers for a message also have scroll bars as expected.
Everything seems to open where it should with the correct size and onscreen. Things like preferences can be covered by the browser window.
One thing I did notice was that I was trying to get a Flash video to play (it was permanently paused which happens sometimes here) and right clicking on it to get the Flash menu up sometimes when the Flash right click menu went away the web page right click menu would show up like the right click was leaking through the plugin window. All the right click menus also seem correct.

Comment 7

[Dave Yeo]

Tested Thunderbird, most windows are fine, some that can not be hidden work the same as my last Thunderbird build. The account settings window is too large, displaying below the warpcentre so that an equivalent amount of the window is below the bottom of the desktop, It is the same in my last build.
There are some display glitches but I think they are related to recent changes and not your patches. Would have to build from branch to compare though.

Comment 8

[Rich Walsh]

Attachment: cm01 - remove nsdefs.h & nsWidgetDefs.h v2

These things are already starting to bit-rot...

Comment 9

[Rich Walsh]

Attachment: cm03 - remove references to native scrollbars

This removes references to the native scrollbars that Mozilla no longer uses (e.g. no more "if (mScrollbar) ...).

Comment 10

[Rich Walsh]

Attachment: cm04 - fullscreen fix for v1.9.3

This fixes fullscreen (aka kiosk-mode) issues for v1.9.3.  See Bug 524258 for details.

Comment 11

[Rich Walsh]

Attachment: cm05 - reformat nsFrameWindow.*

This reformats nsFrameWindow.cpp & nsFrameWindow.h to provide a consistent style.  There are no code changes except the elimination of two variables that are no longer used (mSizeBorder & fHiddenWindowCreated).

Comment 12

[Peter Weilbacher]

Rich, thanks for your efforts on this! I wanted to do something like this for years.

I have quickly glanced at some of the patches but not have time for a real review yet. And I need to finally make a build of trunk again. I'll hopefully be able to do this next weekend.

Comment 13

[Rich Walsh]

(In reply to comment #12)
> I wanted to do something like this for years.

Only now am I starting to get to "the good stuff" - the next several patches will be more far-reaching.  All of this is laying the groundwork for a new bug:  "Eliminate nsFrameWindow".  More on that later...

> And I need to finally make a build of trunk again.

I hope it's more stable than the one I built last night.  Something that changed in the last 2-3 days has made it unusable (for me, constant crashes in gcc433).

Comment 14

[Walter Meinl]

> I hope it's more stable than the one I built last night.  Something that
> changed in the last 2-3 days has made it unusable (for me, constant crashes in
> gcc433).
I dont see any problems here with all the patches applied and compiled with gcc-4.4.1 (first edition, no extra gcc441.dll)

Comment 15

[Rich Walsh]

The next 2 patches, cm06a & cm06b, reformat and clean up nsWindow.h and the top of nsWindow.cpp (everything above the nsWindow constructor).

cm06a reformats & reorganizes the files without deleting anything.  Everything that will be eliminated appears at the end of the .h file or in the last section before the constructor in the .cpp file.

cm06b removes the unreferenced, obsolete, or unnecessary code, data, & macros identified by cm06a (and there's quite a lot).  There are also a few other small changes, like combining several booleans into a single bit-flag variable.

Comment 16

[Rich Walsh]

Attachment: cm06a - reformat nsWindow.h

Comment 17

[Rich Walsh]

Attachment: cm06b - cleanup nsWindow.*

Comment 18

[Peter Weilbacher]

Comment on attachment 406880 [details] [diff] [review]
cm02 - rewrite widget creation

Oh, this is a big one, and great, too. This Create/RealCreate etc. was really annoying.

Still, some remarks:

>+  // Set the frame control HWNDs as properties for use by fullscreen mode.
>+  // XXX Making these data members of nsFrameWindow would probably be better.

Why don't you do that right now? If we are in this big overhaul phase, it would be better not to introduce new XXX comments unnecessarily. They tend to stay forever... But what exactly would you make a member of nsFrameWindow? Not the temporary handle?

>+  // XXX Toolkit is obsolete & will be removed.

OK for this XXX.

>+  #ifdef DEBUG_FOCUS
>+    mWindowIdentifier = currentWindowIdentifier;
>+    currentWindowIdentifier++;
>+  #endif

I think canonical indentation rules have the # in the first column and the code on the same level as the surrounding stuff.

>+  // If a TrackPoint is in use, create dummy scrollbars.
>+  // XXX  Popups may need this also to scroll comboboxes.
>+  if (gIsTrackPoint && mWindowType == eWindowType_child) {
>+    WinCreateWindow(mWnd, WC_SCROLLBAR, 0, SBS_VERT,
>+                    0, 0, 0, 0, mWnd, HWND_TOP,
>+                    FID_VERTSCROLL, 0, 0);
>+  }

Where does this XXX come from? The old code also worked without any test for eWindowType_popup (if that's what you mean). If this TrackPoint workaround is still necessary, don't you have to leave the rest of the scrollbar stuff in, too? I mean everything with mIsScrollBar?

Comment 19

[Peter Weilbacher]

Comment on attachment 408796 [details] [diff] [review]
cm03 - remove references to native scrollbars

Assuming I'll be convinced by the c02 patch, this will be OK, too.

Comment 20

[Peter Weilbacher]

Comment on attachment 408797 [details] [diff] [review]
cm04 - fullscreen fix for v1.9.3

Haven't tried it out yet, but cool. :-)

Comment 21

[Peter Weilbacher]

Comment on attachment 409486 [details] [diff] [review]
cm06a - reformat nsWindow.h

>+ * John Fairhurst 17-09-98 first version
>+ *        Revised 01-12-98 to inherit from nsBaseWidget.
>+ *        Revised 24-01-99 to use hashtable
>+ *        Revised 15-03-99 for new menu classes
>+ *        Revised 05-06-99 to use pres-params
>+ *        Revised 19-06-99 drag'n'drop, etc.

I think you should remove this block. His name is already in the contributors list and while I'm sure he did great work back then, the above doesn't really tell us anything any more.

>+//-----------------------------------------------------------------------------
>+/*  XXX this needs to be rewritten

If you want to do that in this bug, don't add an XXX, just remove the whole section and add a new one when the related code is redone.

>+ * The subclass proc (fnwpNSWindow) calls ProcessMessage() in the object.
>+ * Decisions are taken here about what to do - the purpose of the OnFoo()
>+ * methods is to generate an NS event to the various people who are
>+ * listening, or not.
>+ *
>+ * OS/2 things: remember supplied coords are in the XP space.  There are
>+ * NS2PM methods for conversion of points & rectangles; position is a bit
>+ * different in that it's the *parent* window whose height must be used.
>+ *
>+ * SetColorSpace() is not implemented on purpose.  So there.

>+//-----------------------------------------------------------------------------
>+//  App Command messages for IntelliMouse and Natural Keyboard Pro
>+
>+// XXX Is there any evidence that this is actually used on OS/2?
>+#define WM_APPCOMMAND                   0x0319
>+
>+#define APPCOMMAND_BROWSER_BACKWARD     1
>+#define APPCOMMAND_BROWSER_FORWARD      2
>+#define APPCOMMAND_BROWSER_REFRESH      3
>+#define APPCOMMAND_BROWSER_STOP         4

Yes, see bug 257619.


>+ *  XXX this needs to be rewritten
>+ *  Base widget class.
>+ *  This is abstract.  Controls (labels, radio buttons, listboxen) derive
>+ *  from here.  A thing called a child window derives from here, and the
>+ *  frame window class derives from the child.
>+ *  nsFrameWindow is separate because work needs to be done there to decide
>+ *  whether methods apply to frame or client.

See above.

>+  // unreferenced - delete from .h
>+  BOOL              mIsScrollBar;
>+  QMSG              mQmsg;

You are not doing this now?

>+  /*extern nsWindow *     NS_HWNDToWindow(HWND hwnd);*/
>+  virtual void          SetupForPrint(HWND hwnd)    {}
>+  HWND                  GetHWND() const             {return mWnd;}
>+  enum {
>+     CREATE,
>+     DESTROY,
>+     SET_FOCUS,
>+     UPDATE_WINDOW,
>+     SET_TITLE,
>+     GET_TITLE
>+  };
> 

>+  // move to nsWindow.cpp
>+  #define PM2NS_PARENT NS2PM_PARENT
>+  #define PM2NS NS2PM

So you don't like # in the first column? Oh well, then forget my previous comment about this. Obviously you are the one to maintain this code for the forseeable future so you should be using your style where Mozilla rules don't dictate otherwise.

>+  #define isNumlockOn (BOOL)WinGetKeyState(HWND_DESKTOP, VK_NUMLOCK) & 0x0001
>+  #define WinIsKeyDown(vk) ((WinGetKeyState(HWND_DESKTOP,vk) & 0x8000) ? PR_TRUE : PR_FALSE)
>+//  extern PRUint32       WMChar2KeyCode(MPARAM mp1, MPARAM mp2);

Why are you adding this again commented out?

Comment 22

[Peter Weilbacher]

Comment on attachment 408798 [details] [diff] [review]
cm05 - reformat nsFrameWindow.*

OK, but if you want to remove this why are you still reformatting it?

Comment 23

[Peter Weilbacher]

Comment on attachment 409487 [details] [diff] [review]
cm06b - cleanup nsWindow.*

>+PRUint32 nsFrameWindow::GetFCFlags()

I don't think I have understood why you are adding this for the subclass. Will that become clear with the next patches?

>+  PRUint32 style = FCF_TITLEBAR | FCF_SYSMENU | FCF_TASKLIST |
>+                   FCF_CLOSEBUTTON | FCF_NOBYTEALIGN | FCF_AUTOICON;
>+
>+  if (mWindowType == eWindowType_dialog) {
>+    if (mBorderStyle == eBorderStyle_default) {
>+      style |= FCF_DLGBORDER;
>+    } else {
>+      style |= FCF_SIZEBORDER | FCF_MINMAX;
>+    }
>+  }
>+  else {
>+    style |= FCF_SIZEBORDER | FCF_MINMAX;
>+  }
>+
>+  if (gOS2Flags & kIsDBCS) {
>+    style |= FCF_DBE_APPSTAT;
>+  }
>+  if (mWindowType == eWindowType_invisible) {
>+    style &= ~FCF_TASKLIST;
>+  }
>+
>+  if (mBorderStyle != eBorderStyle_default && mBorderStyle != eBorderStyle_all) {
>+    if (mBorderStyle == eBorderStyle_none || !(mBorderStyle & eBorderStyle_resizeh)) {
>+      style &= ~FCF_SIZEBORDER;
>+      style |= FCF_DLGBORDER;
>+    }

Could you please fold all these long lines?

>+    if (mBorderStyle == eBorderStyle_none ||
>+      !(mBorderStyle & (eBorderStyle_menu | eBorderStyle_close))) {

Please shift the ! two spaces to the right, so that it's clear where it belongs to.


>+#define PMSCAN_PAD6         0x4D
>+#define PMSCAN_PADPLUS      0x4E
>+#define PMSCAN_PAD1         0x4F
>+#define PMSCAN_PAD2         0x50
>+#define PMSCAN_PAD3         0x51
>+#define PMSCAN_PAD0         0x52
>+#define PMSCAN_PADPERIOD    0x53
>+#define PMSCAN_PADDIV       0x5c
>+
>+#define isNumPadScanCode(scanCode) !((scanCode < PMSCAN_PAD7) ||      \
>+                                     (scanCode > PMSCAN_PADPERIOD) || \
>+                                     (scanCode == PMSCAN_PADMULT) ||  \
>+                                     (scanCode == PMSCAN_PADDIV) ||   \
>+                                     (scanCode == PMSCAN_PADMINUS) || \
>+                                     (scanCode == PMSCAN_PADPLUS))
>+
>+#define isNumlockOn     (WinGetKeyState(HWND_DESKTOP, VK_NUMLOCK) & 0x0001)
>+#define isKeyDown(vk)   ((WinGetKeyState(HWND_DESKTOP,vk) & 0x8000) == 0x8000)

Seeing all of these again I guess the previous patch was not so useful...

Comment 24

[Peter Weilbacher]

Comment on attachment 406878 [details] [diff] [review]
cm00 -eliminate gcc 4.x warnings [checkin comment 24]

http://hg.mozilla.org/mozilla-central/rev/379dddd8c052

Comment 25

[Rich Walsh]

Peter, a general response to many of your comments...

This is being done in stages primarily to benefit you, the reviewer.  I could have created one mega-patch but it would have been nearly impossible to track the changes.  For example, I divided cm06 into two parts so I could document in cm06a what happened to 60+ lines in nsWindow.h that are missing after cm06b is applied.

Another reason for this approach is to make it easier to find bugs I may have introduced.  After applying each patch, all of widget/src/os2 should compile & run with no perceptible differences.  If a bug is found, it will be easy to roll them back, one by one, until the bug disappears.

When I'm done, all of your requests & concerns will have been addressed (formatting, XXX comments, etc.).  However, I really don't want to do so by modifying any of the existing patches.  Many of them cover the same lines of code, so changing one means regenerating & reposting all of them.  Instead, I'll take care of them in future patches (there are at least 3 more to come).

Comment 26

[Rich Walsh]

Peter, some specific responses to your comments...

>>+ // Set the frame control HWNDs as properties for use by fullscreen mode.
>>+ // XXX Making these data members of nsFrameWindow would probably be better.
> Why don't you do that right now?

Because it's part of my next project, after this one is done.  I'll remove the XXX comment.

>>+PRUint32 nsFrameWindow::GetFCFlags()
>
> I don't think I have understood why you are adding this for the subclass.
> Will that become clear with the next patches?

Only frame windows use FCF flags, so why not move it to the file that calls this function?  nsWindow never references it.

>>+  // If a TrackPoint is in use, create dummy scrollbars.
>>+  // XXX  Popups may need this also to scroll comboboxes.
>
> Where does this XXX come from? The old code also worked without any test
> for eWindowType_popup (if that's what you mean). If this TrackPoint
> workaround is still necessary, don't you have to leave the rest of the
> scrollbar stuff in, too? I mean everything with mIsScrollBar?

There are two separate scrollbar issues here:

- Mozilla used to use native scrollbar widgets, i.e. nsWindow objects that created scrollbar windows rather than MozillaWindowClass windows.  That's long gone but some of the support code remained (e.g. mIsScrollbar & tests for WC_SCROLLBAR).  Since it was totally obsolete, I got rid of it.

- To this day, the Trackpoint driver for both OS/2 & Windows won't send scroll msgs to windows that don't have native scrollbars.  This suddenly became such a big issue for the Windows version a few months ago that they implemented a clone of the OS/2 solution.  So, I guess it's still needed.  My only question is whether a trackpoint can scroll a combobox's list - the list is of type 'popup', not type 'child'.  I'll ask in the newsgroups.

>>+ * John Fairhurst 17-09-98 first version
>
> I think you should remove this block.

OK, done.  What about the list of IBM contributions in nsWindow.h?

Comment 27

[Peter Weilbacher]

(In reply to comment #25)
> Peter, a general response to many of your comments...
> 
> This is being done in stages primarily to benefit you, the reviewer.  I could
> have created one mega-patch but it would have been nearly impossible to track
> the changes.

Yes, and I very much appreciate that. It's just sometimes difficult to understand things in intermediate patches when I don't fully grasp the big picture of what you are doing.

> When I'm done, all of your requests & concerns will have been addressed
> (formatting, XXX comments, etc.).  However, I really don't want to do so by
> modifying any of the existing patches.

OK. I guess I'll wait a bit more for the rest of your patches to appear, before pushing the existing ones. None of this is necessary for 1.9.2 so we don't need to rush.


(In reply to comment #26)
> >>+PRUint32 nsFrameWindow::GetFCFlags()
> >
> > I don't think I have understood why you are adding this for the subclass.
> > Will that become clear with the next patches?
> 
> Only frame windows use FCF flags, so why not move it to the file that calls
> this function?  nsWindow never references it.

My bad, I missed that you are removing the implementation from nsWindow in the same patch.

> >>+ * John Fairhurst 17-09-98 first version
> >
> > I think you should remove this block.
> 
> OK, done.  What about the list of IBM contributions in nsWindow.h?

I think that can go, too. Unless Mike has strong feelings about it.

Comment 28

[Mike Kaply [:mkaply]]

Yes, you can remove the IBM stuff

Comment 29

[Rich Walsh]

(In reply to comment #27)
> It's just sometimes difficult to understand things in intermediate patches
> when I don't fully grasp the big picture of what you are doing.

There's no big picture here.  These files are full of junk and I'm removing it.  While I'm at it, I'm also reorganizing them.  That's it.

> I guess I'll wait a bit more for the rest of your patches to appear, before
> pushing the existing ones. None of this is necessary for 1.9.2 so we don't > need to rush.

Waiting makes it very difficult to confirm that no bugs have slipped in.  I've tested these patches & everything seems OK, but only more extensive use will confirm this, particularly wrt SM & TB.

The next patch will be, by far, the biggest.  If any problems arise, I'd like to feel confident that they can be attributed to the latest one and not the 7 patches that preceded it.

Comment 30

[Peter Weilbacher]

I won't have time to build any nightlies in the next weeks, probably not for FF trunk and certainly not for SM/TB using mozilla-central. So pushing the current set to trunk won't help for what you have in mind.

If you want more widespread testing, please create package(s) with the current set of patches in, and post about it in the newsgroup.

Comment 31

[Rich Walsh]

I've been sitting on my remaining patches for almost a month waiting for something to happen with the existing ones.  I'm posting them now in the hope that the current code freeze means they won't go bad for a while.

Comment 32

[Rich Walsh]

Attachment: cm06b - cleanup nsWindow.* - v2

All windows were getting a hidden scrollbar - this fixes the bug.

Comment 33

[Rich Walsh]

Attachment: cm07a - reformat nsWindow

This completely reorganizes & reformats nsWindow*.*.  It imposes consistent indentation, use of braces, positioning of '*' and '&', commenting style, and anything else that doesn't involve "words".  Methods have been rearranged and grouped based on function & volume of code.  It also addresses formatting issues mentioned in comments on the previous patches.

Comment 34

[Rich Walsh]

Attachment: cm07b - refactor nsWindow

This imposes a consistent coding style and refactors & renames a number of methods.

It replaces constructs like "if (nsnull != foo)" with "if (foo)", substitutes NSPR typedefs for OS/2 versions where possible, and makes other textual changes to improve consistency, readability, and standards compliance.

Dead or obsolete code has been removed, and several methods have been renamed to better reflect what they do.  Some trivial methods have been eliminated by inlining their code while a few new ones have been created, largely to remove big blocks of code from the window procedure.  I have avoided "improving" the code except where I was already making significant changes.

Comment 35

[Rich Walsh]

Attachment: cm08 - tweak nsWindow

This is a grab-bag of code improvements:  simplification of contorted logic, elimination of pointless code, consistent use of the GetMainWindow() method, etc.  It also eliminates the 'virtual' keyword from every nsWindow method that isn't overridden by nsFrameWindow.

Comment 36

[Rich Walsh]

Attachment: cm09 - rollup handling

This fixes a bug, simplifies rollup code, and gives the handlers meaningful names.

The bug:  display a menu from the menubar, then click on the system menu button.  The system menu pops up but the Mozilla menu fails to rollup.  The reason is that the handler only does a rollup if the window that just got the focus is a descendent of a different frame window.  In this case, it isn't.

This patch replaces DealWithPopups() and bothFromSameWindow() with RollupOnButtonDown() and RollupOnFocusLost(), simplifies the logic, and also tweaks the wndproc to make the process more self-evident.

Comment 37

[Rich Walsh]

Attachment: cm10 - reconfigure the MozillaWindowClass

IIUC, mozilla used to turn native controls into widgets by subclassing them.  For consistency, it also subclassed the windows it created to avoid special-case logic.  Further, the code used the alien notion of attaching a "property" to a window to store a pointer to its nsWindow object - presumably, because there was nowhere else to put it.

None of this applies anymore, so there's no reason to preserve this legacy baggage.  With this patch, the MozillaWindowClass is registered with its own window procedure (i.e. the existing fnwpNSWindow() function), and an additional 4 bytes is allocated in the "window words" to hold the nsWindow pointer.  The eliminates the need for subclassing & calling the previous wndproc, as well as the use of WinQuery/SetProperty().  Unhandled msgs go directly to WinDefWindowProc(), and the nsWindow pointer is accessed via WinQuery/SetWindowPtr(QWL_USER+4).

Comment 38

[Rich Walsh]

Attachment: cm01 thru cm10 - combined patch

Applying 12 patches can be a pain, so this combines all of them into a single omnibus patch that's easier to deal with (note:  cm00 was already checked in so it isn't included).

Comment 39

[Rich Walsh]

This set of code-maintenance patches is complete.  See Bug 533200 for a follow-on patch.

Comment 40

[Rich Walsh]

Attachment: cm07a - reformat nsWindow - v2

bit-rot had set in...

Comment 41

[Rich Walsh]

Attachment: cm07b - refactor nsWindow - v2

Trivial bug fix:  prevent the Alt key from popping up the system menu (I don't know why Mozilla wants to suppress this, but that's what the original code did).  While I was at it, I suppressed that action for Alt-Graf too.

Comment 42

[Rich Walsh]

Attachment: cm01 thru cm10 - combined patch - v2

Updated to include changes to cm07a & cm07b.

Comment 43

[Rich Walsh]

Attachment: cm06a - reformat nsWindow.h - v2

Comment 44

[Rich Walsh]

Attachment: cm07a - reformat nsWindow - v3

Comment 45

[Rich Walsh]

Attachment: cm07b -refactor nsWindow - v3

Comment 46

[Rich Walsh]

Attachment: cm08 - tweak nsWindow - v2

Comment 47

[Rich Walsh]

Attachment: cm09 - rollup handling - v2

Comment 48

[Rich Walsh]

Attachment: cm01 thru cm10 - combined patch - v3 [Checkin: Comment 52]

Walter, could you approve these?  As 3 months of use by you, me, Dave, and others have demonstrated, they do what they should: clean up the code without introducing new bugs.

Comment 49

[Walter Meinl]

(In reply to comment #48)
> Created an attachment (id=424410) [details]
> cm01 thru cm10 - combined patch - v3
> 
> Walter, could you approve these?  As 3 months of use by you, me, Dave, and
> others have demonstrated, they do what they should: clean up the code without
> introducing new bugs.

I rebuilt widget and xul.dll after application of every patch. Each of the steps from the whole patch compiled and did not alter behavior of widgets unexpectedly. The combined patch leads to the same code as the concatenation of the single patches, so we should get it in.

Comment 50

[Walter Meinl]

Rich, to get this in, it needs to set the checkin-needed keyword. In the whiteboard field the attachment of the complete patch should be mentioned and its probably a good idea to add in the whiteboard that its OS/2 only files.
As this bug is assigned to you I did not want to "hijack" it, but can do if you agree. The patch still applies and it should certainly be checked in such that a fix for bug549426 will be on the new code.

Comment 51

[Rich Walsh]

(In reply to comment #50)
> Rich, to get this in, it needs to set the checkin-needed keyword.

Thanks, Walter.  I was distracted by another project (fixing sound playback in Flash10) and didn't think to do this.

Comment 52

[Serge Gautherie (:sgautherie)]

Comment on attachment 424410 [details] [diff] [review]
cm01 thru cm10 - combined patch - v3
[Checkin: Comment 52]


http://hg.mozilla.org/mozilla-central/rev/c4169105a5d4