Bug 72687

Created attachment 115735 [details] Patch

(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.

(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 on attachment 115735 [details] Patch I shouldn't have changed the public API.

(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!

Created attachment 115752 [details] Patch v2 (don't change public API)

Comment on attachment 115752 [details] Patch v2 (don't change public API) r=me

Thanks!

Comment on attachment 115752 [details] Patch v2 (don't change public API) Clearing flags on attachment: 115752 Committed r100763: <http://trac.webkit.org/changeset/100763>

All reviewed patches have been landed. Closing bug.

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

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.