OS/2 Code Maintenance

RESOLVED FIXED in mozilla1.9.3a3

Status

()

Core
General
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Rich Walsh, Assigned: Rich Walsh)

Tracking

Trunk
mozilla1.9.3a3
x86
OS/2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 11 obsolete attachments)

5.46 KB, patch
Peter Weilbacher
: review+
Details | Diff | Splinter Review
24.79 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
17.95 KB, patch
Peter Weilbacher
: review+
Details | Diff | Splinter Review
3.51 KB, patch
Peter Weilbacher
: review+
Details | Diff | Splinter Review
4.23 KB, patch
Peter Weilbacher
: review+
Details | Diff | Splinter Review
22.93 KB, patch
Peter Weilbacher
: review+
Details | Diff | Splinter Review
36.66 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
5.75 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
36.06 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
214.99 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
61.33 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
27.67 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
7.32 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
273.16 KB, patch
Walter Meinl
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

8 years ago
Created attachment 406878 [details] [diff] [review]
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.
Attachment #406878 - Flags: review?(mozilla)
(Assignee)

Comment 2

8 years ago
Created attachment 406879 [details] [diff] [review]
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.
Attachment #406879 - Flags: review?(mozilla)
(Assignee)

Updated

8 years ago
Attachment #406878 - Attachment description: eliminate gcc 4.x warnings → cm00 -eliminate gcc 4.x warnings
(Assignee)

Updated

8 years ago
Attachment #406879 - Attachment description: remove nsdefs.h & nsWidgetDefs.h → cm01 - remove nsdefs.h & nsWidgetDefs.h
(Assignee)

Comment 3

8 years ago
Created attachment 406880 [details] [diff] [review]
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.
Attachment #406880 - Flags: review?(mozilla)

Comment 4

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

Comment 5

