Bug 30324 – Console shows no repeat count when repeatedly logging an Event

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

[Keishi Hattori]

Created attachment 41088 [details]
test-case

Console shows no repeat count when repeatedly logging an Event. Logging with ConsoleCommandResult however shows the proper repeat count.

Aside from this bug, I think treating different Event objects as a repeat is wrong.

[Keishi Hattori]

Created attachment 41509 [details]
proposed patch

WebInspector.ConsoleMessage.prototype.isEqual was returning true, even when ConsoleMessage::isEqual would return false.

[Darin Adler]

Comment on attachment 41509 [details]
proposed patch

Shouldn't this be >= rather than > ?

[Keishi Hattori]

Created attachment 41514 [details]
test case that fails if I use >= instead of >

This test case fails if I use >= instead of >.

If ConsoleMessage::isEqual() returns true the repeatCount is incremented so 
  this.repeatCount > msg.repeatCount
will always be satisfied.

[Pavel Feldman]

Comment on attachment 41509 [details]
proposed patch

>              && (this.url == msg.url)
> -            && (this.message == msg.message);
> +            && (this.message == msg.message)
> +            && (this.repeatCount > msg.repeatCount);

This does not look right: messages are equal when their properties are equal, whereas this check should be made external. We are already handle isEqual in InspectorController, I don't think we should do it in the frontend as well. They way I would solve it is following: Split InspectorFrontend::addMessageToConsole into InspectorFrontend::addConsoleMessage(msg) and InspectorFrontend::updateConsoleMessageCount(count). Call latter when you increment count from inspector controller; Nuke this isEqual you are modifying as a whole.

[Keishi Hattori]

Created attachment 41544 [details]
proposed patch 2

I thinks this is much better. I hope I didn't break anything.

[Pavel Feldman]

Comment on attachment 41544 [details]
proposed patch 2

Thanks for doing this!

> +        messagesElement.removeChild(messagesElement.lastChild);
> +        messagesElement.appendChild(msg.toMessageElement());

Nit: There is no need to re-create a message, you could provide updaeRepeatCount on ConsoleMessage that would either update the textContent of the messageRepeatCountElement or would lazily create it. But this can be a part of another bug really.

> +        // Increment the error or warning count
> +        switch (msg.level) {
> +        case WebInspector.ConsoleMessage.MessageLevel.Warning:
> +            WebInspector.warnings += msg.repeatDelta;
> +            break;
> +        case WebInspector.ConsoleMessage.MessageLevel.Error:
> +            WebInspector.errors += msg.repeatDelta;
> +            break;
> +        }
> +        
> +        // Add message to the resource panel
> +        if (msg.url in WebInspector.resourceURLMap) {
> +            msg.resource = WebInspector.resourceURLMap[msg.url];
> +            if (WebInspector.panels.resources)
> +                WebInspector.panels.resources.addMessageToResource(msg.resource, msg);
> +        }
> +    },
> +

This looks like a copy-paste from addMessage, I would try to avoid that. First part with counters makes sense to me, second really does not. We know that updateRepeatCounter is invoked on identical message, there is nowhere it could get its url from. It is unlikely that the resource is added after the message (if it is so - we are in trouble with non-repeating messages). So I suggest removing the url-related part from here and extracting warning+errors updates code into a private method. Otherwise r+.

Please make sure WebKitTools/Scripts/run-webkit-tests --debug LayoutTests/inspector are passing!

[Keishi Hattori]

Created attachment 41789 [details]
proposed patch 3

Was more trouble than I thought. Had to change quite a lot to handle case where ConsoleCommand comes between the repeated messages.

[Pavel Feldman]

Comment on attachment 41789 [details]
proposed patch 3

Looks good. Couple of nits, otherwise r+.

> +            msgCopy = new WebInspector.ConsoleMessage(msg.source, msg.type, msg.level, msg.line, msg.url, msg.groupLevel, count - prevRepeatCount);

You should either use this.repeatCountBeforeCommand here or remove it since it is no more used.

> +    incrementErrorWarningCount: function(msg) {

Please use _incrementErrorWarningCount as this is private method.


> +    updateRepeatCount: function() {

Also private


> -    toMessageElement: function()
> +    get element()

(No need to fix): Why rename? Since this code below is kinda ugly:

> +        this._element = WebInspector.ConsoleMessage.prototype.__lookupGetter__("element").call(this);

[Keishi Hattori]

Created attachment 41806 [details]
proposed patch 4

>You should either use this.repeatCountBeforeCommand here or remove it since it is no more used.
Removed.

[Timothy Hatcher]

You should try run manual-tests/inspector/multiple-console-messages.html and the other console releated manual tests in that directory.

More automated tests for repeated messages would be good.

[Keishi Hattori]

Seems to pass the manual tests so I'm trying to add an automated test.

I want to execute a command in the console so I tried something like this but it doesn't seem to give me the results I'm looking for. Is there a proper way to do this?

https://gist.github.com/75074e73fa7d2bb0bec3

[Pavel Feldman]

(In reply to comment #11)
> Seems to pass the manual tests so I'm trying to add an automated test.
> 
> I want to execute a command in the console so I tried something like this but
> it doesn't seem to give me the results I'm looking for. Is there a proper way
> to do this?
> 
> https://gist.github.com/75074e73fa7d2bb0bec3

You don't need to eval console.log in console - evaluating 1+1 would be enough to cause the interrupting output. What you should do is:

1. introduce a function that does console.log call
2. call it in doIt
3. evaluate dumpMessages in frontend
4. make dumpMessages issue some console command as you do
5. in the callback to dumpMessages call your function containting console.log again
6. dump messages again to see if interrupt has been processed well.

[WebKit Commit Bot]

Comment on attachment 41806 [details]
proposed patch 4

Clearing flags on attachment: 41806

Committed r50064: <http://trac.webkit.org/changeset/50064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.