128905 - nsIPromptService.idl confirmEx is poorly prototyped

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[timeless]

77   boolean confirm(in nsIDOMWindow parent,
 78                   in wstring dialogTitle,
 79                   in wstring text);

135   void confirmEx(in nsIDOMWindow parent,
136                  in wstring dialogTitle,
137                  in wstring text,
138                  in unsigned long buttonFlags,
139                  in wstring button0Title,
140                  in wstring button1Title,
141                  in wstring button2Title,
142                  in wstring checkMsg,
143                  inout boolean checkValue,
144                  out PRInt32 buttonPressed);

confirmEx should not be void, it should be PRInt32, and buttonPressed should just be the return value. In converting the idl to js I managed to flub this which of course resulted in my code not working when i tried to call the xpcom impl using 9 args and expecting buttonPressed as a retval.

Comment 1

[Conrad Carlen (not reading bugmail)]

Yeah, that is pretty awkward.

Comment 2

[timeless]

Attachment: draft

This should be a clean patch of just the relevant bits.  unfortunately the
biggest code holder is mailnews.

i think (as usual) some of the mailnews code was just incorrect.  and i've
sprinkled a few /**/'s in places where i can imagine the jseng will eventually
warn about not always returning a value.  I won't check those lines in, but I
would like opinions about what to do.

Note that the C++ consumers and impl's don't notice this change at all...

Comment 3

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 4

[timeless]

this bug was only assigned to conrad as a courtesy.

Comment 5

[Conrad Carlen (not reading bugmail)]

I like it. The only problem I found is this one:

===================================================================
RCS file: /cvsroot/mozilla/mailnews/base/resources/content/mail-offline.js,v
retrieving revision 1.6
diff -u -r1.6 mail-offline.js
--- mailnews/base/resources/content/mail-offline.js	2002/02/19 22:39:33	1.6
+++ mailnews/base/resources/content/mail-offline.js	2002/03/04 22:40:09
@@ -97,9 +97,8 @@
   InitServices();
 
   if (gPromptService) {
-    var buttonPressed = {value:0};
     var checkValue = {value:true};
-    gPromptService.confirmEx(window, 
+    buttonPressed = gPromptService.confirmEx(window, 

You dropped the decl of "buttonPressed"

As far as the warnings about not returning values, fix the ones that are obvious
(safe) to fix, leave the others commented, but file bugs. In my experience,
those helpful comments get ignored and stay there forever unless bugs are filed.

Also, since with js there is no compiler to check prototypes, make sure you've
actually tested each of these confirms.

with that, r=ccarlen

Comment 6

[timeless]

Attachment: address comments + resolve conflicts

i picked false
i changed (x*a+x*b+x*c) to x*(a+b+c)
i removed function calling convention comments because they were confusing me.
Anyone who wants to know the function parameters should be able to read the
idl.
very little changed, but I can provide a -w if someone wants it...

Comment 7

[Scott Collins]

Comment on attachment 77677 [details] [diff] [review]
address comments + resolve conflicts

sr=scc

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 77677 [details] [diff] [review]
address comments + resolve conflicts

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 9

[timeless]

Attachment: browser.xml, nsIPromptTest.txt

ok something messed up, I know that i made the changes to the other files in
cvs, but for some reason they didn't end up in my patch or the updated patch.

for reference bug 135672 has the changes to ComposerCommands.js editor.js and
msgCompSMIMEOverlay.js

Comment 10

[Kathleen :Brade]

Comment on attachment 77892 [details] [diff] [review]
browser.xml, nsIPromptTest.txt

r=brade

Comment 11

[kinmoz]

Comment on attachment 77892 [details] [diff] [review]
browser.xml, nsIPromptTest.txt

sr=[e-mail]

Comment 12

[(not reading, please use [e-mail] instead)]

are these two patches up to date?

I'm sure there have been changes since 4/5/2002.

Can you provide an up to date patch?  I'd like to review the mailnews changes.  
Also, can you let me know what you tested?

Comment 13

[timeless]

this was fixed around then

Comment 14

[Michael Dunn]

Over to depstein for verification

Comment 15

[David Epstein]

Mozilla 1.2b Gecko/20021008. ConfirmEx() now returns PRInt32 in nsIPromptService
and nsIPrompt. Verified patch checkins against trunk build; looks good.