Bug 54226 – [Chromium] WebSocket: Implement WebKit::WebSocketStreamError

• Component: WebKit API
• Status: RESOLVED WONTFIX

[Yuta Kitamura]

Currently, WebKit::WebSocketStreamError is not implemented and there is no way to report errors arising from underlying sockets. 

Need to implement it to show these errors in JavaScript console and let developers debug WebSocket issues easily.

[Yuta Kitamura]

Related bug: bug 40945 (SocketStreamError implementation for Apple port), http://crbug.com/67662

[Yuta Kitamura]

Created attachment 82033 [details]
Patch

[Yuta Kitamura]

FYI, follow-up patch for Chromium: http://codereview.chromium.org/6474012/

The change above should be committed *after* the patch on this bug is merged.

[Fumitoshi Ukai]

Comment on attachment 82033 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82033&action=review

> Source/WebCore/platform/network/chromium/SocketStreamError.h:43
> +    SocketStreamError(int errorCode, const String& failingURL, const String& localizedDescription)

I'm not sure, but localized description should be retrieved via
WebCore/platform/LocalizedStrings.h ?

Do we need failingURL?  I think SocketStreamHandleClient (that is, WebSocketChannel) should know the corresponding URL.

[Yuta Kitamura]

(In reply to comment #4)
> I'm not sure, but localized description should be retrieved via
> WebCore/platform/LocalizedStrings.h ?
> 

As I understand, localizedDescription represents a system error message (such as "Connection was refused" or "Network is unreachable"), which each port has some way to translate (strerror for example).

Errors come from the browser process, so we can translate them inside Chromium, not using WebCore's LocalizedStrings.


> Do we need failingURL?  I think SocketStreamHandleClient (that is, WebSocketChannel) should know the corresponding URL.

No. I'll remove it in another changelist. I'd like to focus this change on Chromium port.

(I just copied failingURL from ResourceError and did not notice it was not necessary)

[Fumitoshi Ukai]

(In reply to comment #5)
> (In reply to comment #4)
> > I'm not sure, but localized description should be retrieved via
> > WebCore/platform/LocalizedStrings.h ?
> > 
> 
> As I understand, localizedDescription represents a system error message (such as "Connection was refused" or "Network is unreachable"), which each port has some way to translate (strerror for example).
> 
> Errors come from the browser process, so we can translate them inside Chromium, not using WebCore's LocalizedStrings.

Then, I think it's better to call it systemErrorMessage.

  
> > Do we need failingURL?  I think SocketStreamHandleClient (that is, WebSocketChannel) should know the corresponding URL.
> 
> No. I'll remove it in another changelist. I'd like to focus this change on Chromium port.

Why not in this changelist?
> 
> (I just copied failingURL from ResourceError and did not notice it was not necessary)

[Darin Fisher (:fishd, Google)]

Comment on attachment 82033 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82033&action=review

> Source/WebKit/chromium/src/WebSocketStreamError.cpp:46
> +    WTF::String errorDescription(m_errorDescription);

should be no need for WTF:: in .cpp file.  please add a 'using namespace WebCore' at the
top so you don't need the WebCore:: prefixes either.

also, i don't think you need this temporary.

> Source/WebKit/chromium/src/WebSocketStreamError.cpp:47
> +    WTF::String failingURL(static_cast<WebCore::KURL>(m_failingURL).string());

instead of casting to KURL, please just construct a KURL.  casts are ugly, but
WebURL provides the casting operator so that you can just construct a KURL from
a WebURL w/o having to make KURL know about WebURL.

come to think about it, perhaps it would be more consistent w/ webkit API
conventions if this function were a casting operator to WebCore::SocketStreamError.
then when using WebSocketStreamError, you could just happily pass those objects to
methods expecting a WebCore::SocketStreamError :)

> Source/WebKit/chromium/src/WebSocketStreamError.cpp:48
> +    return WebCore::SocketStreamError(m_errorCode, failingURL, errorDescription);

i think you should be able to compact to this:

return WebCore::SocketStreamError(m_errorCode, KURL(failingURL).string(), m_errorDescription);

[Yuta Kitamura]

(In reply to comment #6)
> Then, I think it's better to call it systemErrorMessage.

What's systemErrorMessage? I could not find any usage inside WebKit.

> Why not in this changelist?

To lower the reviewer's load. Reviewers who are good at WebCore/websockets (like ap@apple) may not be good at WebKit/chromium, and vice versa.

[Yuta Kitamura]

Created attachment 90317 [details]
Patch v2

[Yuta Kitamura]

I think I addressed all issues raised by Darin.

In terms of failingURL, we have decided not to remove it (see bug 58765).

[Adam Barth]

Comment on attachment 90317 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=90317&action=review

This looks great, but we need fishd for WebKit API approval.

> Source/WebKit/chromium/public/WebSocketStreamError.h:55
> +    WebSocketStreamError()
> +        : m_isNull(true), m_errorCode(0) { }
> +    WebSocketStreamError(int errorCode, const WebString& errorDescription, const WebURL& failingURL)
> +        : m_isNull(false), m_errorCode(errorCode), m_errorDescription(errorDescription), m_failingURL(failingURL) { }

Each initializer should be on it's own line.

> Source/WebKit/chromium/src/WebSocketStreamError.cpp:48
> +    return SocketStreamError(m_errorCode, KURL(m_failingURL).string(), m_errorDescription);

Is KURL(m_failingURL).string() the right pattern here?

[Adam Barth]

(In reply to comment #11)
> (From update of attachment 90317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90317&action=review
>
> > Source/WebKit/chromium/src/WebSocketStreamError.cpp:48
> > +    return SocketStreamError(m_errorCode, KURL(m_failingURL).string(), m_errorDescription);
> 
> Is KURL(m_failingURL).string() the right pattern here?

Looks like it is the right pattern, according to fishd.

[Adam Barth]

+fishd for WebKit API approval.

[Yuta Kitamura]

Created attachment 92558 [details]
Patch v3

[Eric Seidel (no email)]

Has this really been pending API approval for 8 months? :p