Bug 30324

Summary: Console shows no repeat count when repeatedly logging an Event
Product: WebKit Reporter: Keishi Hattori <keishi>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, commit-queue, joepeck, pfeldman, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 19879    
Attachments:
Description Flags
test-case
none
proposed patch
pfeldman: review-
test case that fails if I use >= instead of >
none
proposed patch 2
pfeldman: review-
proposed patch 3
pfeldman: review-
proposed patch 4 none

Description Keishi Hattori 2009-10-12 23:41:09 PDT
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.
Comment 1 Keishi Hattori 2009-10-20 09:50:27 PDT
Created attachment 41509 [details]
proposed patch

WebInspector.ConsoleMessage.prototype.isEqual was returning true, even when ConsoleMessage::isEqual would return false.
Comment 2 Darin Adler 2009-10-20 10:48:14 PDT
Comment on attachment 41509 [details]
proposed patch

Shouldn't this be >= rather than > ?
Comment 3 Keishi Hattori 2009-10-20 11:04:02 PDT
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.
Comment 4 Pavel Feldman 2009-10-20 15:22:58 PDT
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.
Comment 5 Keishi Hattori 2009-10-20 19:54:52 PDT
Created attachment 41544 [details]
proposed patch 2

I thinks this is much better. I hope I didn't break anything.
Comment 6 Pavel Feldman 2009-10-22 07:24:42 PDT
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!
Comment 7 Keishi Hattori 2009-10-24 12:33:27 PDT
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.
Comment 8 Pavel Feldman 2009-10-24 17:48:07 PDT
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);
Comment 9 Keishi Hattori 2009-10-24 20:01:56 PDT
Created attachment 41806 [details]
proposed patch 4

>You should either use this.repeatCountBeforeCommand here or remove it since it is no more used.
Removed.
Comment 10 Timothy Hatcher 2009-10-25 02:43:24 PDT
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.
Comment 11 Keishi Hattori 2009-10-25 05:39:06 PDT
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
Comment 12 Pavel Feldman 2009-10-26 09:06:48 PDT
(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.
Comment 13 WebKit Commit Bot 2009-10-26 09:10:17 PDT
Comment on attachment 41806 [details]
proposed patch 4

Clearing flags on attachment: 41806

Committed r50064: <http://trac.webkit.org/changeset/50064>
Comment 14 WebKit Commit Bot 2009-10-26 09:10:26 PDT
All reviewed patches have been landed.  Closing bug.