Bug 37274 – Report resource URL and error details in resource events

• Component: Platform
• Status: RESOLVED FIXED

[Andrey Kosyakov]

When resource loader reports resource events, the following methods return unexpected values:

- ResourceError::failingURL() is always empty
- ResourceResponse::httpStatusText() is always "OK"

[Andrey Kosyakov]

Created attachment 52866 [details]
Fixed ResourceError::failingURL() and ResourceResponse::httpStatusText() to return meaningful values

[Andrey Kosyakov]

Comment on attachment 52866 [details]
Fixed ResourceError::failingURL() and ResourceResponse::httpStatusText() to return meaningful values

One of changed test expectations was accidently missing from patch, about to commit another one in a few minutes.

[Andrey Kosyakov]

Created attachment 52868 [details]
Fixed ResourceError::failingURL() and ResourceResponse::httpStatusText() to return meaningful values

[Darin Adler]

Comment on attachment 52868 [details]
Fixed ResourceError::failingURL() and ResourceResponse::httpStatusText() to return meaningful values

> +    m_failingURL = failingURLString; 

Good fix. Where's the test case for this, though? It's separate from the other fix. We don't take bug fixes without test cases.

> -        // FIXME: it would be nice to have a way to get the real status text eventually.
> -        m_httpStatusText = "OK";
> +        // we used to return "OK" for everything, so be compatible and return "OK" for 200.
> +        m_httpStatusText = m_httpStatusCode == 200 ? String("OK") 
> +                                                   : String([NSHTTPURLResponse localizedStringForStatusCode: m_httpStatusCode]);

Does the localizedStringForStatusCode method exist on all platforms we build on? Tiger?

Formatting here is wrong -- we normally don't indent the second line of a conditional expression like this. We just indent by a single tab stop.

What does [NSHTTPURLResponse localizedStringForStatusCode:] return for 200? Do we really need the hard-coded string?

[Andrey Kosyakov]

(In reply to comment #4)
> (From update of attachment 52868 [details])
> > +    m_failingURL = failingURLString; 
> 
> Good fix. Where's the test case for this, though? It's separate from the other
> fix. We don't take bug fixes without test cases.

There's a bit of a problem with testing for this -- currently, resource notification related code in DumpRenderTree is platform-specific (DumpRenderTree/mac/ResourceLoadDelegate.mm) and uses native platform classes (NSError & co) only -- the classes that I changed are not used at all. Adding tests for them at this level would require implementing tests explicitly for different platforms, plus passing additional flags via LayoutTestsController. A much cheaper and practical solution would be to test it on a higher level -- e.g. in inspector tests, as my initial motivation for fixing this was to enable logging of messages to inspector console in case of resource loading problems. This will also cover all platforms at once. This requires, however, that the patch for logging resource errors to inspector is landed (http://webkit.org/b/37215), so I added the test there. In will only pass once both this and 37215 are landed, though.
 
> > -        // FIXME: it would be nice to have a way to get the real status text eventually.
> > -        m_httpStatusText = "OK";
> > +        // we used to return "OK" for everything, so be compatible and return "OK" for 200.
> > +        m_httpStatusText = m_httpStatusCode == 200 ? String("OK") 
> > +                                                   : String([NSHTTPURLResponse localizedStringForStatusCode: m_httpStatusCode]);
> 
> Does the localizedStringForStatusCode method exist on all platforms we build
> on? Tiger?

At least the manual says so (http://developer.apple.com/mac/library/documentation/Cocoa/Reference/Foundation/Classes/NSHTTPURLResponse_Class/Reference/Reference.html):

Return Value
A localized string suitable for displaying to users that describes the specified status code.

Availability
Available in Mac OS X v10.2 with Safari 1.0 installed.
Available in Mac OS X v10.2.7 and later.

> 
> Formatting here is wrong -- we normally don't indent the second line of a
> conditional expression like this. We just indent by a single tab stop.

Fixed!

> What does [NSHTTPURLResponse localizedStringForStatusCode:] return for 200? Do
> we really need the hard-coded string?

localizedStringForStatusCode returns "no error". I don't have an evidence that we really need this hack (except for a handful of tests that would need to be re-baselined as a result of this), but considering this value is made available to JS via XMLHttpRequest.statusText, I'd rather be on a safe side and leave it as it used to be in cases where it doesn't contradict numeric value (especially given that OK is what HTTP/1.1 suggests for 200).

[Andrey Kosyakov]

Created attachment 52967 [details]
Fixed ResourceError::failingURL() and ResourceResponse::httpStatusText() to return meaningful values (style fixed)

[WebKit Commit Bot]

Comment on attachment 52967 [details]
Fixed ResourceError::failingURL() and ResourceResponse::httpStatusText() to return meaningful values (style fixed)

Clearing flags on attachment: 52967

Committed r57351: <http://trac.webkit.org/changeset/57351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

I don't think it's a good idea to synthesize status text - if someone actually needs to look at it, they won't appreciate us lying.

Fortunately, there was a way to fix this long-standing issue, see bug 24572.