XP_PC a misnomer. Should be XP_WIN32

RESOLVED FIXED in mozilla1.4beta

Status

SeaMonkey
Build Config
P3
normal
RESOLVED FIXED
18 years ago
14 years ago

People

(Reporter: cls, Assigned: Daniel (Leaf) Nunes)

Tracking

Trunk
mozilla1.4beta
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments)

577 bytes, patch
Details | Diff | Splinter Review
7.42 KB, patch
Details | Diff | Splinter Review
5.65 KB, patch
Details | Diff | Splinter Review
5.78 KB, patch
Details | Diff | Splinter Review
22.51 KB, patch
Details | Diff | Splinter Review
1.34 KB, patch
Details | Diff | Splinter Review
2.81 KB, patch
Details | Diff | Splinter Review
2.84 KB, patch
Details | Diff | Splinter Review
4.51 KB, patch
Details | Diff | Splinter Review
1.00 KB, patch
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
1.09 KB, patch
Details | Diff | Splinter Review
76.02 KB, patch
mkaply
: review+
Alec Flett
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
mkaply has brought this up a number of times and timeless & I just went over it
again on irc.  For some reason (mkaply can fill in the details), XP_PC is used
on platforms other than MS Windows, yet every developers continue to use XP_PC
as a win32 exculsive define.

To start with, we need to add -DXP_WIN32 to config/config.mak so that it is
always defined instead of requiring xp_core.h.  That's a 30sec process.  Then we
need to scrub XP_PC from the source trees.  That should eat up a couple of
hours.  Once that's done, we should be able to remove XP_PC from
config/config.mak and listen to the develoeprs wonder why their new code breaks.

Comment 1

18 years ago
In a number of cases, XP_PC is used for both because it relates to 
"Intel" type things, or things that are consistent between OS/2 and 
Windows.

Another note - I believe there should be XP_WIN and XP_WIN32. XP_WIN 
for stuff that is generic windows (like include windows.h for instance) 
and XP_WIN32 for WIN32 specific stuff and probably eventually XP_WIN64.

I'll be more than happy to help with this change next week :)
(Reporter)

Comment 2

18 years ago
"Intel" sort of things?  Isn't that what -D_X86_ is for? So does OS/2 use any
code that falls under XP_WIN?
(Reporter)

Comment 3

18 years ago
Created attachment 17390 [details] [diff] [review]
Add -DXP_WIN & -DXP_WIN32 to default WIN32 OS_CFLAGS

Comment 4

18 years ago
Specific examples of XP_PC between OS/2 and Windows

DLL names:

http://lxr.mozilla.org/seamonkey/source/editor/base/nsEditor.cpp#124

File system stuff (back slashes)

http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3102

Other stuff that I don't know:

http://lxr.mozilla.org/seamonkey/source/js/src/jsfile.c#264

URl conversion:

http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsStdURL.cpp#983

Context menus:

http://lxr.mozilla.org/seamonkey/source/rdf/content/src/nsXULPopupListener.cpp#1
94

etc.

By the way, if you search on XP_PC in lxr - every line that has the !defined 
XP_OS2 can be fixed because of this change. Very nice.
(Reporter)

Comment 5

18 years ago
Ok, patch has been checked in.  As for the next step....mkaply, are you saying
that we should leave XP_PC or replace it with XP_WIN (or XP_DOS)?

Comment 6

18 years ago
I think for step one I'd like to remove a lot if the !defined XP_OS2 and elif 
XP_PC.

I'll put together a patch in the next couple of days.

Once we get rid of the "Windows" XP_PC stuff then we can decide what to really 
call XP_PC.

Note by the way that I'll be moving stuff around in these diffs. Example:

#ifdef XP_OS2
#elif defined XP_PC
#elif defined XP_UNIX
#endif

I only had to put XP_OS2 at the beginning because of the XP_PC.

Should be:

#ifdef XP_WIN
#elif defined XP_UNIX
#elif defined XP_OS2
#endif

Comment 7

18 years ago
Created attachment 17616 [details] [diff] [review]
Diffs for xpcom/base dir - note the removal of lots of XP_OS2 :)

Comment 8

18 years ago
Created attachment 17617 [details] [diff] [review]
intl.locale.src fixes - also some tab removals.

Comment 9

18 years ago
Created attachment 17618 [details] [diff] [review]
Fixes for netwerk/dns/src

Comment 10