8 years ago
(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

8 years ago
(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

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

Comment 8

8 years ago
Created attachment 408795 [details] [diff] [review]
cm01 - remove nsdefs.h & nsWidgetDefs.h v2

These things are already starting to bit-rot...
Attachment #406879 - Attachment is obsolete: true
Attachment #406879 - Flags: review?(mozilla)
(Assignee)

Comment 9

8 years ago
Created attachment 408796 [details] [diff] [review]
cm03 - remove references to native scrollbars

This removes references to the native scrollbars that Mozilla no longer uses (e.g. no more "if (mScrollbar) ...).
Attachment #408796 - Flags: review?(mozilla)
(Assignee)

Updated

8 years ago
Attachment #408796 - Attachment description: remove references to native scrollbars → cm03 - remove references to native scrollbars
(Assignee)

Comment 10

8 years ago
Created attachment 408797 [details] [diff] [review]
cm04 - fullscreen fix for v1.9.3

This fixes fullscreen (aka kiosk-mode) issues for v1.9.3.  See Bug 524258 for details.
Attachment #408797 - Flags: review?(mozilla)
(Assignee)

Comment 11

8 years ago
Created attachment 408798 [details] [diff] [review]
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).
Attachment #408798 - Flags: review?(mozilla)
(Assignee)

Updated

8 years ago
Attachment #408795 - Flags: review?(mozilla)

Comment 12

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

Comment 13

8 years ago
(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

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

Comment 15

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

Comment 16

8 years ago
Created attachment 409486 [details] [diff] [review]
cm06a - reformat nsWindow.h
Attachment #409486 - Flags: review?(mozilla)
(Assignee)

Comment 17

8 years ago
Created attachment 409487 [details] [diff] [review]
cm06b - cleanup nsWindow.*
Attachment #409487 - Flags: review?(mozilla)

Updated

8 years ago
Attachment #406878 - Flags: review?(mozilla) → review+

Comment 18

8 years ago
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?

Updated

8 years ago
Attachment #408796 - Flags: review?(mozilla) → review+

Comment 19

8 years ago
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.

Updated

8 years ago
Attachment #408795 - Flags: review?(mozilla) → review+

Comment 20

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

Haven't tried it out yet, but cool. :-)
Attachment #408797 - Flags: review?(mozilla) → review+

Comment 21

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

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

OK, but if you want to remove this why are you still reformatting it?
Attachment #408798 - Flags: review?(mozilla) → review+

Comment 23

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

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

http://hg.mozilla.org/mozilla-central/rev/379dddd8c052
Attachment #406878 - Attachment description: cm00 -eliminate gcc 4.x warnings → cm00 -eliminate gcc 4.x warnings [checkin comment 24]
(Assignee)

Comment 25

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

Comment 26

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

8 years ago
(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.

Updated

8 years ago
Attachment #409487 - Flags: review?(mozilla) → review+
Yes, you can remove the IBM stuff
(Assignee)

Comment 29

8 years ago
(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

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

Comment 31

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

Comment 32

8 years ago
Created attachment 416336 [details] [diff] [review]
cm06b - cleanup nsWindow.* - v2

All windows were getting a hidden scrollbar - this fixes the bug.
Attachment #409487 - Attachment is obsolete: true
(Assignee)

Comment 33

8 years ago
Created attachment 416338 [details] [diff] [review]
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.
(Assignee)

Comment 34

8 years ago
Created attachment 416343 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Attachment #416343 - Attachment description: refactor nsWindow → cm07b - refactor nsWindow
(Assignee)

Comment 35

8 years ago
Created attachment 416345 [details] [diff] [review]
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.
(Assignee)

Comment 36

8 years ago
Created attachment 416350 [details] [diff] [review]
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.
(Assignee)

Comment 37

8 years ago
Created attachment 416353 [details] [diff] [review]
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).
(Assignee)

Comment 38

8 years ago
Created attachment 416354 [details] [diff] [review]
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).
(Assignee)

Updated

8 years ago
Blocks: 533200
(Assignee)

Comment 39

8 years ago
This set of code-maintenance patches is complete.  See Bug 533200 for a follow-on patch.
(Assignee)

Comment 40

8 years ago
Created attachment 418594 [details] [diff] [review]
cm07a - reformat nsWindow - v2

bit-rot had set in...
Attachment #416338 - Attachment is obsolete: true
(Assignee)

Comment 41

8 years ago
Created attachment 418595 [details] [diff] [review]
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.
Attachment #416343 - Attachment is obsolete: true
(Assignee)

Comment 42

8 years ago
Created attachment 418596 [details] [diff] [review]
cm01 thru cm10 - combined patch - v2

Updated to include changes to cm07a & cm07b.
Attachment #416354 - Attachment is obsolete: true
(Assignee)

Comment 43

8 years ago
Created attachment 424405 [details] [diff] [review]
cm06a - reformat nsWindow.h - v2
Attachment #409486 - Attachment is obsolete: true
Attachment #424405 - Flags: review?(wuno)
Attachment #409486 - Flags: review?(mozilla)
(Assignee)

Comment 44

8 years ago
Created attachment 424406 [details] [diff] [review]
cm07a - reformat nsWindow - v3
Attachment #418594 - Attachment is obsolete: true
Attachment #424406 - Flags: review?(wuno)
(Assignee)

Comment 45

8 years ago
Created attachment 424407 [details] [diff] [review]
cm07b -refactor nsWindow - v3
Attachment #418595 - Attachment is obsolete: true
Attachment #424407 - Flags: review?(wuno)
(Assignee)

Comment 46

8 years ago
Created attachment 424408 [details] [diff] [review]
cm08 - tweak nsWindow - v2
Attachment #416345 - Attachment is obsolete: true
Attachment #424408 - Flags: review?(wuno)
(Assignee)

Comment 47

8 years ago
Created attachment 424409 [details] [diff] [review]
cm09 - rollup handling - v2
Attachment #416350 - Attachment is obsolete: true
Attachment #424409 - Flags: review?(wuno)
(Assignee)

Comment 48

8 years ago
Created attachment 424410 [details] [diff] [review]
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.
Attachment #418596 - Attachment is obsolete: true
Attachment #424410 - Flags: review?(wuno)
(Assignee)

Updated

8 years ago
Attachment #416336 - Flags: review?(wuno)
(Assignee)

Updated

8 years ago
Attachment #416353 - Flags: review?(wuno)
(Assignee)

Updated

8 years ago
Attachment #406880 - Flags: review?(mozilla) → review?(wuno)

Updated

8 years ago
Attachment #424405 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #416336 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #424406 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #424407 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #424408 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #424409 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #416353 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #406880 - Flags: review?(wuno) → review+

Updated

8 years ago
Attachment #424410 - Flags: review?(wuno) → review+

Comment 49

8 years ago
(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

8 years ago
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.

Updated

8 years ago
Blocks: 549426
(Assignee)

Comment 51

8 years ago
(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.
Keywords: checkin-needed
Whiteboard: OS/2 only file ("combined patch-v3" only / id=424410)
Comment on attachment 424410 [details] [diff] [review]
cm01 thru cm10 - combined patch - v3
[Checkin: Comment 52]


http://hg.mozilla.org/mozilla-central/rev/c4169105a5d4
Attachment #424410 - Attachment description: cm01 thru cm10 - combined patch - v3 → cm01 thru cm10 - combined patch - v3 [Checkin: Comment 52]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: OS/2 only file ("combined patch-v3" only / id=424410)
Target Milestone: --- → mozilla1.9.3a3

Updated

6 years ago
Depends on: 684487
You need to log in before you can comment on or make changes to this bug.