907050 - about:networking DNS Lookup tool - frontend part

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Catalin Iordache]

Attachment: ProxySettingsDiagnosticToolBackendPart.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130803192641

Steps to reproduce:

After Patrick Mcmanus had reviewed my Proxy Settings Test Diagnostic Tool patch, I had realised that there are some things that are need to be changed on my previous patch - DNS Lookup Diagnostic Tool. Also, I'll keep this opened until DNS lookup tool will have a frontend reviewed

Comment 1

[Catalin Iordache]

Attachment: DNSLookupFrontend.patch

This patch is the frontend of DNS Lookup Tool (a patch which has been already included in Mozilla code base)

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 792656 [details] [diff] [review]
ProxySettingsDiagnosticToolBackendPart.patch

Review of attachment 792656 [details] [diff] [review]:
-----------------------------------------------------------------

isn't this the (obsoleted) patch from 898237? What is it doing in this bug about DNS? I don't understand.

Comment 3

[Catalin Iordache]

Attachment: DNSLookupPatchOverBackendPart.patch

I have no ideea how that patch got here. My mistake. This are the modifications which I wanted you to look at

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 792883 [details] [diff] [review]
DNSLookupFrontend.patch

Review of attachment 792883 [details] [diff] [review]:
-----------------------------------------------------------------

It looks like the patch here should be based off the one from bug 905325.

::: toolkit/content/aboutNetworking.xhtml
@@ +100,5 @@
>              </table>
>          </div>
> +
> +        <div id="dnslookuptool" class="tab">
> +            <button onclick="dnsLookupDiagnosticTool()">&aboutNetworking.dnsLookupButton;</button>

That listener should definitely be registered from the JS file.

::: toolkit/locales/en-US/chrome/global/aboutNetworking.dtd
@@ +30,5 @@
>  <!ENTITY aboutNetworking.bytesSent             "Bytes Sent">
>  <!ENTITY aboutNetworking.bytesReceived         "Bytes Received">
> +<!ENTITY aboutNetworking.dnsLookup			   "DNS Lookup">
> +<!ENTITY aboutNetworking.dnsLookupButton	   "Resolve">
> +<!ENTITY aboutNetworking.dnsLookupTableColumn  "IPs">

The indentation is a little off here.

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 793867 [details] [diff] [review]
DNSLookupPatchOverBackendPart.patch

Review of attachment 793867 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. r+ with a  minor change

::: netwerk/base/src/Dashboard.cpp
@@ +421,5 @@
> +    rv = mDnsup.serv->AsyncResolve(aHost, 0, this, NS_GetCurrentThread(), getter_AddRefs(mDnsup.cancel));
> +    if(NS_FAILED(rv))
> +        return rv;
> +
> +    mDnsup.cb = cb;

I think you should put this on the line prior to AsyncResolve() for the same reasons it ended up there in the proxy resolution version. That will protect you if the implementation of dns::asyncresolve() changes. (and of course set it to null in the failed case)

Comment 6

[Catalin Iordache]

Attachment: DNSLookupToolBACKEND.patch

Made changes that were specified by Patrick

Comment 7

[Catalin Iordache]

Attachment: DNSLookupFRONTEND.patch

Comment 8

[Catalin Iordache]

Attachment: DNSLookupFrontendV2.patch

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 798627 [details] [diff] [review]
DNSLookupFrontendV2.patch

Review of attachment 798627 [details] [diff] [review]:
-----------------------------------------------------------------

Including this feature as a tab like all the other data tables feels weird to me. Sure, the result is a table but other than a few IPs it usually won't have that much data to offer. The refresh button and the auto-refresh checkbox also seem like they should somehow apply to the lookup tab, which doesn't really make sense.

I think this should be integrated differently - I don't have any idea how, though. Should that be a separate feature? Maybe a separate section below the current one?

::: toolkit/locales/en-US/chrome/global/aboutNetworking.dtd
@@ +28,5 @@
>  <!ENTITY aboutNetworking.messagesSent          "Messages Sent">
>  <!ENTITY aboutNetworking.messagesReceived      "Messages Received">
>  <!ENTITY aboutNetworking.bytesSent             "Bytes Sent">
>  <!ENTITY aboutNetworking.bytesReceived         "Bytes Received">
> +<!ENTITY aboutNetworking.dnsLookup		"Lookup">

Naming the tab 'DNS Lookup' or something would probably be a little more expressive.

@@ +30,5 @@
>  <!ENTITY aboutNetworking.bytesSent             "Bytes Sent">
>  <!ENTITY aboutNetworking.bytesReceived         "Bytes Received">
> +<!ENTITY aboutNetworking.dnsLookup		"Lookup">
> +<!ENTITY aboutNetworking.dnsLookupButton	"Resolve">
> +<!ENTITY aboutNetworking.dnsLookupTableColumn  "IPs">

nit: indentation is off.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Attachment: about:networking DNS Lookup tool - frontend part

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8386134 [details] [diff] [review]
about:networking DNS Lookup tool - frontend part

Review of attachment 8386134 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/aboutNetworking.js
@@ +216,5 @@
> +  }
> +}
> +
> +function displayDNSLookup(data) {
> +  let cont = document.getElementById('dnslookuptool_content');

Nit: please use " instead of ' like we do in the rest of the file.

@@ +222,5 @@
> +  let new_cont = document.createElement('tbody');
> +  new_cont.setAttribute('id', 'dnslookuptool_content');
> +
> +  if (data.answer) {
> +    for (let i = 0; i < data.address.length; i++) {

for (let address of data.address) {

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: about:networking DNS Lookup tool - frontend part

Rebased on latest version about:networking. r+d in comment 11

Comment 13

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f93c69071a9d6202c97d38a3c561b34baf662050
Bug 907050 - about:networking DNS Lookup tool - frontend part r=ttaubert

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f93c69071a9d