Last Comment Bug 473687 - Widget removal of unneeded WinCE stuff
: Widget removal of unneeded WinCE stuff
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows CE
-- normal (vote)
: mozilla6
Assigned To: Ed Morley [:emorley]
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks: 614720
  Show dependency treegraph
 
Reported: 2009-01-14 17:02 PST by Doug Turner (:dougt)
Modified: 2011-07-07 09:29 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v.1 (1.75 KB, patch)
2009-01-14 17:02 PST, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
Part A - File removal (36.76 KB, patch)
2011-04-19 17:44 PDT, Ed Morley [:emorley]
doug.turner: review+
Details | Diff | Splinter Review
Part B - Main ifdef removal (153.07 KB, patch)
2011-04-19 17:59 PDT, Ed Morley [:emorley]
doug.turner: review+
Details | Diff | Splinter Review
Part C - Remove ENABLE_IME_MOUSE_HANDLING (6.02 KB, patch)
2011-04-19 18:06 PDT, Ed Morley [:emorley]
doug.turner: review+
Details | Diff | Splinter Review
Part D - Remove nsWindowGfx::OnSettingsChangeGfx & nsWindow::OnSettingsChange (4.18 KB, patch)
2011-04-19 18:16 PDT, Ed Morley [:emorley]
doug.turner: review+
Details | Diff | Splinter Review
Part A - File removal (36.77 KB, patch)
2011-04-24 18:36 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review
Part B - Main ifdef removal (153.11 KB, patch)
2011-04-24 18:37 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review
Part C - Remove ENABLE_IME_MOUSE_HANDLING (6.03 KB, patch)
2011-04-24 18:38 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review
Part D - Remove nsWindowGfx::OnSettingsChangeGfx & nsWindow::OnSettingsChange (4.19 KB, patch)
2011-04-24 18:39 PDT, Ed Morley [:emorley]
emorley: review+
Details | Diff | Splinter Review

Description User image Doug Turner (:dougt) 2009-01-14 17:02:58 PST
Created attachment 357072 [details] [diff] [review]
patch v.1
Comment 1 User image Ere Maijala (slow) 2009-02-12 07:57:33 PST
Comment on attachment 357072 [details] [diff] [review]
patch v.1

r=me for the remaining nsScreenWin.cpp part.
Comment 2 User image Ed Morley [:emorley] 2011-04-08 06:33:38 PDT
Doug, I'm happy to unbitrot this, but before I do so:

1) Is this still wanted? ie: Your reply in bug 614720 comment 2 implies hesitance to strip out WinCE stuff. However, Mike's comment in bug 647389 comment 4 seems to imply that removing WinCE is acceptable, given that a lot of things would need to change if WinCE was supported in the future anyway.

2) There is a lot more other widget WINCE stuff that can be stripped out, per:
http://mxr.mozilla.org/mozilla-central/search?string=wince&find=widget
...would you want me to do that too?
Comment 3 User image Ed Morley [:emorley] 2011-04-19 05:45:24 PDT
(In reply to comment #2)
> 1) Is this still wanted?

Answering my own question: WinCE/Windows Mobile support has already been removed from the main build system, Spidermonkey, mobile installer, in-app updater and so on (see bug 614720, bug 554087 and all their dependants); so building isn't possible on WinCE anyway. Until such point where MS decide to release a Windows Phone 7 NDK and the decision is made to port to that platform, the WinCE/WinMO code is unnecessary cruft that is only going to complicate code maintenance, so IMHO should be removed. In X years time if a port goes ahead, and if any of the old WinCE code is deemed still useful for reference; then that's what a VCS is for.

> 2) There is a lot more other widget WINCE stuff that can be stripped out, per:
> http://mxr.mozilla.org/mozilla-central/search?string=wince&find=widget

Since the bug title is about widget code in general; going to just cover it all here:

http://mxr.mozilla.org/mozilla-central/search?string=wince&find=%2Fwidget%2F&tree=mozilla-central

http://mxr.mozilla.org/mozilla-central/search?string=windows+ce&find=%2Fwidget%2F&tree=mozilla-central

http://mxr.mozilla.org/mozilla-central/search?string=windows+mob&find=%2Fwidget%2F&tree=mozilla-central
Comment 5 User image Ed Morley [:emorley] 2011-04-19 14:30:23 PDT
Almost done with the patch, but quick question:

Any idea whether SND_PURGE is still required?

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsSound.cpp#118
> #ifndef SND_PURGE
> // Not available on Windows CE, and according to MSDN
> // doesn't do anything on recent windows either.
> #define SND_PURGE 0
> #endif

It is only ever set to 0 and is only used here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsSound.cpp#144
and
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsSound.cpp#229

Also, the MSDN doc here says it's not supported:
http://msdn.microsoft.com/en-us/library/dd743680%28VS.85%29.aspx
Comment 6 User image Ed Morley [:emorley] 2011-04-19 17:44:57 PDT
Created attachment 527164 [details] [diff] [review]
Part A - File removal

Hg rm:
nsWindowCE.(h|cpp)
nsClipboardCE.(h|cpp)
Comment 7 User image Ed Morley [:emorley] 2011-04-19 17:59:15 PDT
Created attachment 527169 [details] [diff] [review]
Part B - Main ifdef removal

Removes all WinCE & Windows Mobile ifdef'ed code from widget/*

nsWindowGfx::OnSettingsChangeGfx is now empty as it previously only contained |ifdef WINCE_WINDOWS_MOBILE| reachable code. Will remove the placeholder comment separately (in part D) to keep things easier to follow.
Comment 8 User image Ed Morley [:emorley] 2011-04-19 18:06:51 PDT
Created attachment 527171 [details] [diff] [review]
Part C - Remove ENABLE_IME_MOUSE_HANDLING

