Widget removal of unneeded WinCE stuff

RESOLVED FIXED in mozilla6

Status

()

Core
Widget: Win32
RESOLVED FIXED
9 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

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

Updated

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

Comment 1

9 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
(Assignee)

Comment 2

7 years ago
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?
(Assignee)

Comment 3

6 years ago
(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
(Assignee)

Comment 4

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #357072 - Attachment is obsolete: true
Attachment #357072 - Flags: review+
(Assignee)

Comment 5

6 years ago
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]
(Assignee)

Comment 6

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

Hg rm:
nsWindowCE.(h|cpp)
nsClipboardCE.(h|cpp)
Attachment #527164 - Flags: review?(doug.turner)
(Assignee)

Comment 7

6 years ago
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)
(Assignee)

Comment 8

6 years ago
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)
(Assignee)

Comment 9

6 years ago
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)
(Assignee)

Comment 10

6 years ago
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!
(Assignee)

Comment 15

6 years ago
(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
(Assignee)

Updated

6 years ago
No longer depends on: 648541
(Assignee)

Comment 16

6 years ago
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!
(Assignee)

Comment 17

6 years ago
Also, any idea about comment 5?
(Assignee)

Comment 18

6 years ago
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+
(Assignee)

Comment 19

6 years ago
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+
(Assignee)

Comment 20

6 years ago
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+
(Assignee)

Comment 21

6 years ago
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+
(Assignee)

Comment 22

6 years ago
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!]
(Assignee)

Comment 24

6 years ago
Thanks Zack, much obliged :-)
Target Milestone: --- → mozilla6
(Assignee)

Comment 25

6 years ago
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.