Closed Bug 865850 Opened 12 years ago Closed 11 years ago

Integrate about:networking into Firefox

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: u408661, Assigned: valentin)

References

Details

Attachments

(1 file, 4 obsolete files)

aboutNetworking.patch
19.95 KB, patch
Details | Diff | Splinter Review
We would like to have the about:networking dashboard (the UI of which is currently in an add-on) integrated into Firefox so that you don't have to install anything extra to view this information. Right now, this isn't meant to be something the average user uses a lot (if at all) outside debugging, so we shouldn't worry about making it pretty. If we make any change to the UI at all, we should maybe have a page similar to the warning from about:config that says "This page is for debugging purposes only. Do not use for fun, keep away from children." or something like that.
Bah, wrong component.
Component: Nanojit → Networking
Attached patch patch v1.0 (obsolete) — — Splinter Review
Finally got around to this. Hope it looks good.
Attachment #763277 - Flags: feedback?(hurley)
Comment on attachment 763277 [details] [diff] [review] patch v1.0 Review of attachment 763277 [details] [diff] [review]: ----------------------------------------------------------------- This all looks pretty good to me (I assume it's just a straight import of your addon into Firefox), just a couple comments that I'd like to see cleaned up before review. Someone on Gavin's team is probably the right person to review (if not, they'll know better than I would who the right reviewer is). ::: toolkit/content/aboutNetworking.js @@ +12,5 @@ > + > +const dash = Cc['@mozilla.org/network/dashboard;1']. > +getService(Ci.nsIDashboard); > + > +alert("This is very experimental. Do not use without adult supervision"); I would prefer this not be an alert, but instead be something like you see on about:config the first time you go to it (warning text, a button saying you'll be ok, and a checkbox to say whether or not you want to show the warning page on future loads). It looks to me like toolkit/components/viewconfig/content/config.xul is where the code for that lives, as an example. You might need to transform the UI from xhtml into XUL for that to work, but I think what you want is doable in plain ol' HTML. ::: toolkit/content/aboutNetworking.xhtml @@ +22,5 @@ > + </head> > + > + <body> > + > + <div class="netwk"> Please don't abbreviate. It mks it hrd to rd. :)
Attachment #763277 - Flags: feedback?(hurley) → feedback+
Attached patch Patch 1.1 (obsolete) — — Splinter Review
Posting an updated version of the patch. I changed the CSS tabs to a JS switching of the divs because it offers more control. I don't know for sure if it's optimal, but I think it works pretty good for now. I also cleaned up the JS and HTML a bit.
Attachment #763277 - Attachment is obsolete: true
Attachment #769133 - Flags: review?(gavin.sharp)
Attachment #769133 - Flags: review?(gavin.sharp) → review?(ttaubert)
Comment on attachment 769133 [details] [diff] [review] Patch 1.1 Review of attachment 769133 [details] [diff] [review]: ----------------------------------------------------------------- This looks good in general but needs a couple of adjustments to adhere to our coding guide lines. We don't use inline style="" attributes and attach event listener using addEventListener() instead of using onclick="" etc. Using .innerHTML is discouraged, we usually make use of all the DOM APIs we have at hand. Another really important missing feature is translation. This needs a DTD file that contains all the strings used in the XHTML file. ::: toolkit/content/aboutNetworking.css @@ +10,5 @@ > +} > + > +#menu { > + position: absolute; > + top: 0; I think this could use a little more margin at the top. Nit: indentation is off. @@ +17,5 @@ > +div.warningBackground { > + background: -moz-Dialog; > + width:100%; > + height:100%; > + z-index:10; Nit: indentation is off. ::: toolkit/content/aboutNetworking.js @@ +10,5 @@ > + > +Cu.import("resource://gre/modules/Services.jsm"); > + > +const dash = Cc['@mozilla.org/network/dashboard;1'].getService(Ci.nsIDashboard); > +var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).getBranch("network."); Please wrap those long lines. @@ +13,5 @@ > +const dash = Cc['@mozilla.org/network/dashboard;1'].getService(Ci.nsIDashboard); > +var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService).getBranch("network."); > + > +function displayHttp(data) { > + var cont = document.getElementById('http_content'); There's almost never a reason to use 'var' instead of 'let'. We like to say 'let' is the new 'var' :) @@ +16,5 @@ > +function displayHttp(data) { > + var cont = document.getElementById('http_content'); > + cont.innerHTML = getRows(data); > + > + function getRows(data) { Why is this an inner function? Looking at the other display* functions it seems like this could as well be a helper function. @@ +27,5 @@ > + rows += '<td>' + data.ssl[i] + '</td>'; > + // TODO: display rtt and ttl for every socket > + rows += '<td>' + data.active[i].rtt.length + '</td>'; > + rows += '<td>' + data.idle[i].rtt.length + '</td>'; > + rows += '</tr>'; Please don't build strings and use innerHTML to update the page. Use document.createElement() to create an element and element.appendChild() to insert e.g. a TD into a TR. @@ +97,5 @@ > + > +function getData() { > + get(); > + > + function get() { The name "getData()" is a little misleading as you would expect the function to return data. "get()" is a little to generic as well as this is what actually requests network data. @@ +104,5 @@ > + dash.requestWebsocketConnections(displayWebsockets); > + dash.requestDNSInfo(displayDns); > + } > + > + setInterval(get, 3000); Do you really want to automatically update the page every 3s? What if there's some data in there that I want to copy/paste somewhere? That's quite unexpected, IMO. about:networking should just work like about:memory in that I manually need to refresh the page whenever I want the most up-to-date data. @@ +111,5 @@ > +function init() { > + dash.enableLogging = true; > + if (prefs.getBoolPref("warnOnAboutNetworking")) { > + var div = document.getElementById("warning_message"); > + div.style.display = "block"; Please use a CSS class here. @@ +118,5 @@ > +} > + > +function uninit() { > + > +} Empty function? @@ +122,5 @@ > +} > + > +function confirm () { > + var div = document.getElementById("warning_message"); > + div.style.display = "none"; Please use a CSS class here. @@ +126,5 @@ > + div.style.display = "none"; > +} > + > +function handleCheckbox(box) { > + prefs.setBoolPref("warnOnAboutNetworking", box.checked); Do we really want to update the pref when I toggle the checkbox? I think we might want to do this in confirm(), no? @@ +127,5 @@ > +} > + > +function handleCheckbox(box) { > + prefs.setBoolPref("warnOnAboutNetworking", box.checked); > + alert(box.checked); I don't think you want to keep that alert() here, do you? @@ +130,5 @@ > + prefs.setBoolPref("warnOnAboutNetworking", box.checked); > + alert(box.checked); > +} > + > +var current_tab='http'; Instead of having a global variable keeping track of the selected tab, you can use an "active" class and just do document.querySelector(".active") to retrieve the currently shown tab. @@ +141,5 @@ > + content.style.display = "none"; > + } > + > + var content = document.getElementById(tab); > + content.style.display = "block"; As noted in the XHTML file, please use classList.add("active"). ::: toolkit/content/aboutNetworking.xhtml @@ +7,5 @@ > + <!DOCTYPE html [ > + <!ENTITY % htmlDTD PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "DTD/xhtml1-strict.dtd"> %htmlDTD; > + <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd"> %globalDTD; > + <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd"> %brandDTD; > + ]> Nit: indentation is off. @@ +16,5 @@ > + <link rel="stylesheet" href="chrome://global/skin/about.css" type="text/css" /> > + <link rel="stylesheet" href="chrome://global/content/aboutNetworking.css" type="text/css" /> > + <script type="application/javascript;version=1.7" src="chrome://global/content/aboutNetworking.js" /> > + </head> > + <body id="body" onload="init()" onunload="uninit();"> Please use addEventLister("load") and addEventListener("unload"). @@ +17,5 @@ > + <link rel="stylesheet" href="chrome://global/content/aboutNetworking.css" type="text/css" /> > + <script type="application/javascript;version=1.7" src="chrome://global/content/aboutNetworking.js" /> > + </head> > + <body id="body" onload="init()" onunload="uninit();"> > + <div id="warning_message" class="warningBackground" style="display: none"> Please don't use inline styles. You can put this into the css file. @@ +21,5 @@ > + <div id="warning_message" class="warningBackground" style="display: none"> > + <div class="warningMessage"> > + This is very experimental. Do not use without adult supervision. <br/> <br/> > + <label><input type='checkbox' checked="yes" onclick='handleCheckbox(this);' />Show this warning next time</label> <br/> <br/> > + <button onclick="confirm()">OK</button> Please use addEventListener() here as well. @@ +29,5 @@ > + <button onclick="show('http')">Http</button> > + <button onclick="show('sockets')">Sockets</button> > + <button onclick="show('dns')">DNS</button> > + <button onclick="show('websockets')">WebSockets</button> > + </div> We don't really use on* attributes anymore. Why not use #menu.addEventLister(click) and then check event.target to see which button was clicked? @@ +46,5 @@ > + <tbody id="http_content" /> > + </table> > + </div> > + > + <div id="sockets" style="display: none"> We don't use inline styles. These divs should probably have appropriate classes that mark them as a "tab" or the like. You can then switch between tabs by assigning or removing an "active" class for example.
Attachment #769133 - Flags: review?(ttaubert)
Thanks for the review, Tim. I'll try to fix everything and post an updated patch in the next few days.
(In reply to Tim Taubert [:ttaubert] from comment #5) > @@ +104,5 @@ > > + dash.requestWebsocketConnections(displayWebsockets); > > + dash.requestDNSInfo(displayDns); > > + } > > + > > + setInterval(get, 3000); > > Do you really want to automatically update the page every 3s? What if > there's some data in there that I want to copy/paste somewhere? That's quite > unexpected, IMO. about:networking should just work like about:memory in that > I manually need to refresh the page whenever I want the most up-to-date data. This one I can see going either way. I can think of plenty of cases in which I would want to have the data on this page updating as I do things (especially once there are a lot of graphs to show real-time traffic, etc), to see if the network layer is behaving as I expect. I know we aren't big fans of extraneous checkboxes, but given that this is designed to help us know what necko is doing, I think we should have a checkbox to say "auto-refresh this page every 3 seconds". It can be unchecked by default, to facilitate the copying of data that you mention, but I really think the option should be there. In future work, it would be great if a refresh didn't fully refresh the page, and instead was able to just add rows to the tables as necessary. I don't want to block landing the dashboard in firefox on that functionality, though, so that should be a follow-up bug.
I guess I don't feel very strongly about either of those options. If you think that landing this feature with auto-refresh would be best, then feel free to leave it that way and file a bug to make it optional later. I trust the people actually using the feature to make that call :) I just wanted to point out possibilities and possible drawbacks.
Attached patch Patch 1.2 (obsolete) — — Splinter Review
I think this fixes all the issues in the comments. Thanks to Robert for cleaning this up.
Attachment #769133 - Attachment is obsolete: true
Attachment #781025 - Flags: review?(ttaubert)
Comment on attachment 781025 [details] [diff] [review] Patch 1.2 Review of attachment 781025 [details] [diff] [review]: ----------------------------------------------------------------- This page needs RTL support. Also I wonder, why do have the warning dialog here? That feels not like something people can do bad things with, is it? ::: docshell/base/nsAboutRedirector.cpp @@ +65,5 @@ > nsIAboutModule::ALLOW_SCRIPT }, > { "telemetry", "chrome://global/content/aboutTelemetry.xhtml", > nsIAboutModule::ALLOW_SCRIPT }, > + { "networking", "chrome://global/content/aboutNetworking.xhtml", > + nsIAboutModule::ALLOW_SCRIPT }, Adding a new privileged about: page will require security review. ::: toolkit/content/aboutNetworking.css @@ +10,5 @@ > +} > + > +#menu { > + position: absolute; > + top: 0; Nit: I think this could use some more top margin? @@ +13,5 @@ > + position: absolute; > + top: 0; > +} > + > +div.warningBackground { Nit: no need to have the tag in the selector as well. @@ +24,5 @@ > + left:0; > + position:fixed; > +} > + > +div.warningMessage { Nit: no need to have the tag in the selector as well. @@ +39,5 @@ > + margin-left: auto; > + text-align: center; > +} > + > +div.tab { Nit: no need to have the tag in the selector as well. @@ +43,5 @@ > +div.tab { > + display: none; > +} > + > +div.active { Nit: no need to have the tag in the selector as well. @@ +45,5 @@ > +} > + > +div.active { > + display: block; > +} I think that active tabs should be a little more hightlighted. I cannot differentiate the active tab that's showing at the beginning and even after switching to a different tab it's just the focus ring that's helping. @@ +50,5 @@ > + > +#refreshdiv { > + top: 0; > + position: absolute; > + right: 0; Nit: I think this could use some right margin? ::: toolkit/content/aboutNetworking.js @@ +10,5 @@ > + > +Cu.import("resource://gre/modules/Services.jsm"); > + > +const dash = Cc['@mozilla.org/network/dashboard;1']. > + getService(Ci.nsIDashboard); A common convention is to name global gSomething. So that would be gDash and gPrefs. Makes it easy to differentiate from local variables later in the code. @@ +12,5 @@ > + > +const dash = Cc['@mozilla.org/network/dashboard;1']. > + getService(Ci.nsIDashboard); > +const prefs = Cc["@mozilla.org/preferences-service;1"]. > + getService(Ci.nsIPrefService).getBranch("network."); Either you use Services.prefs here or remove the Services.jsm include above. @@ +127,5 @@ > + > + document.getElementById("autorefcheck").addEventListener("click", function() { > + let refrButton = document.getElementById("refreshButton"); > + if (this.checked) { > + this.value = setInterval(requestNetworkingData, 3000); this.value is an unfortunate name. this.interval maybe? @@ +136,5 @@ > + } > + }); > + > + let refr = document.getElementById("refreshButton"); > + refr.addEventListener("click", getNetworkingData); Should this be 'requestNetworkingData'? @@ +146,5 @@ > + menu.addEventListener("click", function click(e) { > + if (!e.target.value) > + return; > + > + show(e.target.value); How about: if (e.target) show(e.target.value); @@ +167,5 @@ > + content.classList.add("active"); > +} > + > +window.addEventListener("load", function load() { > + window.removeEventListener("load", load); You can use DOMContentLoaded. That should fire a little earlier when the content is ready to be worked with. ::: toolkit/content/aboutNetworking.xhtml @@ +21,5 @@ > + <div id="warning_message" class="warningBackground"> > + <div class="warningMessage"> > + This is very experimental. Do not use without adult supervision. <br/> <br/> > + <label><input id="warncheck" type='checkbox' checked="yes" />Show this warning next time</label> <br/> <br/> > + <button id="confpref">OK</button> We'll need a DTD file that contains the strings used in the file to make it localizable.
Attachment #781025 - Flags: review?(ttaubert)
Daniel, are you the right one to sec-review this?
Flags: sec-review?(dveditz)
(In reply to Tim Taubert [:ttaubert] from comment #10) > Comment on attachment 781025 [details] [diff] [review] > Patch 1.2 > > Review of attachment 781025 [details] [diff] [review]: > ----------------------------------------------------------------- > > This page needs RTL support. Also I wonder, why do have the warning dialog > here? That feels not like something people can do bad things with, is it? See Nick's comment #0 There shouldn't be any risks of using the about page, except the fact that it's unfinished, and it may change often. Would you suggest removing the warning? > > ::: toolkit/content/aboutNetworking.css > @@ +10,5 @@ > > +} > > + > > +#menu { > > + position: absolute; > > + top: 0; > > Nit: I think this could use some more top margin? > A value that is not 0 makes it sit on top of the text below. CSS isn't my strong point, but I'll try to give it a better look.
(In reply to Tim Taubert [:ttaubert] from comment #10) > Adding a new privileged about: page will require security review. I think we're somewhat past the point where this policy is reasonable (there are plenty of such privileged pages and adding one more doesn't significantly impact risk). We do certainly need to be careful with the nsIAboutModule flags and making sure it's not linkable etc., but that can be covered by code review rather than a security review.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #13) > (In reply to Tim Taubert [:ttaubert] from comment #10) > > Adding a new privileged about: page will require security review. > > I think we're somewhat past the point where this policy is reasonable (there > are plenty of such privileged pages and adding one more doesn't > significantly impact risk). We do certainly need to be careful with the > nsIAboutModule flags and making sure it's not linkable etc., but that can be > covered by code review rather than a security review. Ok.
Status: NEW → ASSIGNED
Flags: sec-review?(dveditz)
(In reply to Valentin Gosu from comment #12) > > This page needs RTL support. Also I wonder, why do have the warning dialog > > here? That feels not like something people can do bad things with, is it? > > See Nick's comment #0 > There shouldn't be any risks of using the about page, except the fact that > it's unfinished, and it may change often. Would you suggest removing the > warning? It feels like we can simplify the code a little by removing it. The fact that it changes often and may be unfinished doesn't quite justify that dialog for me. But in the end it's really up to you, I'm just making a suggestion here. > > > +#menu { > > > + position: absolute; > > > + top: 0; > > > > Nit: I think this could use some more top margin? > > > > A value that is not 0 makes it sit on top of the text below. > CSS isn't my strong point, but I'll try to give it a better look. That just means that you would also need to move the content below it further away from the top. You should also investigate not having the menu positioned absolute so everything is moved to the bottom automatically when giving the menu a little more top margin.
Attached patch p1.patch (obsolete) — — Splinter Review
Fixed the nits. Added DTD file.
Attachment #781025 - Attachment is obsolete: true
Attachment #785379 - Flags: review?(ttaubert)
Comment on attachment 785379 [details] [diff] [review] p1.patch Review of attachment 785379 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thank you. The only thing missing is RTL support but I'm fine with you filing a follow-up bug about that. You can test RTL mode easily with https://addons.mozilla.org/en-US/firefox/addon/force-rtl/ Basically, the tabs and the refresh button should switch sides. The order of the buttons and table columns needs to change, too. r=me with the two comments addressed and a follow-up bug filed. ::: toolkit/content/aboutNetworking.js @@ +125,5 @@ > + > + document.getElementById("autorefcheck").addEventListener("click", function() { > + let refrButton = document.getElementById("refreshButton"); > + if (this.checked) { > + this.interval = setInterval(requestNetworkingData, 3000); The 3000 should be put in a const at the top of the file and named something like REFRESH_INTERVAL_MS. ::: toolkit/content/aboutNetworking.xhtml @@ +21,5 @@ > + <body id="body"> > + <div id="warning_message" class="warningBackground"> > + <div class="warningMessage"> > + &aboutNetworking.warning; <br/> <br/> > + <label><input id="warncheck" type='checkbox' checked="yes" />&aboutNetworking.showNextTime;</label> <br/> <br/> Nit: "checkbox" instead of 'checkbox' (double quotes)
Attachment #785379 - Flags: review?(ttaubert) → review+
Attachment #785379 - Attachment is obsolete: true
Keywords: checkin-needed
Blocks: 902596
https://hg.mozilla.org/mozilla-central/rev/dcb7c39d1cf8
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla26
Blocks: 903396
No longer blocks: 903396
Depends on: 903396
Depends on: 903404
Blocks: 903409
Depends on: 1378713
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: