56767 - XP_PC a misnomer. Should be XP_WIN32

Status: RESOLVED FIXED

Description

[cls]

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

[Mike Kaply [:mkaply]]

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 :)

Comment 2

[cls]

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

Comment 3

[cls]

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

Comment 4

[Mike Kaply [:mkaply]]

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.

Comment 5

[cls]

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

[Mike Kaply [:mkaply]]

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

[Mike Kaply [:mkaply]]

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

Comment 8

[Mike Kaply [:mkaply]]

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

Comment 9

[Mike Kaply [:mkaply]]

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

Comment 10

[Mike Kaply [:mkaply]]

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

Comment 11

[Mike Kaply [:mkaply]]

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

Comment 12

[Mike Kaply [:mkaply]]

Created attachment 17622 [details] [diff] [review]
Fixes for libpref/src - a couple tab removals

Comment 13

[cls]

The patches look good to me. r=cls

Comment 14

[Mike Kaply [:mkaply]]

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

Comment 15

[Jon Granrose]

yes, he should be back tomorrow.

Comment 16

[Mike Kaply [:mkaply]]

Created attachment 18303 [details] [diff] [review]
Fix missing paren in modules\pref\src diff

Comment 17

[Daniel (Leaf) Nunes]

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

[Mike Kaply [:mkaply]]

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.

Comment 19

[Daniel (Leaf) Nunes]

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

Comment 20

[Mike Kaply [:mkaply]]

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

[Mike Kaply [:mkaply]]

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

Comment 22

[Mike Kaply [:mkaply]]

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

Comment 23

[Mike Kaply [:mkaply]]

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

Comment 24

[Mike Kaply [:mkaply]]

Created attachment 19908 [details] [diff] [review]
morkConfig.h

Comment 25

[Scott Collins]

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

Comment 26

[Mike Kaply [:mkaply]]

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.

Comment 27

[Jon Granrose]

bug cleanup - all leaf's bugzilla bugs should be assigned to [e-mail]
(not [e-mail]), now and any future bugs created.

this should be a one time change, apologies for the spam.

Comment 28

[hacker formerly known as [e-mail]]

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)

Comment 29

[Alec Flett]

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

good riddens!
sr=alecf

Comment 30

[Mike Kaply [:mkaply]]

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

agreed. lets get rid of these

Comment 31

[hacker formerly known as [e-mail]]

All gone. (All that count anyway.)