Drop support for pre-Win2k platforms from Win32 widgets

RESOLVED FIXED

Status

()

--
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: sciguyryan, Assigned: sciguyryan)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 11 obsolete attachments)

14.44 KB, patch
Details | Diff | Splinter Review
1.10 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

12 years ago
Assignee: win32 → sciguyryan+bugzilla
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

12 years ago
Created attachment 245902 [details] [diff] [review]
Patch v1

Patch v1.

First try at a patch, killed as much as I could, if I missed anything please let me know!
Attachment #245902 - Flags: review?(emaijala)

Comment 2

12 years ago
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.
Attachment #245902 - Flags: review?(emaijala) → review-
(Assignee)

Comment 3

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

Comment 4

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

Updated

12 years ago
Attachment #245945 - Attachment is obsolete: true
Attachment #245945 - Flags: review?(emaijala)
(Assignee)

Comment 5

12 years ago
Created attachment 245949 [details] [diff] [review]
Patch v1

Patch v1

* Simple changes too nsKeyboardLayout.h, nsLookAndFeel.cpp and nsToolkit.cpp.
Attachment #245949 - Flags: review?(emaijala)

Comment 6

12 years ago
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.
Attachment #245949 - Flags: review?(emaijala) → review-
(Assignee)

Comment 7

12 years ago
Created attachment 245975 [details] [diff] [review]
Patch v2

Patch v2

* Simple changes too nsKeyboardLayout.h, nsLookAndFeel.cpp and nsToolkit.cpp - updated.
Attachment #245949 - Attachment is obsolete: true
Attachment #245975 - Flags: review?(emaijala)
(Assignee)

Updated

12 years ago
Attachment #245975 - Flags: review?(emaijala)
(Assignee)

Comment 8

12 years ago
Created attachment 245977 [details] [diff] [review]
Patch v2.1 - a little better.

Patch v2.1

* Like Patch v2, fixed a mistake and updated the code to the file standards used.
Attachment #245975 - Attachment is obsolete: true
Attachment #245977 - Flags: review?(emaijala)

Comment 9

12 years ago
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
Attachment #245977 - Flags: review?(emaijala) → review+
(Assignee)

Comment 10

12 years ago
Created attachment 245986 [details] [diff] [review]
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.
Attachment #245977 - Attachment is obsolete: true
Attachment #245986 - Flags: superreview?(blizzard)

Comment 11

12 years ago
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.
Attachment #245986 - Flags: superreview?(blizzard) → review-
(Assignee)

Comment 12

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

12 years ago
Sure they can be moved, and while at it you could remove those that aren't actually used anywhere...
(Assignee)

Comment 14

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

Comment 15

12 years ago
Created attachment 246035 [details] [diff] [review]
Patch v3

Patch v3.

* Includes some define clean-up.
Attachment #245986 - Attachment is obsolete: true
Attachment #246035 - Flags: review?(emaijala)

Comment 16

12 years ago
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.
Attachment #246035 - Flags: review?(emaijala) → review-
(Assignee)

Comment 17

12 years ago
Created attachment 246557 [details] [diff] [review]
Patch v3.1

* Patch v3.1

Addressed all of Ere's comments - thanks for reviewing!
Attachment #246035 - Attachment is obsolete: true
Attachment #246557 - Flags: review?(emaijala)
(Assignee)

Updated

12 years ago
Attachment #246557 - Attachment is obsolete: true
Attachment #246557 - Flags: review?(emaijala)
(Assignee)

Comment 18

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

Comment 19

12 years ago
Created attachment 246559 [details] [diff] [review]
Patch v3.2

* Patch v3.2

Non-broken patch this time.
Attachment #246559 - Flags: review?(emaijala)

Comment 20

12 years ago
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
Attachment #246559 - Flags: review?(emaijala) → review+
(Assignee)

Comment 21

12 years ago
Created attachment 246561 [details] [diff] [review]
Patch v3.2 with corrected indenting.

Requesting sr=Blizzard.

Thanks for all your reviewing Ere.
Attachment #246559 - Attachment is obsolete: true
Attachment #246561 - Flags: superreview?(blizzard)
(Assignee)

Updated

12 years ago
Attachment #246561 - Attachment is obsolete: true
Attachment #246561 - Flags: superreview?(blizzard)
(Assignee)

Comment 22

12 years ago
Created attachment 246562 [details] [diff] [review]
Final (hopefully)
Attachment #246562 - Flags: superreview?(blizzard)
Comment on attachment 246562 [details] [diff] [review]
Final (hopefully)

Blizzard isn't an active reviewer (his realname says it, too :), I recommend roc.
Attachment #246562 - Flags: superreview?(blizzard)
(Assignee)

Updated

12 years ago
Attachment #246562 - Flags: superreview?(roc)
 #ifdef WINCE
-#define NS_VK_APP1  0x0201

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

Comment 25

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

Comment 26

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

12 years ago
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.
Attachment #246562 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
> 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?
(Assignee)

Comment 30

12 years ago
(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?)
(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|.
> by return value from |SystemParametersInfo|.
correction:
 by |isFlatMenus|.
(Assignee)

Comment 33

12 years ago
Created attachment 250603 [details] [diff] [review]
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.
Attachment #250603 - Flags: superreview?(emaijala)
Attachment #250603 - Flags: review?(VYV03354)

Comment 34

12 years ago
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.
Attachment #250603 - Flags: superreview?(roc)
Attachment #250603 - Flags: superreview?(emaijala)
Attachment #250603 - Flags: review?(VYV03354)
Attachment #250603 - Flags: review+
(Assignee)

Comment 35

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

(Assignee)

Updated

12 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #250603 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 36

12 years ago
Created attachment 250772 [details] [diff] [review]
Fix patch for checkin

Minor fix patch with the comment updated as requested ready for checkin.
Attachment #250603 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
mozilla/widget/src/windows/nsNativeThemeWin.cpp 	3.73
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.