18 years ago
Created attachment 17620 [details] [diff] [review]
Fixes for xpcom/io

Comment 11

18 years ago
Created attachment 17621 [details] [diff] [review]
xpfe/components/build - small

Comment 12

18 years ago
Created attachment 17622 [details] [diff] [review]
Fixes for libpref/src - a couple tab removals
(Reporter)

Comment 13

18 years ago
The patches look good to me. r=cls

Comment 14

18 years ago
Who would be the sr/a= on these? leaf I assume?

Comment 15

18 years ago
yes, he should be back tomorrow.

Comment 16

18 years ago
Created attachment 18303 [details] [diff] [review]
Fix missing paren in modules\pref\src diff
(Assignee)

Comment 17

18 years ago
ahem.
@@ -531,7 +531,7 @@
 #ifdef NS_EXPLICIT_FUNC_TEMPLATE_ARG
          if ((conv=use_facet<ofacet_type>(getloc()).
 #elif defined(XP_PC)
-
     if ((conv=use_facet(getloc(), (ofacet_type*)0, false).
+         if ((conv=use_facet(getloc(), (ofacet_type*)0, false).
 #else
          if ((conv=use_facet(getloc(), (ofacet_type*)0).
 #endif

am i an idiot, or are these if statements having `(' matched with `.'? (patch 17620)

Comment 18

18 years ago
Nope, just a fine example of one of the worst #ifdefs I have ever seen in my 
life:

Start reading at:

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsStdFileStream.h#531

And please make sure you are sitting down.
(Assignee)

Comment 19

18 years ago
r=leaf with the fixed paren patch...my eyes are bleeding after reading that
code. Thanks, mike.

Comment 20

18 years ago
Note from brendan giving approval:

Michael Kaply wrote:

> Hey guys,
>
> The changes for bug:
>
> http://bugzilla.mozilla.org/show_bug.cgi?id=56767
>
> impact lots of areas, so we were going to have shaver sr= the changes
> rather than going to each component owner.
>
> He hasn't really been responsive.

Shaver's been busy with other stuff lately, and isn't the best fit here
anyway.

> So question is, can cls act as a super reviewer on this stuff? Or do one of
> you want to do it?
>
> I'm getting both leaf and cls to look at all the changes I make.

I think you're done, per
http://www.mozilla.org/hacking/reviewers.html#exceptions .

/be

Comment 21

18 years ago
Created attachment 19905 [details] [diff] [review]
Diffs resume - modules/plugin/nglsrc

Comment 22

18 years ago
Created attachment 19906 [details] [diff] [review]
docshell\base\nsDefaultURIFixup.cpp

Comment 23

18 years ago
Created attachment 19907 [details] [diff] [review]
two files from editor

Comment 25

18 years ago
Does this bug have any relationship to bug #65727 (currently assigned to dprice)?

Comment 26

18 years ago
65727 is a side effect of this work.

When we added XP_WIN to the build, we didn't add it to component builds.

So when I started changing XPCOM to use XP_WIN, components were still using 
XP_PC.

65727 is asking for an #ifdef somewhere in a global headers that #errors if 
XP_PC is defined without XP_WIN or XP_OS2.

This is probably a really good idea.

Note that although this bug hasn't been touched in a while, it is still being 
worked. I've just been busy.

Updated

17 years ago
Target Milestone: --- → mozilla1.2

Comment 27

17 years ago
bug cleanup - all leaf's bugzilla bugs should be assigned to leaf@mozilla.org
(not leaf@netscape.com), now and any future bugs created.

this should be a one time change, apologies for the spam.
Assignee: leaf → leaf
Created attachment 119034 [details] [diff] [review]
v1.0

* Fixes the remaining XP_PC ifdefs for the entire tree except NSPR, NSS & JS
(see bug 74999 for the last one)
Attachment #119034 - Flags: superreview?(alecf)
Attachment #119034 - Flags: review?(mkaply)

Comment 29

16 years ago
Comment on attachment 119034 [details] [diff] [review]
v1.0

good riddens!
sr=alecf
Attachment #119034 - Flags: superreview?(alecf) → superreview+

Comment 30

16 years ago
Comment on attachment 119034 [details] [diff] [review]
v1.0

agreed. lets get rid of these
Attachment #119034 - Flags: review?(mkaply) → review+
All gone. (All that count anyway.)
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.2alpha → mozilla1.4beta
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.