Closed Bug 1303762 Opened 3 years ago Closed 3 years ago

Add button to enable logging to about:networking

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
thunderbird_esr45 - ---
firefox52 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-active])

Attachments

(1 file, 1 obsolete file)

+++ 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-
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
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.
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
https://hg.mozilla.org/mozilla-central/rev/6edd719019eb
https://hg.mozilla.org/mozilla-central/rev/8708f9c2e9fe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Duplicate of this bug: 193873
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.
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.