Closed
Bug 890597
Opened 11 years ago
Closed 11 years ago
Ping Diagnostic Tool
Categories
(Core :: Networking, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: robertbindar, Assigned: robertbindar)
Details
Attachments
(1 file, 6 obsolete files)
11.54 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release) Build ID: 20130620122336
Assignee | ||
Comment 1•11 years ago
|
||
I need some feedback on this start patch. I want to know if it's ok how I've approached this. I also need to know if the timeouts about we talked on irc reduces to closing the socket and the stream when the time for the connection to reach is up.
Attachment #771758 -
Flags: feedback?(mcmanus)
Comment 2•11 years ago
|
||
(In reply to Robert Bindar from comment #1) > Created attachment 771758 [details] [diff] [review] > testing behavior for a new openned connection > > I need some feedback on this start patch. I want to know if it's ok how I've > approached this. sure.. I wouldn't let arbitrary protocol strings through.. e.g. udp doesn't make any sense because it doesn't generate CONNECTED events.. and starttls is going to require an application layer protocol to indicate when starttls has taken place. so I'd limit it to ssl and tcp > I also need to know if the timeouts about we talked on irc reduces to > closing the > socket and the stream when the time for the connection to reach is up. yep.. close them with NS_ERROR_ABORT and then free the objects
Updated•11 years ago
|
Attachment #771758 -
Flags: feedback?(mcmanus)
Assignee | ||
Comment 3•11 years ago
|
||
Thanks a lot, this was very helpful.
Updated•11 years ago
|
Severity: normal → enhancement
Assignee | ||
Comment 4•11 years ago
|
||
I'm currently waiting for an answer from bsmedberg about mapping the error codes to corresponding strings. I need to know if it looks ok by now and what I should improve before asking for a review.
Attachment #771758 -
Attachment is obsolete: true
Attachment #774548 -
Flags: feedback?(mcmanus)
Updated•11 years ago
|
Attachment #774548 -
Flags: feedback?(mcmanus) → feedback?(valentin.gosu)
Comment on attachment 774548 [details] [diff] [review] Ping diagnostic tool v1 Review of attachment 774548 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIDashboard.idl @@ +39,5 @@ > + * ex: nullptr for TCP, "starttls", "ssl", "udp". > + * aHost: the host's name > + * aPort: the port which the connection will open on > + * aTimeout: the timespan before the connection will be timed out */ > + void requestNewConnectionStatus(in ACString aHost, in unsigned long aPort, Maybe requestConnection would be better? ::: netwerk/base/src/Dashboard.cpp @@ +562,5 @@ > + uint64_t aProgress, uint64_t aProgressMax) > +{ > + printf("aStatus: %d\n", NS_ERROR_GET_CODE(aStatus)); > + printf("aProgress: %ld\n", aProgress); > + printf("aProgressMax: %ld\n", aProgressMax); We shouldn't leave printf's in. @@ +563,5 @@ > +{ > + printf("aStatus: %d\n", NS_ERROR_GET_CODE(aStatus)); > + printf("aProgress: %ld\n", aProgress); > + printf("aProgressMax: %ld\n", aProgressMax); > + if (aStatus == NS_NET_STATUS_CONNECTED_TO) { I'd say we want other events other than CONNECTED_TO. Hopefully we can do nsresult -> nsString without much difficulty. @@ +576,5 @@ > + > +NS_IMETHODIMP > +Dashboard::Notify(nsITimer *timer) > +{ > + printf("FIRE: the timer was triggered\n"); Remove the printf.
Attachment #774548 -
Flags: feedback?(valentin.gosu) → feedback+
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #774548 -
Attachment is obsolete: true
Attachment #779835 -
Flags: review?(valentin.gosu)
Comment on attachment 779835 [details] [diff] [review] ping diagnostic tool Review of attachment 779835 [details] [diff] [review]: ----------------------------------------------------------------- Getting closer now. I would also have a smaller status message for the patch. The bug number and title will do. ::: netwerk/base/src/Dashboard.cpp @@ +596,5 @@ > + > +const char * > +Dashboard::GetErrorString(nsresult rv) > +{ > + ErrorEntry errors[] = { I would make errors a static variable in Dashboard.cpp I don't know if allocating it here is very optimal. ::: netwerk/base/src/Dashboard.h @@ +52,5 @@ > nsresult GetSockets(); > nsresult GetHttpConnections(); > nsresult GetWebSocketConnections(); > nsresult GetDNSCacheEntries(); > + nsresult GetConnectionStatus(nsString aStatus); Maybe we in the future we could pass more info than a string. Make a structure and pass that to the method. @@ +141,5 @@ > struct HttpData mHttp; > struct WebSocketData mWs; > struct DnsData mDns; > + struct ConnectionData mConn; > + ErrorEntry mSocketTransportStatuses[7] = { Also move this to Dashboard.cpp - we only use it in that one function. I also don't think ErrorEntry is needed here.
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #779835 -
Attachment is obsolete: true
Attachment #779835 -
Flags: review?(valentin.gosu)
Attachment #781166 -
Flags: review?(valentin.gosu)
Comment on attachment 781166 [details] [diff] [review] ping diagnostic tool Review of attachment 781166 [details] [diff] [review]: ----------------------------------------------------------------- r+ with a small change. ::: netwerk/base/src/Dashboard.h @@ +120,5 @@ > }; > > + struct ConnectionData > + { > + ConnStatus status; You don't need this. Just use a local ConnStatus variable.
Attachment #781166 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 10•11 years ago
|
||
r=valentin.gosu
Attachment #781166 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•11 years ago
|
||
rebasing done r=valentin.gosu
Attachment #782260 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
Backed out for Windows Werror bustage. https://hg.mozilla.org/integration/fx-team/rev/051d88f15bc7 https://tbpl.mozilla.org/php/getParsedLog.php?id=25911194&tree=Fx-Team
Whiteboard: [fixed-in-fx-team]
Comment on attachment 782984 [details] [diff] [review] ping diagnostic tool Review of attachment 782984 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/src/Dashboard.cpp @@ +621,5 @@ > + const char *error; > +} ErrorEntry; > + > +ErrorEntry errors[] = { > + #define ERROR(key, val) {key, #key} Place a #undef ERROR before defining this macro. You may use this for defining both errors and socketTransportStatuses. Do a try-server run before requesting checkin-needed.
Assignee | ||
Comment 16•11 years ago
|
||
r=valentin.gosu https://tbpl.mozilla.org/?tree=Try&rev=61812e74a7fc
Attachment #782984 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a3d8476211b3
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a3d8476211b3
Status: UNCONFIRMED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•