585237 - Web Console should limit the number of lines displayed

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

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.

Comment 1

[Philip Chee]

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.

Comment 2

[Julian Viereck]

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.

Comment 3

[David Dahl :ddahl]

(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:)

Comment 4

[Philip Chee]

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.

Comment 5

[David Dahl :ddahl]

(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?

Comment 6

[Philip Chee]

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...

Comment 7

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch.

Proposed patch attached. This patch implements a preference that can be changed in about:config.

Comment 8

[Kevin Dangoor]

Reprioritizing bugs. You can filter the mail on the word TEABAGS.

Comment 9

[Mihai Sucan [:msucan]]

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)

Comment 10

[Shawn Wilsher :sdwilsh]

(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 11

[Shawn Wilsher :sdwilsh]

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.

Comment 12

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 1.1.

Patch updated per review.

Comment 13

[Patrick Walton (:pcwalton)]

Attachment: Proposed patch, version 1.2.

New patch fixes an issue in the test whereby the pref wasn't being deleted after the test was run.

Comment 14

[David Dahl :ddahl]

http://hg.mozilla.org/mozilla-central/rev/0e773b4cc7ec