about:networking DNS Lookup tool - frontend part

RESOLVED FIXED in Firefox 46

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Catalin Iordache, Assigned: valentin)

Tracking

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 792656 [details] [diff] [review]
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
(Reporter)

Updated

4 years ago
Attachment #792656 - Flags: review?(mcmanus)
(Reporter)

Comment 1

4 years ago
Created attachment 792883 [details] [diff] [review]
DNSLookupFrontend.patch

This patch is the frontend of DNS Lookup Tool (a patch which has been already included in Mozilla code base)
Attachment #792883 - Flags: feedback?(ttaubert)
(Reporter)

Updated

4 years ago
Attachment #792883 - Flags: feedback?(valentin.gosu)
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.
Attachment #792656 - Flags: review?(mcmanus)
(Reporter)

Comment 3

4 years ago
Created attachment 793867 [details] [diff] [review]
DNSLookupPatchOverBackendPart.patch

I have no ideea how that patch got here. My mistake. This are the modifications which I wanted you to look at
Attachment #792656 - Attachment is obsolete: true
Attachment #793867 - Flags: review?(mcmanus)
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.
Attachment #792883 - Flags: feedback?(ttaubert)
Assignee: nobody → catalinn.iordache
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
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)
Attachment #793867 - Flags: review?(mcmanus) → review+
(Reporter)

Updated

4 years ago
Depends on: 905325
(Reporter)

Comment 6

4 years ago
Created attachment 793988 [details] [diff] [review]
DNSLookupToolBACKEND.patch

Made changes that were specified by Patrick
Attachment #793867 - Attachment is obsolete: true
(Reporter)

Comment 7

4 years ago
Created attachment 793997 [details] [diff] [review]
DNSLookupFRONTEND.patch
Attachment #792883 - Attachment is obsolete: true
Attachment #792883 - Flags: feedback?(valentin.gosu)
Attachment #793997 - Flags: feedback?(ttaubert)
(Reporter)

Comment 8

4 years ago
Created attachment 798627 [details] [diff] [review]
DNSLookupFrontendV2.patch
Attachment #793997 - Attachment is obsolete: true
Attachment #793997 - Flags: feedback?(ttaubert)
Attachment #798627 - Flags: review?(ttaubert)
(Reporter)

Updated

4 years ago
Attachment #793988 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #798627 - Flags: review?(ttaubert)
Attachment #798627 - Flags: feedback?(ttaubert)
Attachment #798627 - Flags: feedback?(gavin.sharp)
(Reporter)

Updated

4 years ago
Attachment #798627 - Flags: review?(ttaubert)
Attachment #798627 - Flags: feedback?(ttaubert)
Attachment #798627 - Flags: feedback?(gavin.sharp)
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.
Attachment #798627 - Flags: review?(ttaubert)
(Assignee)

Updated

4 years ago
Assignee: catalinn.iordache → valentin.gosu
Blocks: 802398
(Assignee)

Updated

4 years ago
No longer blocks: 802398
Depends on: 802398
(Assignee)

Comment 10

4 years ago
Created attachment 8386134 [details] [diff] [review]
about:networking DNS Lookup tool - frontend part
(Assignee)

Updated

4 years ago
Attachment #8386134 - Flags: review?(ttaubert)
(Assignee)

Updated

4 years ago
Attachment #798627 - Attachment is obsolete: true
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) {
Attachment #8386134 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 12

2 years ago
Created attachment 8711389 [details] [diff] [review]
about:networking DNS Lookup tool - frontend part

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

Updated

2 years ago
Attachment #8386134 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f93c69071a9d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.