Last Comment Bug 778680 - Make nsISocketTransport constants compatible with nsresult
: Make nsISocketTransport constants compatible with nsresult
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: nsresult-enum
  Show dependency treegraph
 
Reported: 2012-07-30 03:17 PDT by Aryeh Gregor (:ayg) (next working March 28-April 26)
Modified: 2012-08-02 19:10 PDT (History)
3 users (show)
ayg: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1 - Make netwerk status codes actually nsresult (23.05 KB, patch)
2012-07-31 07:55 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
jduell.mcbugs: review+
Details | Diff | Splinter Review
Patch part 2 - Fix incorrect nsresult usage in netwerk/ (3.70 KB, patch)
2012-07-31 07:56 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
jduell.mcbugs: review+
Details | Diff | Splinter Review
email and chat with biesi (3.88 KB, text/plain)
2012-08-01 11:45 PDT, Jason Duell [:jduell] (needinfo me)
no flags Details

Description Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-30 03:17:59 PDT
(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.
Comment 1 Jason Duell [:jduell] (needinfo me) 2012-07-30 22:50:29 PDT
> 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?
Comment 2 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-31 07:55:05 PDT
Created attachment 647528 [details] [diff] [review]
Patch part 1 - Make netwerk status codes actually nsresult

This is a bit of an abuse of nsresult, but better than using PRUint, once nsresult is a class or enum.
Comment 3 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-07-31 07:56:59 PDT
Created attachment 647530 [details] [diff] [review]
Patch part 2 - Fix incorrect nsresult usage in netwerk/

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.
Comment 4 Jason Duell [:jduell] (needinfo me) 2012-08-01 11:45:14 PDT
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.
Comment 5 Jason Duell [:jduell] (needinfo me) 2012-08-01 11:46:02 PDT
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.
Comment 6 Jason Duell [:jduell] (needinfo me) 2012-08-01 16:12:03 PDT
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!
Comment 7 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-08-02 02:15:27 PDT
(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.
Comment 9 Jason Duell [:jduell] (needinfo me) 2012-08-02 11:05:59 PDT
> 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.

Note You need to log in before you can comment on or make changes to this bug.