Bug 113842 - [Qt] WebSocket errors should be logged to console
Summary: [Qt] WebSocket errors should be logged to console
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 113843 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-04-02 20:09 PDT by Seokju Kwon
Modified: 2013-04-04 03:50 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.93 KB, patch)
2013-04-02 20:12 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (4.91 KB, patch)
2013-04-02 21:15 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2013-04-03 18:13 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (6.04 KB, patch)
2013-04-03 21:58 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff
Patch (5.72 KB, patch)
2013-04-04 02:32 PDT, Seokju Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Seokju Kwon 2013-04-02 20:09:42 PDT
Pass the reason of a failure to SocketStreamError instance 
and notify it via SocketStreamHandleClient::didFailSocketStream().
Comment 1 Seokju Kwon 2013-04-02 20:12:16 PDT
Created attachment 196269 [details]
Patch
Comment 2 Seokju Kwon 2013-04-02 20:20:33 PDT
*** Bug 113843 has been marked as a duplicate of this bug. ***
Comment 3 Seokju Kwon 2013-04-02 21:15:49 PDT
Created attachment 196274 [details]
Patch
Comment 4 Jocelyn Turcotte 2013-04-03 04:34:09 PDT
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?
Comment 5 Seokju Kwon 2013-04-03 18:13:55 PDT
Created attachment 196427 [details]
Patch
Comment 6 Seokju Kwon 2013-04-03 18:20:11 PDT
(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.
Comment 7 Seokju Kwon 2013-04-03 21:58:52 PDT
Created attachment 196436 [details]
Patch
Comment 8 Jocelyn Turcotte 2013-04-04 02:23:27 PDT
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 :)
Comment 9 Seokju Kwon 2013-04-04 02:32:07 PDT
Created attachment 196459 [details]
Patch
Comment 10 Seokju Kwon 2013-04-04 02:33:21 PDT
(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 :)
Comment 11 WebKit Review Bot 2013-04-04 03:50:33 PDT
Comment on attachment 196459 [details]
Patch

Clearing flags on attachment: 196459

Committed r147618: <http://trac.webkit.org/changeset/147618>
Comment 12 WebKit Review Bot 2013-04-04 03:50:38 PDT
All reviewed patches have been landed.  Closing bug.