Closed Bug 585237 Opened 12 years ago Closed 12 years ago

Web Console should limit the number of lines displayed

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Firefox 4.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: pcwalton, Assigned: pcwalton)

References

Details

(Keywords: perf, Whiteboard: [kd4b6])

Attachments

(2 files, 1 obsolete file)

Because performance severely degrades when the Web Console displays many lines, it should limit the number of lines displayed. We should fix the performance issues, but in the Firefox 4 timeframe it seems that it would be simplest to just cap the number of lines at some reasonable number (1000? 2000?)

I'm requesting blocking status for Firefox 4 because, if the user leaves the Web Console open for a long time, the memory usage will potentially grow and grow as more nodes are added.
blocking2.0: --- → ?
The Toolkit Error Console sets a hard limit to 250 messages. I believe that with modern processors this can be bumped up to 500 messages before performance drops through the floor and makes the Console unusable. You could use this number as an initial starting point.
How hard would it be to make a setting in about:config? Then "hardcore" developers that have fast machines by default can increase this value. 500 sounds good for a normal developer.
(In reply to comment #2)
> How hard would it be to make a setting in about:config? Then "hardcore"
> developers that have fast machines by default can increase this value. 500
> sounds good for a normal developer.

It is not hard at all, in fact, our storage module inside the console can get and set this value as a regular pref. We just need to add it, and use it.

We can check for an override pref to ignore it "0" (unlimited), or set to another value like 10000 or something. I never do premature optimization:)
The current hard coded limit of 250 exists in two places: the toolkit error console, and in the backend nsIConsoleService. Make sure you add the pref to the C++ backend as well.
(In reply to comment #4)
> The current hard coded limit of 250 exists in two places: the toolkit error
> console, and in the backend nsIConsoleService. Make sure you add the pref to
> the C++ backend as well.

good call. looks like firebug is set to 500 as well:

extensions.firebug.console.logLimit;500

Phillip: do you know the name of the pref/how that would be done?
blocking2.0: ? → betaN+
Keywords: perf
Version: unspecified → Trunk
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsConsoleService.cpp#68
Found via Bug 80703.

Note the comment there:
    // XXX grab this from a pref!
    // hm, but worry about circularity, bc we want to be able to report
    // prefs errs...
Whiteboard: [kd4b6]
Severity: normal → blocker
Attached patch Proposed patch.Splinter Review
Proposed patch attached. This patch implements a preference that can be changed in about:config.
Attachment #472012 - Flags: feedback?(ddahl)
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Blocks: devtools4b7
Attachment #472012 - Flags: feedback?(ddahl) → feedback?(mihai.sucan)
Attachment #472012 - Flags: review?(sdwilsh)
Comment on attachment 472012 [details] [diff] [review]
Proposed patch.

The patch looks good. However:

+      // If there are no more children, then remove the group itself.
+      if (groupNode.querySelector(".hud-msg-node") == null) {
+        groupNode.parentNode.removeChild(groupNode);
+      }

The coding guidelines recommend you do:

if (!groupNode.querySelector(".hud-msg-node")) { ... }

(don't check for null, check if it evaluates to false)

Also the tabBrowser variable is a misnomer, as explained in the other patch feedback I provided. Change from getBrowserForTab() to gBrowser.selectedBrowser.

Lastly, the new preference should be added to browser/app/profile/firefox.js with a description. (check with someone who has more Mozilla experience - Robert should know)
Attachment #472012 - Flags: feedback?(mihai.sucan) → feedback+
(In reply to comment #9)
> (don't check for null, check if it evaluates to false)
This needs to be fixed.

> Lastly, the new preference should be added to browser/app/profile/firefox.js
> with a description. (check with someone who has more Mozilla experience -
> Robert should know)
I think having a hidden preference here is actually OK, so we don't need to add it to firefox.js.
Comment on attachment 472012 [details] [diff] [review]
Proposed patch.

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>   /**
>+   * Destroys lines of output if more lines than the allowed log limit are
>+   * present.
>+   *
>+   * @param nsIDOMNode aConsoleNode
>+   *        The DOM node that holds the output of the console.
>+   * @returns void
>+   */
>+  pruneConsoleOutputIfNecessary:
>+  function HS_pruneConsoleOutputIfNecessary(aConsoleNode)
We don't actually want to expose this method, AFAICT.  Please move it to a global function in the jsm.

>+  {
>+    let prefBranch = Services.prefs.getBranch("devtools.hud.");
>+    let logLimit = prefBranch.getIntPref("loglimit");
Having a hidden pref means that this will throw when it's not set.  Wrap the getting in a try catch, and constify the default value.

>+
>+    // Set the default value for our preference. We have only one, "loglimit".
>+    let defaultPrefBranch = Services.prefs.getDefaultBranch("devtools.hud.");
>+    defaultPrefBranch.setIntPref("loglimit", 200);
No need to do this.

r=sdwilsh with comments addressed.
Attachment #472012 - Flags: review?(sdwilsh) → review+
Attached patch Proposed patch, version 1.1. (obsolete) — Splinter Review
Patch updated per review.
Keywords: checkin-needed
New patch fixes an issue in the test whereby the pref wasn't being deleted after the test was run.
Attachment #473128 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/0e773b4cc7ec
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Target Milestone: --- → Firefox 4.0b6
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.