Bug 113842 – [Qt] WebSocket errors should be logged to console

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Seokju Kwon]

Pass the reason of a failure to SocketStreamError instance 
and notify it via SocketStreamHandleClient::didFailSocketStream().

[Seokju Kwon]

Created attachment 196269 [details]
Patch

[Seokju Kwon]

*** Bug 113843 has been marked as a duplicate of this bug. ***

[Seokju Kwon]

Created attachment 196274 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 196274 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [QT] WebSocket errors should be logged to console

You don't have to change it this time, but for your knowledge QT == QuickTime, we use Qt with a lower 't' :)

> LayoutTests/platform/qt/http/tests/inspector/console-websocket-error-expected.txt:2
> +CONSOLE MESSAGE: WebSocket network error: error code 2

It's nice to pass the test, but this isn't so much useful for the user.
Would it be possible to pass errorString() to an additional parameter in SocketStreamError implementation like in Source/WebCore/platform/network/soup/SocketStreamError.h?

[Seokju Kwon]

Created attachment 196427 [details]
Patch

[Seokju Kwon]

(In reply to comment #4)
> (From update of attachment 196274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196274&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [QT] WebSocket errors should be logged to console
> 
> You don't have to change it this time, but for your knowledge QT == QuickTime, we use Qt with a lower 't' :)
Ossy : Thanks! :)

> 
> > LayoutTests/platform/qt/http/tests/inspector/console-websocket-error-expected.txt:2
> > +CONSOLE MESSAGE: WebSocket network error: error code 2
> 
> It's nice to pass the test, but this isn't so much useful for the user.
> Would it be possible to pass errorString() to an additional parameter in SocketStreamError implementation like in Source/WebCore/platform/network/soup/SocketStreamError.h?

Jocelyn Turcotte : Thank you for review.
Provide more specific error description for SocketStreamError.

[Seokju Kwon]

Created attachment 196436 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 196436 [details]
Patch

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

A few more nitpicks.

> Source/WebCore/platform/network/qt/SocketStreamError.h:46
> +        SocketStreamError(int errorCode, QString description)

const QString&

> Source/WebCore/platform/network/qt/SocketStreamError.h:47
> +            : SocketStreamErrorBase(errorCode, String(), String(description))

The String(const QString&) consturctor isn't explicit, so you can pass the description parameter straight.

> Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:62
> -    unsigned int port = url.hasPort() ? url.port() : (isSecure ? 443 : 80);
> +    unsigned port = url.hasPort() ? url.port() : (isSecure ? 443 : 80);

We usually don't do cleanups like this unless they have a considerable value to avoid poluting the line history (git blame).

> LayoutTests/platform/qt/http/tests/inspector/console-websocket-error-expected.txt:5
> +CONSOLE MESSAGE: WebSocket network error: The host name did not match any of the valid hosts for this certificate

Looks much better, thanks :)

[Seokju Kwon]

Created attachment 196459 [details]
Patch

[Seokju Kwon]

(In reply to comment #8)
> (From update of attachment 196436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=196436&action=review
> 
> A few more nitpicks.
> 
> > Source/WebCore/platform/network/qt/SocketStreamError.h:46
> > +        SocketStreamError(int errorCode, QString description)
> 
> const QString&
> 
> > Source/WebCore/platform/network/qt/SocketStreamError.h:47
> > +            : SocketStreamErrorBase(errorCode, String(), String(description))
> 
> The String(const QString&) consturctor isn't explicit, so you can pass the description parameter straight.
> 
> > Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:62
> > -    unsigned int port = url.hasPort() ? url.port() : (isSecure ? 443 : 80);
> > +    unsigned port = url.hasPort() ? url.port() : (isSecure ? 443 : 80);
> 
> We usually don't do cleanups like this unless they have a considerable value to avoid poluting the line history (git blame).
> 
> > LayoutTests/platform/qt/http/tests/inspector/console-websocket-error-expected.txt:5
> > +CONSOLE MESSAGE: WebSocket network error: The host name did not match any of the valid hosts for this certificate
> 
> Looks much better, thanks :)

Thanks. All done :)

[WebKit Review Bot]

Comment on attachment 196459 [details]
Patch

Clearing flags on attachment: 196459

Committed r147618: <http://trac.webkit.org/changeset/147618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.