After part B is applied, ENABLE_IME_MOUSE_HANDLING is always defined as 1:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIMM32Handler.h#61

This patch removes the now superfluous ENABLE_IME_MOUSE_HANDLING define/ifdefs:
http://mxr.mozilla.org/mozilla-central/search?string=ENABLE_IME_MOUSE_HANDLING
Comment 9 User image Ed Morley [:emorley] 2011-04-19 18:16:15 PDT
Created attachment 527173 [details] [diff] [review]
Part D - Remove nsWindowGfx::OnSettingsChangeGfx & nsWindow::OnSettingsChange

After part B is applied, nsWindowGfx::OnSettingsChangeGfx is now just an empty stub:
https://bugzilla.mozilla.org/attachment.cgi?id=527169&action=diff#a/widget/src/windows/nsWindowGfx.cpp_sec2

Its only caller is nsWindow::OnSettingsChange, which once the redundant call to nsWindowGfx::OnSettingsChangeGfx is removed, also now doesn't do anything:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#7668

Therefore this patch removes...
- nsWindowGfx::OnSettingsChangeGfx
- nsWindow::OnSettingsChange
- The only call to nsWindow::OnSettingsChange:
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsWindow.cpp#5501
Comment 10 User image Ed Morley [:emorley] 2011-04-19 18:21:40 PDT
Note: Parts C & D require B to have been applied first, or they won't apply cleanly.
Comment 11 User image Doug Turner (:dougt) 2011-04-22 15:15:38 PDT
Comment on attachment 527164 [details] [diff] [review]
Part A - File removal

rs+
Comment 12 User image Doug Turner (:dougt) 2011-04-22 15:17:01 PDT
Comment on attachment 527169 [details] [diff] [review]
Part B - Main ifdef removal

rs+
Comment 13 User image Doug Turner (:dougt) 2011-04-22 15:19:15 PDT
please push to try before pushing to m-c  (or use a project branch)
Comment 14 User image Doug Turner (:dougt) 2011-04-22 15:19:31 PDT
and thanks!
Comment 15 User image Ed Morley [:emorley] 2011-04-22 15:28:18 PDT
(In reply to comment #13)
> please push to try before pushing to m-c  (or use a project branch)

Cool will do (once bug 648541 happens).

Thanks for the review :-)
Comment 16 User image Ed Morley [:emorley] 2011-04-23 04:38:12 PDT
Pushed to try yesterday:
http://dev.philringnalda.com/tbpl/?tree=Try&rev=84e893e5949a

However for some reason hg decided that a load of changesets already in m-c needed to be included; was this just caused by the try repo not being up to date with m-c tip due to try being closed for part of yesterday? Or did I do something wrong?

Also, there were 3 oranges in the results, but from what I can tell they are intermittent failures. I've used the rebuild command in the self serve API to trigger those tests again; presuming this is what I'm supposed to do in those situations?

After rerunning the tests, all but one now passes - and that one has an associated bug that is perma-orange. However the bug only mentions one of the many errors for that test - ideas?

Sorry for all the questions; first time using try and the try server wiki page doesn't cover much beyond how to submit tests and where to look for results.

Thanks!
Comment 17 User image Ed Morley [:emorley] 2011-04-24 13:53:27 PDT
Also, any idea about comment 5?
Comment 18 User image Ed Morley [:emorley] 2011-04-24 18:36:53 PDT
Created attachment 528048 [details] [diff] [review]
Part A - File removal

Only change: Updated to tip & r=dougt added to commit message.
Comment 19 User image Ed Morley [:emorley] 2011-04-24 18:37:42 PDT
Created attachment 528049 [details] [diff] [review]
Part B - Main ifdef removal

Only change: Updated to tip & r=dougt added to commit message.
Comment 20 User image Ed Morley [:emorley] 2011-04-24 18:38:51 PDT
Created attachment 528050 [details] [diff] [review]
Part C - Remove ENABLE_IME_MOUSE_HANDLING

Only change: Updated to tip & r=dougt added to commit message.
Comment 21 User image Ed Morley [:emorley] 2011-04-24 18:39:34 PDT
Created attachment 528051 [details] [diff] [review]
Part D - Remove nsWindowGfx::OnSettingsChangeGfx & nsWindow::OnSettingsChange

Only change: Updated to tip & r=dougt added to commit message.
Comment 22 User image Ed Morley [:emorley] 2011-04-24 18:46:25 PDT
All four parts ready for checkin (passed try http://dev.philringnalda.com/tbpl/?tree=Try&rev=84e893e5949a and queries I had in comment 16 have been answered on IRC). r+ from dougt has been carried forwards.

Note: They need to be applied in order A->D, or there will be merge conflicts.

Thanks!
Comment 24 User image Ed Morley [:emorley] 2011-04-25 00:51:33 PDT
Thanks Zack, much obliged :-)
Comment 25 User image Ed Morley [:emorley] 2011-07-07 09:29:10 PDT
Doug, any ideas about this? Thanks :-)

(In reply to comment #5)
> Any idea whether SND_PURGE is still required?
> 
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsSound.
> cpp#118
> > #ifndef SND_PURGE
> > // Not available on Windows CE, and according to MSDN
> > // doesn't do anything on recent windows either.
> > #define SND_PURGE 0
> > #endif
> 
> It is only ever set to 0 and is only used here:
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsSound.
> cpp#144
> and
> http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsSound.
> cpp#229
> 
> Also, the MSDN doc here says it's not supported:
> http://msdn.microsoft.com/en-us/library/dd743680%28VS.85%29.aspx

Note You need to log in before you can comment on or make changes to this bug.