The default bug view has changed. See this FAQ.

Widget removal of unneeded WinCE stuff

RESOLVED FIXED in mozilla6

Status

()

Core
Widget: Win32
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: dougt, Assigned: emorley)

Tracking

Trunk
mozilla6
x86
Windows CE
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 357072 [details] [diff] [review]
patch v.1
Attachment #357072 - Flags: review?
(Reporter)

Updated

8 years ago
Attachment #357072 - Flags: review? → review?(emaijala)

Comment 1

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

r=me for the remaining nsScreenWin.cpp part.
Attachment #357072 - Flags: review?(emaijala) → review+
Blocks: 614720
OS: Windows XP → Windows CE
Whiteboard: [patchlove]
Version: unspecified → Trunk
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?
(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
Assignee: nobody → bmo
Status: NEW → ASSIGNED
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
Attachment #357072 - Attachment is obsolete: true
Attachment #357072 - Flags: review+
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
Whiteboard: [patchlove]
Created attachment 527164 [details] [diff] [review]
Part A - File removal

Hg rm:
nsWindowCE.(h|cpp)
nsClipboardCE.(h|cpp)
Attachment #527164 - Flags: review?(doug.turner)
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.
Attachment #527169 - Flags: review?(doug.turner)
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
Attachment #527171 - Flags: review?(doug.turner)
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
Attachment #527173 - Flags: review?(doug.turner)
Note: Parts C & D require B to have been applied first, or they won't apply cleanly.
(Reporter)

Comment 11

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

rs+
Attachment #527164 - Flags: review?(doug.turner) → review+
(Reporter)

Comment 12

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

rs+
Attachment #527169 - Flags: review?(doug.turner) → review+
(Reporter)

Updated

6 years ago
Attachment #527171 - Flags: review?(doug.turner) → review+
(Reporter)

Updated

6 years ago
Attachment #527173 - Flags: review?(doug.turner) → review+
(Reporter)

Comment 13

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

Comment 14

6 years ago
and thanks!
(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 :-)
Depends on: 648541
No longer depends on: 648541
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!
Also, any idea about comment 5?
Created attachment 528048 [details] [diff] [review]
Part A - File removal

Only change: Updated to tip & r=dougt added to commit message.
Attachment #527164 - Attachment is obsolete: true
Attachment #528048 - Flags: review+
Created attachment 528049 [details] [diff] [review]
Part B - Main ifdef removal

Only change: Updated to tip & r=dougt added to commit message.
Attachment #527169 - Attachment is obsolete: true
Attachment #528049 - Flags: review+
Created attachment 528050 [details] [diff] [review]
Part C - Remove ENABLE_IME_MOUSE_HANDLING

Only change: Updated to tip & r=dougt added to commit message.
Attachment #527171 - Attachment is obsolete: true
Attachment #528050 - Flags: review+
Created attachment 528051 [details] [diff] [review]
Part D - Remove nsWindowGfx::OnSettingsChangeGfx & nsWindow::OnSettingsChange

Only change: Updated to tip & r=dougt added to commit message.
Attachment #527173 - Attachment is obsolete: true
Attachment #528051 - Flags: review+
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!
Keywords: checkin-needed
Summary: widget removal of unneeded wince stuff → Widget removal of unneeded WinCE stuff
Whiteboard: [please push all parts; need to be in order A->D or merge conflicts - thanks!]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [please push all parts; need to be in order A->D or merge conflicts - thanks!]
Thanks Zack, much obliged :-)
Target Milestone: --- → mozilla6
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
You need to log in before you can comment on or make changes to this bug.