209877 - Remove DEBUG_law from nsNativeAppSupportWin.cpp

Status: RESOLVED WONTFIX

(SeaMonkey :: UI Design, defect)

Description

[David G King]

DEBUG_law sets MOZ_DEBUG_DDE which causes some compiler warnings.

One section of code was added 3 years ago due to some tinderbox issues. Bill
isn't sure why this was added, or if it's still needed. I'm going to run some
tests, and maybe even remove all the MOZ_DEBUG_DDE stuff from this file (and
maybe even some other OS specific equivalent files).

Taking.

BILL: I added you to the cc list in case you wanted to follow this bug.

Comment 1

[David G King]

Attachment: Patch v0.1

First version of patch for testing purposes. All going well, I'll tidy it up
and maybe have a look at some other debug statements in the code.

Comment 2

[David G King]

Attachment: patch v0.2

Well, that first patch was really really bad (I was commenting out the complete
opposite of what I should have been doing.)

Anyway, this fixes that, and comments out the rest of the debug code.

I've found some odd behaviour when Mozilla is called by another program (in
this case, one I've just upgraded), so I need to ensure this patch isn't the
cause of it.

Comment 3

[David G King]

Attachment: First "real" patch for Win and OS2

This should do the trick.

Comment 4

[David G King]

Comment on attachment 127402 [details] [diff] [review]
First "real" patch for Win and OS2

Bill, any comments?

Comment 5

[David G King]

Comment on attachment 127402 [details] [diff] [review]
First "real" patch for Win and OS2

Jag, can you give an SR on this patch?

Comment 6

[jag (Peter Annema)]

Comment on attachment 127402 [details] [diff] [review]
First "real" patch for Win and OS2

This debug code seems worth keeping around. Wouldn't it be better to fix the
warnings? And maybe change the #ifdef DEBUG_law to //#define MOZ_DEBUG_DDE?

If not, it seems like you can get rid of a few more things with the debug_dde
code gone. E.g. |BOOL rc = CloseHandle( mHandle );| the |rc| was only used in
the debug_dde code. Also, the |uTypeDesc| and hszValue seem rather useless
after this patch is applied... Could we just get rid of them completely?

Also, you have nsNativeAppSupportWin.cpp in there twice... I guess one of them
should've been the OS2 version?

Comment 7

[David G King]

I'd rather remove the debug code as it hasn't been used by its author for a long
time (I confirmed that with Bill), and any future bug will probably need
different info from what this debug code provides.

I see what you mean about other code that is rendered void by the removal of the
debug code. I'll work on that as time allows.

Many thanks for the comments and pointers.

Comment 8

[David G King]

FYI, I haven't forgotten about this bug, I'm having trouble with my build not
working so I can't test my latest patch.

Comment 9

[David G King]

Attachment: Patch v1.1

Modified patch as per comments.
Also, removed a dup include of XPCOM.H from nsNativeAppSupportWin.cpp

Comment 10

[David G King]

Comment on attachment 130255 [details] [diff] [review]
Patch v1.1

Requesting r or sr

Comment 11

[jag (Peter Annema)]

Comment on attachment 130255 [details] [diff] [review]
Patch v1.1

xpfe/bootstrap/nsNativeAppSupportOS2.cpp:
In the Mutex constructor, remove the APIRET rc = part from 
	 APIRET rc = DosCreateMutexSem( mName.get(), &mHandle, 0, FALSE );

In the OS/2 code you won't (shouldn't) need the uTypeDesc and hszValue
functions either.

sr=jag with these minor things fixed.

Comment 12

[David G King]

I didn't want to mess with the OS/2 code too much as I don't have the ability to
test it. But, I'll make the changes you suggest as, if nothing else, if Windows
doesn't need them, then why should OS/2. New patch coming real soon.

Comment 13

[David G King]

Attachment: Patch v1.2

Patch with minor changes as per jag.

Comment 14

[David G King]

Comment on attachment 130259 [details] [diff] [review]
Patch v1.2

As per Comment #11, changing so that sr=jag.

Bill, can you do an r for me, or should someone else do it?

Comment 15

[David G King]

Comment on attachment 130259 [details] [diff] [review]
Patch v1.2

I've been advised that B Law isn't around anymore. So, trying Alec for an r.

Comment 16

[Alec Flett]

Comment on attachment 130259 [details] [diff] [review]
Patch v1.2

hmm... I have mixed feelings (like jag) about removing this stuff since DDE can
be kinda tricky - it seems useful. I guess we have CVS history though.

sr=alecf

please make sure to put some reference to "removing dde debugging code" in the
cvs log, to make it easier for someone who DOES want to debug the DDE stuff.

Comment 17

[David G King]

Thanks Alec and Jag. I share both your reservations, but I don't like having
Debug code being in a program "just in case". And, from experience, when
debugging what I want to see isn't covered by existing debug code (Murphy's Law
I suppose). However, there is a change log courtesy of CVS so it can be retreived.

I don't have CVS checkin privs yet, and anyway, I suspect 1.5beta is very close
so I don't want to get in its way, so changing milestone to 1.5final (which
doesn't exist so using 1.5beta and asking for 1.5b blocker to get drivers
involved int he final decision).

Note: this is zero impact, even with the removal of the dup include.

Comment 18

[timeless]

if you want approval then ask for it.
there's absolutely no way you can justify claiming that this bug *blocks* the
*release* of mozilla1.5b.
that said *any* patch has some degree of risk. please wait for 1.6 like everyone
else.

Comment 19

[David G King]

Apologies, I got carried away when I set the Blocking flag without asking for an
"a".
Changing milestone as suggested.

Comment 20

[David G King]

As per patch from bug #219681 my patch is now invalid. I will be revisiting
Comment #6 as there does appear to be interest in this debug code.

Comment 21

[David G King]

After recent experience with this piece of code, I've decided that the DEBUG
code should remain. I found the DEBUG code to be rather useful for trying to fix
a seperate bug.

Marking Wontfix.