Closed Bug 890597 Opened 6 years ago Closed 6 years ago

Ping Diagnostic Tool

Categories

(Core :: Networking, enhancement)

21 Branch
x86_64
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: robertbindar, Assigned: robertbindar)

Details

Attachments

(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130620122336
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)
(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
Attachment #771758 - Flags: feedback?(mcmanus)
Thanks a lot, this was very helpful.
Severity: normal → enhancement
Attached patch Ping diagnostic tool v1 (obsolete) — Splinter Review
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)
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+
Attached patch ping diagnostic tool (obsolete) — Splinter Review
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.
Attached patch ping diagnostic tool (obsolete) — Splinter Review
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+
Attached patch ping diagnostic tool (obsolete) — Splinter Review
r=valentin.gosu
Attachment #781166 - Attachment is obsolete: true
Keywords: checkin-needed
Needs a rebased patch.
Keywords: checkin-needed
Attached patch ping diagnostic tool (obsolete) — Splinter Review
rebasing done
r=valentin.gosu
Attachment #782260 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e9530cf2baed
Assignee: nobody → robertbindar
Keywords: checkin-needed
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.
r=valentin.gosu
https://tbpl.mozilla.org/?tree=Try&rev=61812e74a7fc
Attachment #782984 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a3d8476211b3
Status: UNCONFIRMED → RESOLVED
Closed: 6 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.