Closed Bug 67797 Opened 24 years ago Closed 23 years ago

need Components.isSuccessCode

Categories

(Core :: XPConnect, defect, P4)

Product:
Core
Component:
XPConnect
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: shaver, Assigned: shaver)

Details

Attachments

(2 files)

Add Components.results.succeeded
1.12 KB, patch
Details | Diff | Splinter Review
Components.isSuccessCode, welcome!
1.22 KB, patch
Details | Diff | Splinter Review 
jag wanted this, and explained that:

<jag> in this case, the rv is passed in as an in param
<jag> in long aStatus

So the usual exception-is-error-else-success technique doesn't work.  It's good
symmetry, though I don't feel especially compelled to add .failed as well.
r=jag. Duh, of course it can be this simple.
I don't buy this in its current form. So you need to detect success codes, 
fine. But why does the ability need to be built into xpconnect? nsresults are 
PRUint32 values. If the value is >= 0x80000000 then it is an error code. You can 
code that in JS as you please. Where is the more general use?

If it does need to be hanging off Components somewhere then I disagree with 
adding it to Components.results - that is just a list of result codes. Also, 
just calling it succeeded is whacky. In C++ we have the macro that is used to 
check the success of a call so the name 'SUCEEDED' is reasonable. But the JS 
programmer does not see it that way and the name 'succeeded' does not make sense 
in the JS caller context. Perhaps Components.isSuccessCode(code) would be OK. 
But I remain unconvinced that this needs to be built in. Convince me.
I think that something to test the success meaning of a result code should be
near the result codes it's meant to test.  Why does Components.results exist? 
It's a programmer convenience, to make it easier to do the right thing --
there's certainly nothing XPConnect about it, and people could just use the int
constants.

If I saw

   if (result < 0x80000000)

in JS code I was reviewing, I'd insist that they comment on it:

   if (result < 0x80000000) // success code

but then a good hacker would probably write a little routine at the top of their
JS files:

   function succeeded(res) {
       return res < 0x80000000;
   }

because the code's self-documenting that way (and C++ people will recognize it
right away).

So why not make this easier for them?  I'm down with a better name, of course
(isSuccessCode is fine).

(Updating milestone: if we don't agree by 0.8.1, I'm just going to walk away
from this.)
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: --- → mozilla0.8.1
shaver, If you still want to do this, I'm OK with Components.isSuccessCode.
I think Components.result should remain just a list.
0.8.1 is here and ready to leave the station (er, branch).
doesn't sound like something to do at this last minute clean up time..
Components.isSuccessCode it is, and for 0.9 to let chofmann sleep better at night.

I'll attach a new patch shortly.
Target Milestone: mozilla0.8.1 → mozilla0.9
r me! sr me! make me beg for mercy!
Keywords: patch, review
Summary: need Components.results.succeeded? → need Components.isSuccessCode
r=jag
sr=jband.

Did you test it much?

I have change in jband_xpconnect_flattening_BRANCH to deal with a bug I was
seeing in handling large PRUint32 values (large in the sense that the high bit
is set). The current code will turn large unsigned values into negative signed
values when going from C++ to JS (though I'm at a loss right this second on why
the 31 bit jsval ints would get tangled up here). The code below tries to fix
this. I have not run enough test cases through either scheme to know if the
mangling was reversible enough to not matter. Or if the fix causes problems. I
need to dig deeper. I just thought I'd let you know in case it makes a
difference here.

 #define FIT_32(cx,i,d) (INT_FITS_IN_JSVAL(i)?INT_TO_JSVAL(i):JAM_DOUBLE(cx,i,d))
 
+// XXX will this break backwards compatability???
+#define FIT_U32(cx,i,d) ((!(i & ~0x80000000) && INT_FITS_IN_JSVAL(i))? \
+                        INT_TO_JSVAL(i):JAM_DOUBLE(cx,i,d))
+

...

- case nsXPTType::T_U32   : *d = FIT_32(cx,*((uint32*)s),dbl);         break; 
+ case nsXPTType::T_U32   : *d = FIT_U32(cx,*((uint32*)s),dbl);        break;




This is all the testing I did:

