[webkit-reviews] review denied: [Bug 50390] Web Inspector: Console records for failed XHRs should contain call stack and request method : [Attachment 75515] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Yury Semikhatsky
<yurys at chromium.org>'s request for review:
Bug 50390: Web Inspector: Console records for failed XHRs should contain call
stack and request method
https://bugs.webkit.org/show_bug.cgi?id=50390

Attachment 75515: Patch
https://bugs.webkit.org/attachment.cgi?id=75515&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75515&action=review

A bunch of nits, no real suggestions. Switch in Switch worries me a lot.

> WebCore/inspector/InspectorController.cpp:866
> +	   addConsoleMessage(new ConsoleMessage(OtherMessageSource,
LogMessageType, ErrorMessageLevel, info.httpMethod, response.url().string(),
response.httpStatusCode(), response.httpStatusText(), info.callStack));

Nit: this should probably be new message type.

> WebCore/inspector/front-end/ConsoleView.js:260
> +	       var msgCopy = new WebInspector.ConsoleMessage(msg.source,
msg.type, msg.level, msg.line, msg.url, count - prevRepeatCount,
msg._messageText, msg._parameters, msg._stackTrace, msg._httpMethod,
msg._httpStatusCode);

Should this be a ConsoleMessage ancestor?

> WebCore/inspector/front-end/ConsoleView.js:692
> +		       case WebInspector.ConsoleMessage.MessageType.Assert:

switch by type in switch by type clearly tells us that there is something wrong
with the abstraction. We should either create hierarchy of messages (like
ConsoleMessageWithStackInfo), or rely upon message types more extensively and
introduce new ones.

> WebCore/inspector/front-end/ConsoleView.js:704
> +			   hideStackTrace = true;

I think there should be an explicit way of telling which messages have stack
traces.

> WebCore/inspector/front-end/inspector.js:1428
> +	   payload.httpMethod,

This will look quite strange in the protocol. It sounds like we should
distinguish console messages from error messages. It sounds more like an error
message bound to resource load rather than generic console message.