361154 - Drop support for pre-Win2k platforms from Win32 widgets

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Ryan Jones-Ward [:sciguyryan]]

Related to bug 359808 and bug 330276.

There seems to be some pre-Win2k code left under Win32 widgets, this bug will track the codes removal.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1.

First try at a patch, killed as much as I could, if I missed anything please let me know!

Comment 2

[Ere Maijala (slow)]

Comment on attachment 245902 [details] [diff] [review]
Patch v1

Even though we're running on Unicode enabled OS not every program uses Unicode, therefore we need to keep support for A flavors of clipboard data. Sorry for the trouble. Other than that the patch looks fine.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #2)
> (From update of attachment 245902 [details] [diff] [review] [edit])
> Even though we're running on Unicode enabled OS not every program uses Unicode,
> therefore we need to keep support for A flavors of clipboard data. Sorry for
> the trouble. Other than that the patch looks fine.
> 

So I guess that means nsClipboard.(h|cpp) needs to be left alone, associated constants and functions? I'll try and make smaller patches this time so they are easier to review and or correct as needed.

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1 for nsLookAndFeel.cpp

Patch v1 for nsLookAndFeel.cpp

* Only small changes, mostly removal of error checking from the Windows API call for SystemParametersInfo as it shouldn't fail on any Windows version we support.

Comment 5

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1

* Simple changes too nsKeyboardLayout.h, nsLookAndFeel.cpp and nsToolkit.cpp.

Comment 6

[Ere Maijala (slow)]

Comment on attachment 245949 [details] [diff] [review]
Patch v1

+        // This will default to the Window's default

Make that ' Windows' ' or remove the comment altogether and fix nsToolkit.cpp to compile and it starts to look quite good.

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2

* Simple changes too nsKeyboardLayout.h, nsLookAndFeel.cpp and nsToolkit.cpp - updated.

Comment 8

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.1 - a little better.

Patch v2.1

* Like Patch v2, fixed a mistake and updated the code to the file standards used.

Comment 9

[Ere Maijala (slow)]

Comment on attachment 245977 [details] [diff] [review]
Patch v2.1 - a little better.

+    OSVERSIONINFO osversion = { sizeof(OSVERSIONINFO) };

I'd like it better if you explicitly set osversion.dwOSVersionInfoSize (no need to clear the struct).

+        // This will default to the Window's default

The one you added is still wrong.

With those fixed, r=emaijala

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2.2

Patch v2.2

Thanks for the quick reviewing Ere!

* Patch changes per Ere's comments in comment #9 so requesting a super review.

Comment 11

[Ere Maijala (slow)]

Comment on attachment 245986 [details] [diff] [review]
Patch v2.2

This version has one fatal error: osversion is declared as OSVERSIONINFO but dwOSVersionInfoSize is set for size of OSVERSIONINFOEX.

Comment 12

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #11)
> (From update of attachment 245986 [details] [diff] [review] [edit])
> This version has one fatal error: osversion is declared as OSVERSIONINFO but
> dwOSVersionInfoSize is set for size of OSVERSIONINFOEX.
> 

Fixed that, silly mistake - glad you caught it!

I've got a question before I submit the correct patch.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/windows/nsToolkit.cpp&rev=3.69&cvsroot=/cvsroot&mark=162-166#160

Are these defines required to actually be here or can they be moved too the top of the file as is done on normal files?

Comment 13

[Ere Maijala (slow)]

Sure they can be moved, and while at it you could remove those that aren't actually used anywhere...

Comment 14

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #13)
> Sure they can be moved, and while at it you could remove those that aren't
> actually used anywhere...
> 

Good Idea. I'll clean up the constants that aren't used too. Thanks for the suggestion.

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3

Patch v3.

* Includes some define clean-up.

Comment 16

[Ere Maijala (slow)]

Comment on attachment 246035 [details] [diff] [review]
Patch v3

-#define SP_TRACKENDHOR     5
 #define SP_TRACKSTARTVERT  6
-#define SP_TRACKENDVERT    7

I'd leave these two in as they are part of a group and in the middle of it. Same goes for IMEMOUSE_ constants.

-#define FAPPCOMMAND_MASK  0xF000

No. It's used on the very next line (although the whole block is conditionally compiled in.

Otherwise it looks good.

Comment 17

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.1

* Patch v3.1

Addressed all of Ere's comments - thanks for reviewing!

Comment 18

[Ryan Jones-Ward [:sciguyryan]]

