[webkit-reviews] review denied: [Bug 37215] Web Inspector: be more explicit about resource loading errors : [Attachment 52752] Log error message to inspector console if a resource fails to load. Disable checking of mime-type consistency for failed resource.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 37215: Web Inspector: be more explicit about resource loading errors
https://bugs.webkit.org/show_bug.cgi?id=37215

Attachment 52752: Log error message to inspector console if a resource fails to
load. Disable checking of mime-type consistency for failed resource. 
https://bugs.webkit.org/attachment.cgi?id=52752&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +    String message = "Failed to load resource " + url;
> +    if (status)
> +	   message += String::format(", status: %d", status);
> +
> +    addMessageToConsole(OtherMessageSource, LogMessageType,
ErrorMessageLevel, message, 0, url);

I think it might be best to do this in the front-end, then the message can be
localized. But that isn't as important.

The message could be clearer, something like:

     Failed to load resource. The server responded with a status of %u.

Consider using format for both variations of the message instead of appending
strings. I don't think you need to include the URL in the message since the URL
is automatically included as meta data and shown as a link with the message.

> +    void reportResourceError(const String& url, int status = 0);

Status should be unsigned.

> +	   // If status is an error, content is likely to be of an inconsistent
type,
> +	   // as it's going to be an error message. We do not want to emit a
warning
> +	   // for this, though, as this will already be reported as resource
loading
> +	   // failure.

It is unfornanate to have a widow in comments like this. It is less annoying to
have longer lines than to have widows, IMHO.

> -	   resource.warnings = 0;
> -	   resource.errors = 0;

Why is this needed?