Bug 20904 - gracefully handle too many console messages.
Summary: gracefully handle too many console messages.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Kevin McCullough
URL: http://www.boulder.nist.gov/div853/gr...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-09-17 14:39 PDT by Kevin McCullough
Modified: 2008-09-18 10:33 PDT (History)
0 users

See Also:


Attachments
Patch and manual test (8.89 KB, patch)
2008-09-17 14:43 PDT, Kevin McCullough
timothy: review-
Details | Formatted Diff | Diff
New Patch with test (8.91 KB, patch)
2008-09-17 16:25 PDT, Kevin McCullough
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin McCullough 2008-09-17 14:39:17 PDT
From the radar:

Safari, when opening the page 
    http://www.boulder.nist.gov/div853/greenfn/tutorial.html
goes bezerk with its error generation.
Not only does this drown out all other messages, it takes up a ton of CPU power and disk space, and to add insult to injury, it seems to happen not just one when the page is opened, but constantly while the page is active.

this seems like it would be a situation in which you'd like some kind of message that says "too many errors, can't proceed" kinda like how a compiler would.  changing this to enhancement request because this problem doesn't seem to affect the usability and responsiveness of Safari or the Web Inspector.

9/11/08 10:41 AM Kevin McCullough:
There are a couple of solutions to this.  First, we can group repeated messages and just let the user know how many were duplicated.  Second, we could have a cap on how many total messages we show and let it be changeable by the user.

I have  a potential patch for the first suggestion
Comment 1 Kevin McCullough 2008-09-17 14:40:39 PDT
<rdar://problem/5722310>
Comment 2 Kevin McCullough 2008-09-17 14:43:32 PDT
Created attachment 23510 [details]
Patch and manual test
Comment 3 Timothy Hatcher 2008-09-17 15:01:48 PDT
Comment on attachment 23510 [details]
Patch and manual test

+        //call a function here, badge with incremental

This comment isn't good, and doesn't help me understand the code. Remove it or make it better.

+        var repeatedMessage = this.currentGroup.messagesElement.lastChild;
+        if (!repeatedMessage)
+            return;

This is risky and prone to being broken. The Console object should store a reference to the last message element (like previousMessage but previousMessageElement). Then clearMessages would have to clear that reference too.

+            repeatedMessage.style['padding-left'] = "6px";

You should add a new class to the repeated message so the "6px" can be in the CSS and not in the code. Also we don't use single quotes in Inspector JS.

-.resource-sidebar-tree-item .bubble.warning {
+.resource-sidebar-tree-item .bubble.warning, .bubble.warning {

Just remove .resource-sidebar-tree-item.

+.console-error-level .console-message-text, .console-error-level-with-count {
     color: red;
 }
 
What is this? What is red?
Comment 4 Kevin McCullough 2008-09-17 15:15:05 PDT
-.resource-sidebar-tree-item .bubble.warning {
+.resource-sidebar-tree-item .bubble.warning, .bubble.warning {

Removing ".resource-sidebar-tree-item" did not propagate the style, I'm a little fuzzy on CSS but I think this means some other style takes priority?

+.console-error-level .console-message-text, .console-error-level-with-count {
     color: red;
 }

This is the color of the text in an error message.  I added one more style rule because the next rule in this file was for ". console-error-level::before" and added the red X.  I don't want that image so I needed a new rule.

Did you want a better value for "red"?  An RGB?  This was this way before.
Comment 5 Kevin McCullough 2008-09-17 16:25:20 PDT
Created attachment 23513 [details]
New Patch with test
Comment 6 Timothy Hatcher 2008-09-17 16:40:59 PDT
Comment on attachment 23513 [details]
New Patch with test

I would rename lastMessage to previousMessageElement to better match previousMessage, otherwise it is confusing. 

r=me

Make sure you set review to ? when posting a patch to Bugzilla so it shows up in the review queue.
Comment 7 Kevin McCullough 2008-09-18 10:33:47 PDT
Committed revision 36615, which groups repeated messages.

That seems to handle the memory issue too.