Add button to enable logging to about:networking

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
6 months ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(thunderbird_esr45-, firefox52 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

+++ 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.
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!
Attachment #8794653 - Flags: review?(jaws)
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.
Attachment #8794653 - Flags: review?(jaws) → review-
(Assignee)

Updated

3 years ago
Attachment #8794653 - Attachment is obsolete: true
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.
Attachment #8795241 - Flags: review?(jaws) → review+
Attachment #8795241 - Flags: review?(mcmanus) → review+
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 7

3 years ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8708f9c2e9fe
Add UI to about:networking to turn on logging modules at runtime r=jaws,mcmanus
(In reply to Pulsebot from comment #7)
> Pushed by kwierso@gmail.com:
> 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

3 years ago
Backout by kwierso@gmail.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6edd719019eb
https://hg.mozilla.org/mozilla-central/rev/8708f9c2e9fe
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Updated

3 years ago
Duplicate of this bug: 193873

Comment 12

3 years ago
Any chance this can be applied for Thunderbird?
(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

3 years ago
Assuming you meant 45, no worries...

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

Thanks!
You need to log in before you can comment on or make changes to this bug.