[webkit-reviews] review denied: [Bug 30324] Console shows no repeat count when repeatedly logging an Event : [Attachment 41544] proposed patch 2

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Keishi Hattori
<casey.hattori at gmail.com>'s request for review:
Bug 30324: Console shows no repeat count when repeatedly logging an Event
https://bugs.webkit.org/show_bug.cgi?id=30324

Attachment 41544: proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=41544&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
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!