Closed
Bug 361154
Opened 18 years ago
Closed 18 years ago
Drop support for pre-Win2k platforms from Win32 widgets
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sciguyryan, Assigned: sciguyryan)
References
()
Details
Attachments
(2 files, 11 obsolete files)
14.44 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
Details | Diff | Splinter Review |
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•18 years ago
|
Assignee: win32 → sciguyryan+bugzilla
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
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•18 years ago
|
Attachment #245945 -
Attachment is obsolete: true
Attachment #245945 -
Flags: review?(emaijala)
Assignee | ||
Comment 5•18 years ago
|
||
Patch v1 * Simple changes too nsKeyboardLayout.h, nsLookAndFeel.cpp and nsToolkit.cpp.
Attachment #245949 -
Flags: review?(emaijala)
Comment 6•18 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•18 years ago
|
||
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•18 years ago
|
Attachment #245975 -
Flags: review?(emaijala)
Assignee | ||
Comment 8•18 years ago
|
||
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•18 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•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
Sure they can be moved, and while at it you could remove those that aren't actually used anywhere...
Assignee | ||
Comment 14•18 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•18 years ago
|
||
Patch v3. * Includes some define clean-up.
Attachment #245986 -
Attachment is obsolete: true
Attachment #246035 -
Flags: review?(emaijala)
Comment 16•18 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•18 years ago
|
||
* 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•18 years ago
|
Attachment #246557 -
Attachment is obsolete: true
Attachment #246557 -
Flags: review?(emaijala)
Assignee | ||
Comment 18•18 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•18 years ago
|
||
* Patch v3.2 Non-broken patch this time.
Attachment #246559 -
Flags: review?(emaijala)
Comment 20•18 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•18 years ago
|
||
Requesting sr=Blizzard. Thanks for all your reviewing Ere.
Attachment #246559 -
Attachment is obsolete: true
Attachment #246561 -
Flags: superreview?(blizzard)
Assignee | ||
Updated•18 years ago
|
Attachment #246561 -
Attachment is obsolete: true
Attachment #246561 -
Flags: superreview?(blizzard)
Assignee | ||
Comment 22•18 years ago
|
||
Attachment #246562 -
Flags: superreview?(blizzard)
Comment 23•18 years ago
|
||
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•18 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•18 years ago
|
||
the windows ce port doesn't build on the trunk. Feel free to make it worse.
Comment 26•18 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•18 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•18 years ago
|
Whiteboard: [checkin needed]
Comment 28•18 years ago
|
||
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
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Comment 29•18 years ago
|
||
> 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•18 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?)
Comment 31•18 years ago
|
||
(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•18 years ago
|
||
> by return value from |SystemParametersInfo|.
correction:
by |isFlatMenus|.
Assignee | ||
Comment 33•18 years ago
|
||
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•18 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•18 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•18 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #250603 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 36•18 years ago
|
||
Minor fix patch with the comment updated as requested ready for checkin.
Attachment #250603 -
Attachment is obsolete: true
Assignee | ||
Updated•18 years ago
|
Whiteboard: [checkin needed]
Comment 37•18 years ago
|
||
mozilla/widget/src/windows/nsNativeThemeWin.cpp 3.73
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in
before you can comment on or make changes to this bug.
Description
•