Bug 72687 - [Qt] Enable WebSocket hybi tests
: [Qt] Enable WebSocket hybi tests
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To:
:
: Qt
:
: 50099
  Show dependency treegraph
 
Reported: 2011-11-17 21:11 PST by
Modified: 2011-11-18 21:00 PST (History)


Attachments
Patch (6.24 KB, patch)
2011-11-17 21:35 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch v2 (don't change public API) (6.76 KB, patch)
2011-11-18 00:16 PST, Yuta Kitamura
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-17 21:11:18 PST
Currently, WebKitQt only supports the old WebSocket protocol (hixie-76). The old protocol is known to have a security defect, and should be phased out into the new protocol.

The new protocol (hybi-17) is already implemented in WebCore and we can switch to the new protocol using WebCore::Settings. In this bug, I'd like to:
- Add an option to QWebSettings (which corresponds to one in WebCore::Settings),
- Let LayoutTestControllerQt be able to switch WebSocket protocols (using overridePreference()),
- Unskip WebSocket hybi tests.
------- Comment #1 From 2011-11-17 21:35:37 PST -------
Created an attachment (id=115735) [details]
Patch
------- Comment #2 From 2011-11-17 21:56:02 PST -------
(In reply to comment #0)
> Currently, WebKitQt only supports the old WebSocket protocol (hixie-76). The old protocol is known to have a security defect, and should be phased out into the new protocol.
> 
> The new protocol (hybi-17) is already implemented in WebCore and we can switch to the new protocol using WebCore::Settings. In this bug, I'd like to:
> - Add an option to QWebSettings (which corresponds to one in WebCore::Settings),
> - Let LayoutTestControllerQt be able to switch WebSocket protocols (using overridePreference()),
> - Unskip WebSocket hybi tests.

QWebSettings is public API, and at that point I'm wondering: Do we want to expose the users of the API to the complexity of choosing the protocol version?

Could we simply make the newer protocol the default? If we need to be able to test both with layout tests, then I think a helper function to toggle this could be added to DumpRenderTreeSupportQt. My guts feeling is that this change could be done without any addition to the public API.
------- Comment #3 From 2011-11-17 22:18:04 PST -------
(In reply to comment #2)
> 
> QWebSettings is public API, and at that point I'm wondering: Do we want to expose the users of the API to the complexity of choosing the protocol version?

Agreed, it shouldn't be exposed to public. Actually, in Apple's Mac/Win ports, this attribute is hidden in WebPreferencePrivate, so we should do the same in Qt port.

> 
> Could we simply make the newer protocol the default? If we need to be able to test both with layout tests, then I think a helper function to toggle this could be added to DumpRenderTreeSupportQt. My guts feeling is that this change could be done without any addition to the public API.

I'd like to make sure the tests are up and running first (actually I found test failures which need to be fixed before switching the default protocols).

I didn't know DumpRenderTreeSupportQt; this seems like a perfect place to put functions that are needed to implement overridePreference(). I'd like to write another patch that implements overridePreference() without changing the public interface.
------- Comment #4 From 2011-11-17 22:19:04 PST -------
(From update of attachment 115735 [details])
I shouldn't have changed the public API.
------- Comment #5 From 2011-11-17 23:55:29 PST -------
(In reply to comment #3)
> (In reply to comment #2)
> > 
> > QWebSettings is public API, and at that point I'm wondering: Do we want to expose the users of the API to the complexity of choosing the protocol version?
> 
> Agreed, it shouldn't be exposed to public. Actually, in Apple's Mac/Win ports, this attribute is hidden in WebPreferencePrivate, so we should do the same in Qt port.
> 
> > 
> > Could we simply make the newer protocol the default? If we need to be able to test both with layout tests, then I think a helper function to toggle this could be added to DumpRenderTreeSupportQt. My guts feeling is that this change could be done without any addition to the public API.
> 
> I'd like to make sure the tests are up and running first (actually I found test failures which need to be fixed before switching the default protocols).
> 
> I didn't know DumpRenderTreeSupportQt; this seems like a perfect place to put functions that are needed to implement overridePreference(). I'd like to write another patch that implements overridePreference() without changing the public interface.

Sounds good. Thanks for taking the Qt port into account. Much appreciated!
------- Comment #6 From 2011-11-18 00:16:09 PST -------
Created an attachment (id=115752) [details]
Patch v2 (don't change public API)
------- Comment #7 From 2011-11-18 01:58:30 PST -------
(From update of attachment 115752 [details])
r=me
------- Comment #8 From 2011-11-18 04:54:25 PST -------
Thanks!
------- Comment #9 From 2011-11-18 06:00:50 PST -------
(From update of attachment 115752 [details])
Clearing flags on attachment: 115752

Committed r100763: <http://trac.webkit.org/changeset/100763>
------- Comment #10 From 2011-11-18 06:00:54 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #11 From 2011-11-18 07:06:15 PST -------
One test in still broken:

--- /ramdisk/qt-linux-release/build/layout-test-results/http/tests/websocket/tests/hybi/workers/close-code-and-reason-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/http/tests/websocket/tests/hybi/workers/close-code-and-reason-actual.txt 
@@ -3,10 +3,6 @@

 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

-ws.onclose() was called.
-PASS PASS: worker: event.wasClean is false
-PASS PASS: worker: event.code is codeAbnormalClosure
-PASS PASS: worker: event.reason is emptyString
 WebSocketTest.onopen() was called with testId = 0.
 WebSocketTest.onclose() was called with testId = 0.
 PASS PASS: worker: event.wasClean is true
------- Comment #12 From 2011-11-18 21:00:04 PST -------
Oops sorry, I'm going to skip that. It should be the same one as I mentioned in the Skipped file in this patch, and I'll investigate this failure sometime next week.