Repeated identical messages in the Web Console should be collapsed into one or two messages to avoid flooding the console. IMO the most usable way to do this would be to have a "(Previous message repeats n times)" message. But this breaks string freeze. UX input on this would be helpful.

could we add a (xN) to the end showing number of repeats? Again, we'd need some l10n feedback for this as I'm not sure that translates universally.

WebKit uses a circled red number, but I'm not sure how usable that really is.

Really? I don't think I've personally found the circled number confusing in the slightest.

Is this a duplicate or dependency of Bug 609331 (Web Console should collapse equivalent messages together)?

mass change: filter on PRIORITYSETTING

I think webkit's approach to it is good. No one can get confused by that. When are we expecting this change?

we're hoping to get this in Firefox 4.

I think we missed this when nominating blockers. Would really like to get it in for Fx4 because on some sites with a lot of these messages, the repeats totally obscure any and all other output on a console.

also marking dependent on our long-lived styling bug 605621.

If this has string impact, it can't block, plain and simple. Remove the [strings] and renominate if you can get around that.

let's go with the number badge. I don't see a need to add more space to the message for the repeated message string anyway. [strings] removed!

and the renom.

Would the 'Error Console' also be considered for this change? (I don't understand why we have two of them now in FF 4, but that's a separate thing.)

