Closed
Bug 585237
Opened 14 years ago
Closed 14 years ago
Web Console should limit the number of lines displayed
Categories
(DevTools :: General, defect)
DevTools
General
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)
(deleted),
patch
|
sdwilsh
:
review+
msucan
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 1•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
(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?
Updated•14 years ago
|
Comment 6•14 years ago
|
||
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...
Updated•14 years ago
|
Whiteboard: [kd4b6]
Updated•14 years ago
|
Severity: normal → blocker
Assignee | ||
Comment 7•14 years ago
|
||
Proposed patch attached. This patch implements a preference that can be changed in about:config.
Attachment #472012 -
Flags: feedback?(ddahl)
Comment 8•14 years ago
|
||
Reprioritizing bugs. You can filter the mail on the word TEABAGS.
Severity: blocker → normal
Updated•14 years ago
|
Blocks: devtools4b7
Assignee | ||
Updated•14 years ago
|
Attachment #472012 -
Flags: feedback?(ddahl) → feedback?(mihai.sucan)
Updated•14 years ago
|
Attachment #472012 -
Flags: review?(sdwilsh)
Comment 9•14 years ago
|
||
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+
Comment 10•14 years ago
|
||
(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•14 years ago
|
||
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+
Assignee | ||
Comment 12•14 years ago
|
||
Patch updated per review.
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 13•14 years ago
|
||
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
Comment 14•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Keywords: checkin-needed
Target Milestone: --- → Firefox 4.0b6
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•