Ere, the last patch contained changes as suggested by JST Review (http://beaufour.dk/jst-review/). Would this be OK or does it produce un-needed diffs?

Comment 19

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.2

* Patch v3.2

Non-broken patch this time.

Comment 20

[Ere Maijala (slow)]

Comment on attachment 246559 [details] [diff] [review]
Patch v3.2

Yep, it's good. Fix the indentation here, though:

+     // GetSystemParam will return 0 on failure and we get non-flat as
+     // desired for Windows 2000 and sometimes on XP.
+      idx = (GetSystemParam(SPI_GETFLATMENU, 0)) ?
+                            COLOR_HIGHLIGHTTEXT :
+                            COLOR_MENUTEXT;

With this, r=emaijala

Comment 21

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.2 with corrected indenting.

Requesting sr=Blizzard.

Thanks for all your reviewing Ere.

Comment 22

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Final (hopefully)

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 246562 [details] [diff] [review]
Final (hopefully)

Blizzard isn't an active reviewer (his realname says it, too :), I recommend roc.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

 #ifdef WINCE
-#define NS_VK_APP1  0x0201

Is it OK to do this? Could stuff in this patch break winCE builds?

Comment 25

[Doug Turner (:dougt)]

the windows ce port doesn't build on the trunk.  Feel free to make it worse.  

Comment 26

[Ere Maijala (slow)]

Those defines are not used anywhere. Doug, will they be used somewhere in the near future? I don't want to deliberately make it worse..

Comment 27

[Doug Turner (:dougt)]

http://lxr.mozilla.org./mozilla1.8/source/widget/src/windows/nsWindow.cpp#84

these values were defined to that we can map various hardware buttons to key presses in the DOM.  I am fine if you remove them, but I am not sure the need.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/windows/nsDataObj.h 	1.37
mozilla/widget/src/windows/nsKeyboardLayout.h 	3.6
mozilla/widget/src/windows/nsLookAndFeel.cpp 	1.62
mozilla/widget/src/windows/nsNativeThemeWin.cpp 	3.72
mozilla/widget/src/windows/nsToolkit.cpp 	3.70
mozilla/widget/src/windows/nsWindow.cpp 	3.680

Comment 29

[Masatoshi Kimura [:emk]]

> void
> nsNativeThemeWin::UpdateConfig()
> {
>   // Check for Windows XP (or later), and check if 'flat menus' are enabled.
>   BOOL isFlatMenus;
>   mFlatMenus = PR_FALSE;
> 
>   // This will simply fail on Windows versions prior to XP, so we get
>   // non-flat as desired.
>   ::SystemParametersInfo(SPI_GETFLATMENU, 0, &isFlatMenus, 0);
>   mFlatMenus = isFlatMenus;
> }
|isFlatMenus| (hence mFlatMenus) will stay uninitialized on Win2k because |SystemParametersInfo| wouldn't touch params on failure.
Am I missing something?

Comment 30

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #29)
> |isFlatMenus| (hence mFlatMenus) will stay uninitialized on Win2k because
> |SystemParametersInfo| wouldn't touch params on failure.
> Am I missing something?
> 

I'm not sure it matters in this case, the two places that check the code use |if (mFlatMenus)| so I'd presume that it should work fine but either roc or Ere can make the final word on this. (Doesn't null = false for these checks anyway?)

Comment 31

[Masatoshi Kimura [:emk]]

(In reply to comment #30)
> I'm not sure it matters in this case, the two places that check the code use
> |if (mFlatMenus)| so I'd presume that it should work fine but either roc or Ere
> can make the final word on this. (Doesn't null = false for these checks
> anyway?)
|nsNativeThemeWin| will believe erroneously that the flat menu is supported on Win2k if |isFlatMenus| have a garbage other than zero.
>   BOOL isFlatMenus;
>   mFlatMenus = PR_FALSE;
should be:
>   BOOL isFlatMenus = PR_FALSE;
Anyway, |mFlatMenus = PR_FALSE| is a waste because it will always be overwritten by return value from |SystemParametersInfo|.

Comment 32

[Masatoshi Kimura [:emk]]

> by return value from |SystemParametersInfo|.
correction:
 by |isFlatMenus|.

Comment 33

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Minor fix

Should fix the problem bought up by Masatoshi. Use a conditional check when calling for |SystemParametersInfo| with the |SPI_GETFLATMENU| to make sure we get a real return and not some rubbish value that could be interpreted as a bool true later.

Comment 34

[Ere Maijala (slow)]

Comment on attachment 250603 [details] [diff] [review]
Minor fix

Good catch, I didn't notice the problem before.

I'd still like you to change the comment to "On Windows 2000 this SystemParametersInfo call will fail and we get non-flat as desired" as I managed to misread the proposed text. 

I'm not a super-reviewer but r=emaijala.

Comment 35

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #34)
> (From update of attachment 250603 [details] [diff] [review])
> Good catch, I didn't notice the problem before.
> 
> I'd still like you to change the comment to "On Windows 2000 this
> SystemParametersInfo call will fail and we get non-flat as desired" as I
> managed to misread the proposed text. 
> 

If I get a sr+ from roc I'll re-attach the patch with that change made for checkin then. Thanks for the quick review.

Comment 36

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Fix patch for checkin

Minor fix patch with the comment updated as requested ready for checkin.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/windows/nsNativeThemeWin.cpp 	3.73