(In reply to comment #14) > Would the 'Error Console' also be considered for this change? (I don't > understand why we have two of them now in FF 4, but that's a separate thing.) no. The Error Console is going to be strictly for chrome-level messages. See bug 593540.

mac styling only so far. This is going to rely on the truncated url patch in bug 611440 so the labels aren't pushed off screen.

added check for same source, basic linux styling.

final draft. submitting for feedback.

test fixes and a unit test for the repeater code

Comment on attachment 507244 [details] [diff] [review] repeats tests and requesting feedback

Comment on attachment 507243 [details] [diff] [review] repeats v1.0 adding mihai to the feedback request.

Comment on attachment 507243 [details] [diff] [review] repeats v1.0 This is a really nice improvement, especially when viewing sites like twitter.com. Patch comments: + isSameSource: function ConsoleUtils_isSameSource(aMsg1, aMsg2) { This new method is unused. + for (let i = aOutput.childNodes.length - 1; i >= 0; --i) { + let lastMessage = aOutput.childNodes[i]; + + if (!lastMessage.classList.contains("webconsole-msg-cssparser")) + continue; Why not do aOutput.querySelectorAll(".webconsole-msg-cssparser") ? Then loop through the result. In filterRepeatedCSS() you find the old occurrences of CSS parser errors and increment the number, and you also re-append the error to the end. This poses two issues: - you don't scroll to the "new" error. - you create much visual noise. I'd suggest to not re-append the error. Let it stay in place. Afaik, this is what Web Inspector also does. (or maybe it's just a preference of mine) Otherwise, the patch looks fine. Feedback+ with the above comments addressed! (awesome polish, btw)

Comment on attachment 507244 [details] [diff] [review] repeats tests Test looks fine!

(In reply to comment #26) > Comment on attachment 507243 [details] [diff] [review] > repeats v1.0 > > This is a really nice improvement, especially when viewing sites like > twitter.com. > > Patch comments: > > + isSameSource: function ConsoleUtils_isSameSource(aMsg1, aMsg2) { > > This new method is unused. oops! I thought I'd gotten rid of that. > > + for (let i = aOutput.childNodes.length - 1; i >= 0; --i) { > + let lastMessage = aOutput.childNodes[i]; > + > + if (!lastMessage.classList.contains("webconsole-msg-cssparser")) > + continue; > > Why not do aOutput.querySelectorAll(".webconsole-msg-cssparser") ? Then loop > through the result. Yeah, considered doing that, but wasn't sure if the performance hit to first scan out all of the cssparser messages and then iterate through them would be worse than just iterating through the whole collection. I think I'll do the qSA anyway for the sake of readability and eliminating the check inside the loop. > In filterRepeatedCSS() you find the old occurrences of CSS parser errors and > increment the number, and you also re-append the error to the end. This poses > two issues: > > - you don't scroll to the "new" error. > - you create much visual noise. Wasn't sure about that part of it either. What I liked about re-adding it to the end of the console is that when you get a repeat, the timestamp is going to be newer than the first occurrence. It seems like you might want the most recent occurrence in the list. I can see an argument going either way though, (when did I first see this occurrence vs. when was the last one?) so I'm inclined to go in favor of the least amount of visual noise. > I'd suggest to not re-append the error. Let it stay in place. Afaik, this is > what Web Inspector also does. (or maybe it's just a preference of mine) Yeah, hard to say. You've changed my mind. :) > Otherwise, the patch looks fine. Feedback+ with the above comments addressed! > > (awesome polish, btw) Great! Thank you. I think this patch goes a long way to making the console feel more finished and reduces the clutter in the contents significantly.

updated patch based on feedback.

Comment on attachment 507574 [details] [diff] [review] repeats v1.1 Don't need to explicitly convert to String(), that happens implicitly in the call to setAttribute(). Hardcoding childNodes[2/3/4] in mergeFilteredMessageNode/filterRepeatedCSS/filterRepeatedConsole seems fragile. Manually iterating over all past message nodes and comparing them for each addition is expensive. I wonder whether it'd be better to hash the message+filename and store that in the message node's class, and then just look for it via getElementsByClassName? Should the filterRepeated* methods return the node that the new message was merged with, so that it can be made visible? I guess that'd be odd since it could result in the console jumping around, but it also seems odd that new errors can occur without being visible, if the previous copy of it is out of view...

(In reply to comment #30) > Comment on attachment 507574 [details] [diff] [review] > repeats v1.1 > > Don't need to explicitly convert to String(), that happens implicitly in the > call to setAttribute(). Yeah, Overkill. Will fix. > Hardcoding childNodes[2/3/4] in > mergeFilteredMessageNode/filterRepeatedCSS/filterRepeatedConsole seems fragile. A little. I can add some accessor methods to get the child nodes within a message. > Manually iterating over all past message nodes and comparing them for each > addition is expensive. I wonder whether it'd be better to hash the > message+filename and store that in the message node's class, and then just look > for it via getElementsByClassName? That's a funky idea. I thought about constructing an xpath query to dig out the elements but storing a hash we can use one of the quick DOM queries is kind of nice. > Should the filterRepeated* methods return the node that the new message was > merged with, so that it can be made visible? I guess that'd be odd since it > could result in the console jumping around, but it also seems odd that new > errors can occur without being visible, if the previous copy of it is out of > view... Mihai and I kind of went around this. Originally, I was merging the repeat with the original message (taking the timestamp from the latest) and re-appending it to the bottom of the list. Mihai thought this was too noisy and I kind of agreed with him, though really, it's a bit subjective. Since the jumping could be considered "noisy" to some, and not-jumping is probably less annoying, I figured we should go with the less annoying option. Also, it makes for simpler code.

work in progress patch. Works well in interactive testing, but for some reason, I'm getting a ton of mochitest browser chrome failures. Anything which seems to do "console = contentWindow.wrappedJSObject.console" followed by a console.log('content') doesn't dump anything to the console. Not sure why, console.log messages work fine from content and the input line. need to debug this a bit more. feedback welcome,

more testing. The cause of the slowdown is from this block in ConsoleUtils: if (!isRepeated && (aNode.classList.contains("webconsole-msg-console") || aNode.classList.contains("webconsole-msg-exception") || aNode.classList.contains("webconsole-msg-error"))) isRepeated = this.filterRepeatedConsole(aNode, outputNode); I commented it out, and all tests pass (except for the new one that's expecting the repeat node to have a value of 10). Since console messages only check the last output message node, I'm going to revert to using a string comparison for those (not hashing). It only happens once and is apparently quicker. With that, I'll push it through try to see if it passes and report back.

Why md5-hash instead of just concatenating a string key to lookup in a JS object? JS-internal hashing should be as fast as string comparing, to first approximation. /be

I was hoping to avoid having an extra storage object to do this, but talking to Mihai, I think it's a better suggestion than using md5 hashes which are turning out to be too expensive. I'll give it a try and hopefully have a patch up today!

here's a version with a stored cache of nodes (cssNodes on the hudReference, aka HeadsUpDisplay object). I didn't know where to stash these. David mentioned the Storage object, but I didn't see anything in there and wasn't sure I needed the extra indirection. If either of you can think of a better place for these, please let me know. Also, this is showing leaks. I'm not catching all the references on pruning or shutdown. I'm not sure which. Anything look obvious to either of you?

(In reply to comment #36) > I didn't know where to stash these. David mentioned the Storage object, but I > didn't see anything in there and wasn't sure I needed the extra indirection. If > either of you can think of a better place for these, please let me know. > There is a constructor called ConsoleStorage that keeps track of all prefs and was a store of all messages - you could add an object and accessor methods there to store the repeatnodes - if you want. otherwise, I would change the name of the cssNodes property to something more specific like 'repeatCssNodes' > Also, this is showing leaks. I'm not catching all the references on pruning or > shutdown. I'm not sure which. Anything look obvious to either of you? looking...

I used cssNodes because it is literally all of them. I'll check into using the ConsoleStorage object. Thanks for taking a pass through this. Mihai was looking earlier and nothing obvious jumped out at him regarding the leaks. It seems like just handling a DOM node in an array can make them hang around long after the array has disappeared. At least for the lifetime of our test harness.

This is just Rob's patch with the leak fixed.

Once this passes reviews, it has implicit a=beltzner

Comment on attachment 512653 [details] [diff] [review] Rob's patch with a leak fix >+++ b/toolkit/components/console/hudservice/HUDService.jsm >+ if (messageNodes[i].classList.contains("webconsole-msg-cssparser")) { >+ let desc = messageNodes[i].childNodes[2].textContent; >+ let location = messageNodes[i].childNodes[4].getAttribute("title"); >+ delete hudRef.cssNodes[desc+location]; nit: space around the '+' > clearDisplay: function HS_clearDisplay(aHUD) > { > if (typeof(aHUD) === "string") { > aHUD = this.getOutputNodeById(aHUD); > } > >+ let hudRef = HUDService.getHudReferenceForOutputNode(aHUD); >+ >+ if (hudRef) >+ hudRef.cssNodes = {}; global-nit (10 instances): brace one line ifs (including ones with a one line else) (hey look, I just collapsed a message! :P) > /** >+ * Returns the hudReference for a given output node. >+ * >+ * @param nsIDOMNode an output node (as returned by getOutputNodeById()). >+ * @return a HUD or null >+ */ nit: I know the style is inconsistent in the file, but the one that we are moving towards looks like this: https://hg.mozilla.org/mozilla-central/annotate/a63e2fb78899/toolkit/components/console/hudservice/HUDService.jsm#l307 >+ /** >+ * Returns the hudReference for a given id. >+ * >+ * @param string id >+ * @returns Object >+ */ Please have more verbose documentation (I know this is a problem that plagues this file; we are trying to improve it). > /** >+ * Merge the attributes of the two nodes that are about to be filtered. >+ * Increment the number of repeats of aOriginal. >+ * >+ * @param nsIDOMNode aOriginal >+ * The Original Node. The one being merged into. >+ * @param nsIDOMNode aFiltered >+ * The node being filtered out because it is repeated. >+ * @return nsIDOMNode (aOriginal) >+ */ I bit unclear as to why you return the first argument here, and it's never actually used at any call site. Is it really needed? >+ mergeFilteredMessageNode: >+ function ConsoleUtils_mergeFilteredMessageNode(aOriginal, aFiltered) { note: if you wanted to save space, you can s/ConsoleUtils/CU/ here >+ filterRepeatedCSS: >+ function ConsoleUtils_filterRepeatedCSS(aNode, aOutput, aHUDId) { >+ let hud = HUDService.getHudReferenceById(aHUDId); >+ let description = aNode.childNodes[2].textContent; >+ let location; >+ if (aNode.childNodes[4]) // browser_webconsole_bug_595934_message_categories.js >+ location = aNode.childNodes[4].getAttribute("title"); >+ else >+ location = ""; >+ let dupe = hud.cssNodes[description+location]; nit: space around '+' >+ if (!dupe) { >+ // no matching nodes >+ hud.cssNodes[description+location] = aNode; ditto >+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_585237_line_limit.js >- for (let i = 0; i < 20; i++) { >- console.log("foo"); >- } >+ for (let i = 0; i < 20; i++) >+ console.log("foo #" + i); // must change message to prevent repeats >+ please keep the braces >- for (let i = 0; i < 20; i++) { >- console.log("boo"); >- } >+ for (let i = 0; i < 20; i++) >+ console.log("boo #" + i); // must change message to prevent repeats >+ ditto >+++ b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_611795.js >@@ -0,0 +1,56 @@ >+/* vim:set ts=2 sw=2 sts=2 et: */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Any copyright is dedicated to the Public Domain. >+ * http://creativecommons.org/publicdomain/zero/1.0/ >+ * >+ * Contributor(s): >+ * Rob Campbell <rcampbell@mozilla.com> >+ * >+ * ***** END LICENSE BLOCK ***** */ Should just be https://www.mozilla.org/MPL/boilerplate-1.1/pd-c r=sdwilsh, but you should have dao or gavin take a look at the CSS changes (I'm too rusty to feel comfortable reviewing those right now).

Updated patch with Shawn's nits addressed. Requesting review for CSS portion.

The childNodes[MAGICNUMBER] stuff that this patch adds in several places still seems fragile. getHudReferenceById seems unnecessary given that it's only caller does not null check. The object hierarchy that makes it necessary to add getHudReferenceForOutputNode is really broken. File a bug on fixing it, blocking bug 592463 (if one doesn't already exist)?

Comment on attachment 513560 [details] [diff] [review] repeats denitted >+.webconsole-msg-repeat { >+ font: bold 10px sans-serif; >+ font: bold 10px Helvetica; >+ font: bold 10px Arial; Why are these all different? These fonts don't seem to match what's used anywhere else, so I'm not sure why this is needed now (or only on these nodes). >diff --git a/toolkit/themes/gnomestripe/global/webConsole.css b/toolkit/themes/gnomestripe/global/webConsole.css >+.webconsole-msg-repeat { >+ margin-top: 2px; >+ margin-bottom: 2px; margin: 2px 0; ? >+.webconsole-msg-repeat[value="1"] { >+ display: none; >+} This looks like it belongs in a content/ stylesheet.

(In reply to comment #43) > The childNodes[MAGICNUMBER] stuff that this patch adds in several places still > seems fragile. getHudReferenceById seems unnecessary given that it's only > caller does not null check. I dunno. I've done childNodes.getElementsByClassName(x)[0] and that adds a fair bit of overhead to the node lookup. If they're of a particular class (e.g., a css message node), they're guaranteed to have those nodes at those indexes. I could wrapper the lookup inside a more explicit "getDescriptionForNode()" method, but that just seems to add more indirection. What about just commenting the lookups to explain what's in/expected each node? > The object hierarchy that makes it necessary to add > getHudReferenceForOutputNode is really broken. File a bug on fixing it, > blocking bug 592463 (if one doesn't already exist)? Yes it is. I'll dig through and file it if none exists.

(In reply to comment #44) > Comment on attachment 513560 [details] [diff] [review] > repeats denitted > > >+.webconsole-msg-repeat { > > >+ font: bold 10px sans-serif; > >+ font: bold 10px Helvetica; > >+ font: bold 10px Arial; > > Why are these all different? These fonts don't seem to match what's used > anywhere else, so I'm not sure why this is needed now (or only on these nodes). I thought having sans serif fonts looked better in these nodes than the default monospaced font in this case. > >diff --git a/toolkit/themes/gnomestripe/global/webConsole.css b/toolkit/themes/gnomestripe/global/webConsole.css > > >+.webconsole-msg-repeat { > >+ margin-top: 2px; > >+ margin-bottom: 2px; > > margin: 2px 0; ? margin: 2px 0 2px; ? > >+.webconsole-msg-repeat[value="1"] { > >+ display: none; > >+} > > This looks like it belongs in a content/ stylesheet. I'll file a follow up to pull out the functional bits from these style sheets and move them into content.

cleaned up patch based on gavin's review comments. Added a followup bug in bug 635359 to address the functional css bits. still checking for the additional bug mentioned to clean up the object structure around hudReferences...

> > >+.webconsole-msg-repeat { > > >+ margin-top: 2px; > > >+ margin-bottom: 2px; > > > > margin: 2px 0; ? > margin: 2px 0 2px; ? I'm pretty sure margin: a b; is the same as margin: a b a;

(In reply to comment #48) > > > >+.webconsole-msg-repeat { > > > >+ margin-top: 2px; > > > >+ margin-bottom: 2px; > > > > > > margin: 2px 0; ? > > margin: 2px 0 2px; ? > > I'm pretty sure margin: a b; is the same as margin: a b a; yup. I had it confirmed (from multiple sources!) in channel after asking. Thanks though. :)

Comment on attachment 513603 [details] [diff] [review] repeats redenitted Could you take a look at the CSS, Dao?

(In reply to comment #46) > > >+.webconsole-msg-repeat { > > > > >+ font: bold 10px sans-serif; > > >+ font: bold 10px Helvetica; > > >+ font: bold 10px Arial; > > > > Why are these all different? These fonts don't seem to match what's used > > anywhere else, so I'm not sure why this is needed now (or only on these nodes). > > I thought having sans serif fonts looked better in these nodes than the default > monospaced font in this case. You should use font: message-box; then.

Comment on attachment 513603 [details] [diff] [review] repeats redenitted >+ let repeatNode = aDocument.createElementNS(XUL_NS, "xul:label"); What's "xul:" supposed to do here? >+.webconsole-msg-repeat { >+ text-align: center; Are the screenshots up to date? What exactly is getting centered here?

(In reply to comment #52) > Comment on attachment 513603 [details] [diff] [review] > repeats redenitted > > >+ let repeatNode = aDocument.createElementNS(XUL_NS, "xul:label"); > > What's "xul:" supposed to do here? ah, probably nothing. The rest of that method (and other parts of the file) are including it. I did for consistency but realize it's not explicitly required because of the XUL_NS argument. > >+.webconsole-msg-repeat { > > >+ text-align: center; > > Are the screenshots up to date? What exactly is getting centered here? Hopefully the text (number) inside the label. The screenshots are reasonably current although probably off by a pixel or two since their creation. I can recapture if you like.

(In reply to comment #53) > > >+ let repeatNode = aDocument.createElementNS(XUL_NS, "xul:label"); > > > > What's "xul:" supposed to do here? > > ah, probably nothing. The rest of that method (and other parts of the file) are > including it. I did for consistency but realize it's not explicitly required > because of the XUL_NS argument. Even without the namespace argument it wouldn't really make sense, as the script doesn't know what, if anything, 'xul' is associated with in the host document. In fact, browser.xul doesn't seem to set xmlns:xul. I think it might be a no-op regardless of what namespaces exist in the document... > > Are the screenshots up to date? What exactly is getting centered here? > > Hopefully the text (number) inside the label. The screenshots are reasonably > current although probably off by a pixel or two since their creation. I can > recapture if you like. It looks like the label's width adapts to the value, in which case the value couldn't be aligned in a meaningful way.

just an fyi to let you know what I'm up to, I'm going to remove the text-align rule and update the screenshots. I'd remove the xul: prefixes from the labels but that'd make me want to fix them all over this file. It's only 12 occurrences, but I think we should do that in a follow-up attached to our meta-cleanup bug 592463. Patches and screenshots to follow.

filed bug 636538 for the xul: cleanup.

another version. Fixed up the merge conflicts in HUDService.jsm and applied Dao's changes to the CSS files. screenshots forthcoming.

font: message-box; looks bad without any modifications. It's too large for the line.

with font: message-box on mac.

reverted font settings to sans, Arial and Helvetica.

Comment on attachment 514906 [details] [diff] [review] repeats v11 (In reply to comment #58) > font: message-box; looks bad without any modifications. It's too large for the > line. Set a font-size? >+.webconsole-msg-repeat { >+ display: block; Why display: block?

/* Repeated messages */ .webconsole-msg-repeat { margin: 2px 0; padding-left: 4px; padding-right: 4px; width: 1em; color: white; background-color: red; border-radius: 40px; font: message-box; font-size: 10px; font-weight: 600; }

replaced font declaration with message-box, added 10px and 600 weight. The "display: block" rule was set to offset the "display: none" rule in the functional piece. It was unnecessary but I thought didn't hurt. Since it's in xul, it's probably inheriting a display of -moz-block or similar though. IN any case, I've removed it.

with above CSS. Looks like it could use 1px padding on the bottom.

It looks like it shouldn't have the 1px padding on the top.

Removed 1px padding-top from windows and linux. Thanks for the R+, Dao!

Comment on attachment 515138 [details] [diff] [review] [checked-in] repeats v13 http://hg.mozilla.org/mozilla-central/rev/c60c4769c89a

Verified all platforms

(In reply to Kevin Dangoor from comment #3) > Really? I don't think I've personally found the circled number confusing in > the slightest. *blush* I did. I didn't know what to do with the "red balls". :-) So why not mention it there: https://developer.mozilla.org/en/Using_the_Web_Console

(In reply to Daniel Kabs, reporting bugs since 2002 from comment #72) > (In reply to Kevin Dangoor from comment #3) > > Really? I don't think I've personally found the circled number confusing in > > the slightest. > > *blush* I did. I didn't know what to do with the "red balls". :-) Err, yeah I didn't phrase that well, because it's not unreasonable for people to wonder about those... > So why not mention it there: > https://developer.mozilla.org/en/Using_the_Web_Console Behold! sheppy has now done so!

Can I just say that I find this feature quite annoying -- can it be a config option?

(In reply to James Edwards from comment #74) > Can I just say that I find this feature quite annoying -- can it be a config > option? Please file a bug. Thank you!

(In reply to Mihai Sucan [:msucan] from comment #75) > (In reply to James Edwards from comment #74) > > Can I just say that I find this feature quite annoying -- can it be a config > > option? > > Please file a bug. Thank you! And perhaps that bug could depend on bug 851546.

FYI, I created a bug to "disable" this feature on https://bugzilla.mozilla.org/show_bug.cgi?id=1048287