1303762 - Add button to enable logging to about:networking

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

+++ This bug was initially created as a clone of Bug #1239686 +++

Setting environment variables or even prefs to turn on logging is a bit cumbersome for non-technical users.
A bit better would be a section in about:networking with a button to turn on logging, and maybe a few checkboxes to enable/disable common necko modules.

Use case:
Go to about:networking
Click on Logging tab
Enable required modules
Click on Start Logging
Reproduce bug
Click on Stop Logging
Upload logs to bug.

Comment 1

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

Attachment: Add UI to about:networking to turn on logging modules at runtime

This patch changes the initial approach of using checkboxes, to something similar to setting environment variables. This makes it easier for us to instruct the user <Just set the second box to X>.

I want to point out that this wasn't meant to look great, or to have a good UX. We just want to have something that allows people to start logging without restarting the browser, or knowing how the command line works. We can address any usability issues in a followup bug. Thanks!

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8794653 [details] [diff] [review]
Add UI to about:networking to turn on logging modules at runtime

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

This isn't too far off but I'd like to see it one more time before it gets checked in.

Note: I can't review the /netwerk changes.

::: toolkit/content/aboutNetworking.js
@@ +10,5 @@
>  
> +const gEnv = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +
> +const FileUtils = Cu.import("resource://gre/modules/FileUtils.jsm").FileUtils
> +Cu.import("resource://gre/modules/Services.jsm");

Please put these in alphabetical order.

@@ +16,3 @@
>  const gDashboard = Cc['@mozilla.org/network/dashboard;1'].
>    getService(Ci.nsIDashboard);
> +const gDirServ = Cc["@mozilla.org/file/directory_service;1"]

I see three different line-wrapping/indenting styles all within 8 lines of each other. Please make these consistent.

@@ +227,5 @@
> +  try {
> +    let file = gDirServ.getFile("TmpD",  {});
> +    file.append("log.txt");
> +    document.getElementById("log-file").value = file.path;
> +  } catch(e) {

You should run `mach lint toolkit/content/aboutNetworking.js`. This line and two others are causing errors and they will fail the build when you land this.

@@ +251,5 @@
> +  // at runtime.
> +  if (logPath.length > 0) {
> +    currentLogFile.innerText = logPath;
> +    // If the log file is set by an environment variable at startup, do not
> +    // allow changing it at runtime through a pref. It wouldn't work anyway.

This comment is a duplicate of the comment three lines higher.

@@ +272,5 @@
> +    currentLogModules.innerText = logModules;
> +    // If the log modules are set by an environment variable at startup, do not
> +    // allow changing them throught a pref. It would be difficult to figure out
> +    // which ones are enabled and which ones are not. The user probably knows
> +    // what he is doing.

Please change this to "what they are doing" or remove the last sentence altogether. We shouldn't assume the user is a he or she.

@@ +312,5 @@
> +  try {
> +    let currentLogFile = Services.prefs.getCharPref("logging.config.LOG_FILE");
> +    if (currentLogFile == logFile) {
> +      // No need to set it again.
> +      return;

There is no harm in setting a to its identical value, we can remove this code and make the function shorter.

@@ +317,5 @@
> +    }
> +  } catch (e) {}
> +
> +  Services.prefs.setCharPref("logging.config.LOG_FILE", logFile);
> +  document.getElementById("log-file").value = "";

Please don't clear the value of the input here. If the user clicks on the button twice, an empty value gets saved and the user has no way of getting back their previous value.

@@ +328,5 @@
> +    // Turn off all the modules.
> +    let children = Services.prefs.getBranch("logging.").getChildList("", {});
> +    for (let pref of children) {
> +      if (!pref.startsWith("config.")) {
> +        Services.prefs.clearUserPref("logging."+pref);

nit, please use a template string like so: `logging.${pref}`

@@ +346,5 @@
> +      addTimestamp = true;
> +    } else if (module == "rotate") {
> +      // XXX: rotate is not yet supported.
> +    } else if (module == "append") {
> +      // XXX: append is not yet supported.

Are there bugs on file for rotate and append support? Please add the bug numbers to these comments.

@@ +351,5 @@
> +    } else if (module == "sync") {
> +      isSync = true;
> +    } else {
> +      let [key, value] = module.split(":");
> +      Services.prefs.setIntPref("logging."+key, parseInt(value));

Please switch to a template string and add the second argument to parseInt to specify the base. parseInt(value, 10);

@@ +358,5 @@
> +  Services.prefs.setBoolPref("logging.config.add_timestamp", addTimestamp);
> +  Services.prefs.setBoolPref("logging.config.sync", isSync);
> +
> +  updateLogModules();
> +  document.getElementById("log-modules").value = "";

Ditto from above, please don't clear the value of the input here. If the user clicks on the button twice, an empty value gets saved and the user has no way of getting back their previous value.

::: toolkit/content/aboutNetworking.xhtml
@@ +143,5 @@
> +            <div>
> +              &aboutNetworking.logTutorial;
> +            </div>
> +            <div>
> +              &aboutNetworking.currentLogFile;: <div id="current-log-file"></div><br/>

The colon needs to be part of the localizable string.

::: toolkit/locales/en-US/chrome/global/aboutNetworking.dtd
@@ +29,5 @@
>  <!ENTITY aboutNetworking.messagesReceived      "Messages Received">
>  <!ENTITY aboutNetworking.bytesSent             "Bytes Sent">
>  <!ENTITY aboutNetworking.bytesReceived         "Bytes Received">
> +<!ENTITY aboutNetworking.logging               "Logging">
> +<!ENTITY aboutNetworking.logTutorial           "See <a href='https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging'> HTTP Logging </a> for instructions on how to use this tool.">

You should not include spaces around "HTTP Logging" because there are already spaces after "See " and before " for".

The URL should be https://developer.mozilla.org/docs/Mozilla/Debugging/HTTP_logging as the server will redirect to select the proper locale. Many localizers will not change these strings, and we don't want to point them towards the en-US locale if there is a better version available.

Comment 3

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

Attachment: Add UI to about:networking to turn on logging modules at runtime

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8795241 [details] [diff] [review]
Add UI to about:networking to turn on logging modules at runtime

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

r=me for the JS/XHTML/DTD changes.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4508f246a862379bc3e40380f1972fa808fc685
Bug 1303762 - Add UI to about:networking to turn on logging modules at runtime r=jaws,mcmanus

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6edd719019ebae0311dce299d2bf39db5c79306b
Bug 1303762 - Add aboutNetworking.dtd to whitelist a=Aryx

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8708f9c2e9fe
Add UI to about:networking to turn on logging modules at runtime r=jaws,mcmanus

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Pulsebot from comment #7)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8708f9c2e9fe
> Add UI to about:networking to turn on logging modules at runtime
> r=jaws,mcmanus

Ugh, didn't catch that a followup was pushed and I backed out e4508f246a862379bc3e40380f1972fa808fc685

This is a relanding of it.

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9fddae1ada2
Backed out changeset e4508f246a86 for failures in browser_misused_characters_in_strings.js a=backout

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6edd719019eb
https://hg.mozilla.org/mozilla-central/rev/8708f9c2e9fe

Comment 12

[Charles]

Any chance this can be applied for Thunderbird?

Comment 13

[Kent James (:rkent)]

(In reply to Charles from comment #12)
> Any chance this can be applied for Thunderbird?

It will appear in TB52 naturally, but because of string changes it is not a candidate for uplift to TB52.

Comment 14

[Charles]

Assuming you meant 45, no worries...

I'm looking forward to 52, it should be a great release.

Thanks!