Console should have user-settable log limits for each message category

RESOLVED FIXED in Firefox 6

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rc, Assigned: past)

Tracking

unspecified
Firefox 6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [console][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

6 years ago
Currently the Web Console has a single pref (devtools.hud.loglimit) controlling maximum number of retained lines. This maximum number includes items that may be hidden by the filter buttons, so it can be quite jarring if a user only sees 5 network requests and they start to disappear because there were 195 CSS errors even though they're invisible.

We should have prefs for each of JS/Web Developer, CSS and Network so someone trying to catch a network request doesn't have older items removed from the console when they start getting pruned by other message categories.
(Reporter)

Updated

6 years ago
Whiteboard: [console]
(Reporter)

Updated

6 years ago
Duplicate of this bug: 647460
Assignee: nobody → past
Created attachment 531032 [details] [diff] [review]
Proposed patch

The patch replaces devtools.hud.loglimit with four separate knobs, devtools.hud.loglimit.{console,network,cssparser,exception} (for consistency with the names in CATEGORY_CLASS_FRAGMENTS), adds a test, and updates a few more. For faster pruning I've changed the signature of a helper function, in order to take advantage of the fact that we prune on each console output.
Attachment #531032 - Flags: feedback?(rcampbell)
(Reporter)

Comment 3

6 years ago
Comment on attachment 531032 [details] [diff] [review]
Proposed patch

Review of attachment 531032 [details] [diff] [review]:
-----------------------------------------------------------------

// "devtools.hud.loglimit.<CATEGORY_CLASS_FRAGMENT>" preference.

what is <CATEGORY_CLASS_FRAGMENT>? I think I'd prefer to see the actual prefs enumerated here.

Overall, I think the patch looks good, but I'd like to see some additional tests for each class. Afaict, you're only testing the console pref and css.
Status: NEW → ASSIGNED
Created attachment 531566 [details] [diff] [review]
Updated patch

Fixed comment and added more tests.
Attachment #531032 - Attachment is obsolete: true
Attachment #531032 - Flags: feedback?(rcampbell)
Attachment #531566 - Flags: feedback?(rcampbell)
The removal of the devtools.hud.loglimit knob was discussed in this thread: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/582be2d9d19befd2#
(Reporter)

Comment 6

6 years ago
Comment on attachment 531566 [details] [diff] [review]
Updated patch

looks good.

I think you should add a comment to firefox.js to include a reference to these hidden prefs and explain what they can be used for.

with that...
Attachment #531566 - Flags: feedback?(rcampbell) → feedback+
Created attachment 531885 [details] [diff] [review]
Updated patch

Good idea, default preferences added.
Attachment #531566 - Attachment is obsolete: true
Attachment #531885 - Flags: review?(rcampbell)
(In reply to comment #7)
> Created attachment 531885 [details] [diff] [review] [review]
> Updated patch
> 
> Good idea, default preferences added.

...or did you mean to keep them hidden and just put the comments in?
(Reporter)

Comment 9

6 years ago
I did originally, though we might as well put them in explicitly to aid discoverability.
(Reporter)

Comment 10

6 years ago
Comment on attachment 531885 [details] [diff] [review]
Updated patch

in browser_webconsole_bug_644419_log_limits.js

+  // Find the sentinel entry.
+  findLogEntry("testing Net limits");
+  // Fill the log with network messages.
+  let body = content.document.getElementsByTagName("body")[0];
+  for (let i = 0; i < 10; i++) {
+    var image = content.document.createElement("img");
+    image.src = "test-image.png?_fubar=" + i;
+    body.insertBefore(image, body.firstChild);
+  }
+  image.addEventListener("load", onImageLoad, true);
+}

adding the listener on the last image seems a bit iffy. I think it'll work, but you're betting on out-running the image load.

Anyway, this looks fine to me. Thanks for adding the additional tests. R+ with successful run through try server.
Attachment #531885 - Flags: review?(rcampbell) → review+
(In reply to comment #10)
> flag: review?(rcampbell@mozilla.com) => review+Comment on attachment 531885 [details] [diff] [review]
> [details] [review]
> Updated patch
> 
> in browser_webconsole_bug_644419_log_limits.js
> 
> +  // Find the sentinel entry.
> +  findLogEntry("testing Net limits");
> +  // Fill the log with network messages.
> +  let body = content.document.getElementsByTagName("body")[0];
> +  for (let i = 0; i < 10; i++) {
> +    var image = content.document.createElement("img");
> +    image.src = "test-image.png?_fubar=" + i;
> +    body.insertBefore(image, body.firstChild);
> +  }
> +  image.addEventListener("load", onImageLoad, true);
> +}
> 
> adding the listener on the last image seems a bit iffy. I think it'll work,
> but you're betting on out-running the image load.

I'm waiting for the images to load first, before I check for the relevant messages. I had to resort to this, since executeSoon() would fire before the network messages were displayed in the console. 

> Anyway, this looks fine to me. Thanks for adding the additional tests. R+
> with successful run through try server.

Will get one running as soon as I receive my commit access, thanks!
Comment on attachment 531885 [details] [diff] [review]
Updated patch

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

nit: don't repeat the same slightly-tweaked comment multiple times, just make it generic enough to put above all of the prefs, and group them together in a block.

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>+function pruneConsoleOutputIfNecessary(aConsoleNode, aCategory)

>+    let prefBranch = Services.prefs.getBranch("devtools.hud.loglimit.");
>+    logLimit = prefBranch.getIntPref(CATEGORY_CLASS_FRAGMENTS[aCategory]);

Not new to your code, but one other thing worth changing while you're here:
Just use Services.prefs.getIntPref directly rather than getting a branch (with a local variable for the pref name if the line ends up too long).
Created attachment 532656 [details] [diff] [review]
Updated patch

Updated patch to address Gavin's comments, but still waiting for a try build. Gavin, should I ping you for a formal review after a successful run through try?
Attachment #531885 - Attachment is obsolete: true
sure
Created attachment 532957 [details] [diff] [review]
Updated patch

Removed some whitespace fixes that crept in the last version. Try run was successful: http://tbpl.mozilla.org/?tree=Try&rev=00a257c7a45c
Attachment #532656 - Attachment is obsolete: true
Attachment #532957 - Flags: review?(gavin.sharp)
Comment on attachment 532957 [details] [diff] [review]
Updated patch

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>  * Destroys lines of output if more lines than the allowed log limit are
>+ * present. The implementation takes advantage of the fact that the function is
>+ * called on each update to the console, by using the last message category to
>+ * limit the search for necessary pruning.

I don't understand this comment at all ("implementation takes advantage" and "last message" are just confusing). "Ensures that the number of message nodes of type aCategory don't exceed that category's line limit by removing old messages as needed." is more concise and accurate, I think?

>+ * @param integer aCategory
>+ *        The category of the last message that was displayed in the console.

Similarly, reference to "last message" is confusing. Just say "category of message nodes to limit" or something like that.

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_644419_log_limits.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_644419_log_limits.js

>+function testWebDevLimits(aEvent) {
>+  browser.removeEventListener(aEvent.type, arguments.callee, true);
>+
>+  gOldPref = 200;
>+  try {
>+    gOldPref = Services.prefs.getIntPref("devtools.hud.loglimit.console");
>+  }
>+  catch (ex) { }

Don't need the try/catch since you're adding a default pref value (applies to this block elsewhere in the test).

I'm a bit confused by these tests - you're adding a sentinel log entry, setting the limit pref to 10, and then adding ten more entries and testing that the sentinel entry is still there - but shouldn't it have been pushed out by the 10 new entries? What am I missing?

>+function testNetLimits(aEvent) {

>+  for (let i = 0; i < 10; i++) {
>+    var image = content.document.createElement("img");
>+    image.src = "test-image.png?_fubar=" + i;
>+    body.insertBefore(image, body.firstChild);
>+  }
>+  image.addEventListener("load", onImageLoad, true);

I don't see any guarantee that the last inserted image's onload will fire last, which seems like it could cause intermittent orange.
Attachment #532957 - Flags: review?(gavin.sharp) → review-
Created attachment 533224 [details] [diff] [review]
Updated patch

New version with review comments incorporated.
Attachment #532957 - Attachment is obsolete: true
Attachment #533224 - Flags: review?(gavin.sharp)
(In reply to comment #16)
> Comment on attachment 532957 [details] [diff] [review] [review]
> Updated patch
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >  * Destroys lines of output if more lines than the allowed log limit are
> >+ * present. The implementation takes advantage of the fact that the function is
> >+ * called on each update to the console, by using the last message category to
> >+ * limit the search for necessary pruning.
> 
> I don't understand this comment at all ("implementation takes advantage" and
> "last message" are just confusing). "Ensures that the number of message
> nodes of type aCategory don't exceed that category's line limit by removing
> old messages as needed." is more concise and accurate, I think?

Indeed, I've used your suggested wording.

> >+ * @param integer aCategory
> >+ *        The category of the last message that was displayed in the console.
> 
> Similarly, reference to "last message" is confusing. Just say "category of
> message nodes to limit" or something like that.

Done.

> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_644419_log_limits.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_644419_log_limits.js
> 
> >+function testWebDevLimits(aEvent) {
> >+  browser.removeEventListener(aEvent.type, arguments.callee, true);
> >+
> >+  gOldPref = 200;
> >+  try {
> >+    gOldPref = Services.prefs.getIntPref("devtools.hud.loglimit.console");
> >+  }
> >+  catch (ex) { }
> 
> Don't need the try/catch since you're adding a default pref value (applies
> to this block elsewhere in the test).

Removed everywhere.

> I'm a bit confused by these tests - you're adding a sentinel log entry,
> setting the limit pref to 10, and then adding ten more entries and testing
> that the sentinel entry is still there - but shouldn't it have been pushed
> out by the 10 new entries? What am I missing?

The point of the patch and the tests is to make sure that one category's entries don't interfere with those from other categories. You'll see that the sentinel entry is always from a different category than the one being flooded.

> >+function testNetLimits(aEvent) {
> 
> >+  for (let i = 0; i < 10; i++) {
> >+    var image = content.document.createElement("img");
> >+    image.src = "test-image.png?_fubar=" + i;
> >+    body.insertBefore(image, body.firstChild);
> >+  }
> >+  image.addEventListener("load", onImageLoad, true);
> 
> I don't see any guarantee that the last inserted image's onload will fire
> last, which seems like it could cause intermittent orange.

Actually it would succeed even when it shouldn't, since we only checked that one file was loaded later on. But yes, this was definitely not a good test. Fixed.
And another try run, just to be on the safe side:

http://tbpl.mozilla.org/?tree=Try&rev=e1731f420acc
Comment on attachment 533224 [details] [diff] [review]
Updated patch

(In reply to comment #18)
> The point of the patch and the tests is to make sure that one category's
> entries don't interfere with those from other categories.

Oh, I see. Should they also verify that truncation occurs while they're at it? Or is that covered by other tests?
Attachment #533224 - Flags: review?(gavin.sharp) → review+
Created attachment 533561 [details] [diff] [review]
[checked-in] [in-devtools] Final patch

(In reply to comment #20)
>> The point of the patch and the tests is to make sure that one category's
>> entries don't interfere with those from other categories.
>
> Oh, I see. Should they also verify that truncation occurs while they're at
> it? Or is that covered by other tests?

There are a couple of other tests, but you are right, this test should be
more thorough, so I made a trivial change to that effect. Also rebased to
the latest tip.
Attachment #533224 - Attachment is obsolete: true
Whiteboard: [console] → [console][checkin-needed]
(Reporter)

Updated

6 years ago
Whiteboard: [console][checkin-needed] → [console][fixed-in-devtools]
(Reporter)

Comment 22

6 years ago
Comment on attachment 533561 [details] [diff] [review]
[checked-in] [in-devtools] Final patch

http://hg.mozilla.org/projects/devtools/rev/b00797b2e0b9
Attachment #533561 - Attachment description: Final patch → [in-devtools] Final patch

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [console][fixed-in-devtools] → [console][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6

Comment 23

6 years ago
Comment on attachment 533561 [details] [diff] [review]
[checked-in] [in-devtools] Final patch

http://hg.mozilla.org/mozilla-central/rev/b00797b2e0b9
Attachment #533561 - Attachment description: [in-devtools] Final patch → [checked-in] [in-devtools] Final patch
You need to log in before you can comment on or make changes to this bug.