Bug 20949 – Catch repeated messages in Inspector Controller to limit memory usage

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Kevin McCullough]

The Web Inspector captures repeated messages and collapses them, but this could be done in the Inspector Controller, and would reduce the memory usage even more since then we would not be keeping each message in native code.

[Kevin McCullough]

Created attachment 23586 [details]
patch

[Oliver Hunt]

Comment on attachment 23586 [details]
patch

static ConsoleMessage* previousMessage = 0;

Is unsafe as it is global whereas InspectorController is per WebView (afaiaa) so you could have to webviews spewing the same error and you would get excitingly confused.

I'm also not sure what happens if i had a case where i had a repeating group of messages -- say trying to load malformed content or some such could lead to repeating groups of messages, but because it's not the same message over and over in immediate succession it would not be caught.

[Kevin McCullough]

Yes good catch on the static, I'll change that.

wrt the repeating groups we currently have no plans to try and detect that.  To do that you need to answer questions like, how big is a group? and how do you represent their repetition?  I think for the vast majority of the cases, this is the right solution.

[Kevin McCullough]

Created attachment 23668 [details]
new patch with test

Fixes Ollie's comments and some bugs I had when adding repeated messages dynamically (as opposed to just the ones in the InspectorController

[Oliver Hunt]

Comment on attachment 23668 [details]
new patch with test

r=me

[Timothy Hatcher]

Make sure you add m_previousMessage = 0; in clearConsoleMessages.

[Timothy Hatcher]

Please fix the _resourceMessage_resourceMessageRepeatCountElement property name.

It should be: _resourceMessageRepeatCountElement

Also it seems odd that msg.repeatCount is sometimes the difference, if I understand correctly.

[Timothy Hatcher]

Comment on attachment 23668 [details]
new patch with test

+            var previousMessageElement = this.currentGroup.messagesElement;
+            previousMessageElement.removeChild(previousMessageElement.lastChild);
+            previousMessageElement.appendChild(msg.toMessageElement());

previousMessageElement is not the right name for that variable, messagesElement is a better name.

+            msg.repeatCount -= this.previousMessage.repeatCount;

This line deserves a comment.

Even better would be to assign the difference of the repeat count to a new property on the msg, so clients know they are getting the difference. Better yet, just store it in a loval variable, and pass that number to addMessageToResource. In addMessageToResource if the number is > 0 then do the repeat path.

-            msg.resource = WebInspector.resourceURLMap[msg.url];
+            msg.resource = WebInspector.resourceURLMap[msg.url]; 

Don't land the extra space at the end of that line.

r- until some of these are fixed.

[Kevin McCullough]

Created attachment 23673 [details]
new patch with changes

[Kevin McCullough]

Committed revision 36809.

[Kevin McCullough]

my bad.  Wrong bug

[Kevin McCullough]

Committed revision 36817.