Closed Bug 778680 Opened 8 years ago Closed 8 years ago
ISocket Transport constants compatible with nsresult
23.05 KB, patch
|Details | Diff | Splinter Review|
3.70 KB, patch
|Details | Diff | Splinter Review|
3.88 KB, text/plain
(In reply to :Aryeh Gregor from comment #33) > CCing bz as a networking peer, and Christian Biesinger because he's listed > as owner and I don't know any of the other peers. nsISocketTransport.idl > currently defines > > const unsigned long STATUS_RESOLVING = 0x804b0003; > const unsigned long STATUS_RESOLVED = 0x804b000b; > const unsigned long STATUS_CONNECTING_TO = 0x804b0007; > const unsigned long STATUS_CONNECTED_TO = 0x804b0004; > const unsigned long STATUS_SENDING_TO = 0x804b0005; > const unsigned long STATUS_WAITING_FOR = 0x804b000a; > const unsigned long STATUS_RECEIVING_FROM = 0x804b0006; > > and then various things use them instead of nsresult. This breaks when the > final patch in this series is applied (making nsresult an enum). Making > everything that uses these PRUint32 instead more or less works, but it's > both less informative and less type-safe. Plus sometimes it leaks out into > the wider codebase unless I cast a lot of things. > > So what would you suggest? Is this API used by JS anywhere, or only C++? > Could I move the definitions of these class constants to C++, and define > them as being equal to nsresults defined centrally in the enum? Or should I > leave them as-is, but make the C++ equivalents (NS_NET_STATUS_*) into > nsresults and port all C++ code to use them instead? (In reply to Boris Zbarsky (:bz) from comment #54) > > Is this API used by JS anywhere, > > Yes. The HUD service uses it, and lots of use in extensions (e.g. any > extension that needs raw socket access, like ChatZilla). > > Can you just file a separate bug on this? Make sure to cc the active > network maintainers (so jduell, mayhemer, mcmanus). Having a discussion in > this bug is pretty impossible.
> should I leave them as-is, but make the C++ equivalents (NS_NET_STATUS_*) into > nsresults and port all C++ code to use them instead? Sounds like the best plan?
This is a bit of an abuse of nsresult, but better than using PRUint, once nsresult is a class or enum.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #647528 - Flags: review?(jduell.mcbugs)
Also, while I'm here. The only thing here that looks a little dodgy is the OnMsgCancelTransaction changes. It has to take a PRInt32 to work with PostEvent, but then it treats it as an nsresult, so I need to cast my way out of it, which is unfortunate. Tell me if you want me to do this some other way.
Attachment #647530 - Flags: review?(jduell.mcbugs)
Aryeh: here's some communications I've had with biesi on this. Based on that I'm going to +r patch 1. Have a look over it to make sure it fits with your understanding too.
Comment on attachment 647528 [details] [diff] [review] Patch part 1 - Make netwerk status codes actually nsresult Review of attachment 647528 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsISocketTransport.idl @@ +179,1 @@ > %} Seems like we should put a note in the Transport.idl's that C++ must use these #defines instead of the Transport::STATUS_FOO. I suppose people can find out when they get compiler errors, but it's nice to document things.
Attachment #647528 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 647530 [details] [diff] [review] Patch part 2 - Fix incorrect nsresult usage in netwerk/ Review of attachment 647530 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +1900,5 @@ > nsHttpTransaction *temp = trans; > NS_RELEASE(temp); // b/c NS_RELEASE nulls its argument! > } > } > + trans->Close(static_cast<nsresult>(reason)); I have a slight preference to do the cast once, in a nsresult closeCode = static_cast<nsresult>(reason) at the top of the function, then use closeCode. Otherwise looks fine--thanks!
Attachment #647530 - Flags: review?(jduell.mcbugs) → review+
(In reply to Jason Duell (:jduell) from comment #4) > Created attachment 648026 [details] > email and chat with biesi > > Aryeh: here's some communications I've had with biesi on this. Based on > that I'm going to +r patch 1. Have a look over it to make sure it fits with > your understanding too. Just to be clear -- in my final patch, the NS_NET_STATUS_* constants will be moved to the actual enum, along with all other nsresults. They use netwerk error codes 3-11. This overlaps with NS_ERROR_ALREADY_CONNECTED (= 11) which has a comment saying it's unused and could be removed, and dxr confirms it is indeed unused. Also with NS_ERROR_MALFORMED_URI (= 10), and NS_BINDING_REDIRECTED (= 3), and NS_BINDING_RETARGETED (= 4), all of which appear to be used. But multiple names for the same value in the same enum are fine, they'll just be confusing if you do thinks like "if (rv == NS_ERROR_MALFORMED_URI)", so maybe renumbering some of them would make sense.
Target Milestone: --- → mozilla17
> maybe renumbering some of them would make sense. I'd be good with removing NS_ERROR_ALREADY_CONNECTED. The other symbols all are likely to be used by enough JS that I'd be wary of changing any of their values.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.