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]
:
:
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 Doug Turner (:dougt) 2009-01-14 17:02:58 PST
Created attachment 357072 [details] [diff] [review]
patch v.1
Comment 1 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 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 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 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 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 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 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 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 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 Doug Turner (:dougt) 2011-04-22 15:15:38 PDT
Comment on attachment 527164 [details] [diff] [review]
Part A - File removal

rs+
Comment 12 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 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 Doug Turner (:dougt) 2011-04-22 15:19:31 PDT
and thanks!
Comment 15 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 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 Ed Morley [:emorley] 2011-04-24 13:53:27 PDT
Also, any idea about comment 5?
Comment 18 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 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 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 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 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 Ed Morley [:emorley] 2011-04-25 00:51:33 PDT
Thanks Zack, much obliged :-)
Comment 25 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.