js> for (i in Components.results) {
print (i + ": " + (Components.isSuccessCode(Components.results[i]) ? "SUCCESS" :
"FAILURE"));
}
NS_ERROR_XPC_NOT_ENOUGH_ARGS: FAILURE
NS_ERROR_XPC_NEED_OUT_OBJECT: FAILURE
NS_ERROR_XPC_CANT_SET_OUT_VAL: FAILURE
NS_ERROR_XPC_NATIVE_RETURNED_FAILURE: FAILURE
NS_ERROR_XPC_CANT_GET_INTERFACE_INFO: FAILURE
NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO: FAILURE
NS_ERROR_XPC_CANT_GET_METHOD_INFO: FAILURE
NS_ERROR_XPC_UNEXPECTED: FAILURE
NS_ERROR_XPC_BAD_CONVERT_JS: FAILURE
NS_ERROR_XPC_BAD_CONVERT_NATIVE: FAILURE
NS_ERROR_XPC_BAD_CONVERT_JS_NULL_REF: FAILURE
NS_ERROR_XPC_BAD_OP_ON_WN_PROTO: FAILURE
NS_ERROR_XPC_CANT_CONVERT_WN_TO_FUN: FAILURE
NS_ERROR_XPC_CANT_DEFINE_PROP_ON_WN: FAILURE
NS_ERROR_XPC_CANT_WATCH_WN_STATIC: FAILURE
NS_ERROR_XPC_CANT_EXPORT_WN_STATIC: FAILURE
NS_ERROR_XPC_SCRIPTABLE_CALL_FAILED: FAILURE
NS_ERROR_XPC_SCRIPTABLE_CTOR_FAILED: FAILURE
NS_ERROR_XPC_CANT_CALL_WO_SCRIPTABLE: FAILURE
NS_ERROR_XPC_CANT_CTOR_WO_SCRIPTABLE: FAILURE
NS_ERROR_XPC_CI_RETURNED_FAILURE: FAILURE
NS_ERROR_XPC_GS_RETURNED_FAILURE: FAILURE
NS_ERROR_XPC_BAD_CID: FAILURE
NS_ERROR_XPC_BAD_IID: FAILURE
NS_ERROR_XPC_CANT_CREATE_WN: FAILURE
NS_ERROR_XPC_JS_THREW_EXCEPTION: FAILURE
NS_ERROR_XPC_JS_THREW_NATIVE_OBJECT: FAILURE
NS_ERROR_XPC_JS_THREW_JS_OBJECT: FAILURENS_ERROR_XPC_JS_THREW_NULL: FAILURE
NS_ERROR_XPC_JS_THREW_STRING: FAILURE
NS_ERROR_XPC_JS_THREW_NUMBER: FAILURE
NS_ERROR_XPC_JAVASCRIPT_ERROR: FAILURE
NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: FAILURE
NS_ERROR_XPC_CANT_CONVERT_PRIMITIVE_TO_ARRAY: FAILURE
NS_ERROR_XPC_CANT_CONVERT_OBJECT_TO_ARRAY: FAILURE
NS_ERROR_XPC_NOT_ENOUGH_ELEMENTS_IN_ARRAY: FAILURE
NS_ERROR_XPC_CANT_GET_ARRAY_INFO: FAILURE
NS_ERROR_XPC_NOT_ENOUGH_CHARS_IN_STRING: FAILURE
NS_ERROR_XPC_SECURITY_MANAGER_VETO: FAILURE
NS_ERROR_XPC_INTERFACE_NOT_SCRIPTABLE: FAILURE
NS_ERROR_XPC_INTERFACE_NOT_FROM_NSISUPPORTS: FAILURE
NS_ERROR_XPC_CANT_GET_JSOBJECT_OF_DOM_OBJECT: FAILURE
NS_ERROR_XPC_CANT_SET_READ_ONLY_CONSTANT: FAILURE
NS_ERROR_XPC_CANT_SET_READ_ONLY_ATTRIBUTE: FAILURE
NS_ERROR_XPC_CANT_SET_READ_ONLY_METHOD: FAILURE
NS_ERROR_XPC_CANT_ADD_PROP_TO_WRAPPED_NATIVE: FAILURE
NS_ERROR_XPC_CALL_TO_SCRIPTABLE_FAILED: FAILURE
NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED: FAILURE
NS_ERROR_XPC_BAD_ID_STRING: FAILURE
NS_ERROR_XPC_BAD_INITIALIZER_NAME: FAILURE
NS_OK: SUCCESS
NS_ERROR_NOT_INITIALIZED: FAILURENS_ERROR_ALREADY_INITIALIZED: FAILURE
NS_ERROR_NOT_IMPLEMENTED: FAILURE
NS_NOINTERFACE: FAILURE
NS_ERROR_NO_INTERFACE: FAILURE
NS_ERROR_INVALID_POINTER: FAILURE
NS_ERROR_NULL_POINTER: FAILURE
NS_ERROR_ABORT: FAILURE
NS_ERROR_FAILURE: FAILURE
NS_ERROR_UNEXPECTED: FAILURE
NS_ERROR_OUT_OF_MEMORY: FAILURE
NS_ERROR_ILLEGAL_VALUE: FAILURE
NS_ERROR_INVALID_ARG: FAILURE
NS_ERROR_NO_AGGREGATION: FAILURE
NS_ERROR_NOT_AVAILABLE: FAILURE
NS_ERROR_FACTORY_NOT_REGISTERED: FAILURE
NS_ERROR_FACTORY_REGISTER_AGAIN: FAILURE
NS_ERROR_FACTORY_NOT_LOADED: FAILURE
NS_ERROR_FACTORY_NO_SIGNATURE_SUPPORT: FAILURE
NS_ERROR_FACTORY_EXISTS: FAILURE
NS_ERROR_PROXY_INVALID_IN_PARAMETER: FAILURE


Fixx0red.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Marking Verified -
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: