473687 - Widget removal of unneeded WinCE stuff

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 1

[Ere Maijala (slow)]

Comment on attachment 357072 [details] [diff] [review]
patch v.1

r=me for the remaining nsScreenWin.cpp part.

Comment 2

[Ed Morley [:emorley]]

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]]

(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 4

[Ed Morley [:emorley]]

Files to be removed:
http://mxr.mozilla.org/mozilla-central/find?string=%2Fwidget%2Fsrc%2Fwindows%2Fns(Window|Clipboard)CE\..*&tree=mozilla-central

Consumers:
http://mxr.mozilla.org/mozilla-central/search?string=%28window|clipboard%29ce&regexp=1&filter=^[^\0]*%24&tree=mozilla-central

Comment 5

[Ed Morley [:emorley]]

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]]

Attachment: Part A - File removal

Hg rm:
nsWindowCE.(h|cpp)
nsClipboardCE.(h|cpp)

Comment 7

[Ed Morley [:emorley]]

Attachment: 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]]

Attachment: 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]]

Attachment: 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]]

Note: Parts C & D require B to have been applied first, or they won't apply cleanly.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 527164 [details] [diff] [review]
Part A - File removal

rs+

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 527169 [details] [diff] [review]
Part B - Main ifdef removal

rs+

Comment 13

[Doug Turner (:dougt)]

please push to try before pushing to m-c  (or use a project branch)

Comment 14

[Doug Turner (:dougt)]

and thanks!

Comment 15

[Ed Morley [:emorley]]

(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]]

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]]

Also, any idea about comment 5?

Comment 18

[Ed Morley [:emorley]]

Attachment: Part A - File removal

Only change: Updated to tip & r=dougt added to commit message.

Comment 19

[Ed Morley [:emorley]]

Attachment: Part B - Main ifdef removal

Only change: Updated to tip & r=dougt added to commit message.

Comment 20

[Ed Morley [:emorley]]

Attachment: Part C - Remove ENABLE_IME_MOUSE_HANDLING

Only change: Updated to tip & r=dougt added to commit message.

Comment 21

[Ed Morley [:emorley]]

Attachment: Part D - Remove nsWindowGfx::OnSettingsChangeGfx & nsWindow::OnSettingsChange

Only change: Updated to tip & r=dougt added to commit message.

Comment 22

[Ed Morley [:emorley]]

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 23

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/c4864738e3fd
http://hg.mozilla.org/mozilla-central/rev/2460d5f02a5d
http://hg.mozilla.org/mozilla-central/rev/d33506667f79
http://hg.mozilla.org/mozilla-central/rev/d29e9cb9d0c9

Comment 24

[Ed Morley [:emorley]]

Thanks Zack, much obliged :-)

Comment 25

[Ed Morley [:emorley]]

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