Bug 115863

Created attachment 201248 [details] Patch Proposed patch

Comment on attachment 201248 [details] Patch rs=me

Comment on attachment 201248 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=201248&action=review > LayoutTests/ChangeLog:21 > + * http/tests/websocket/tests/hybi/url-with-empty-query-expected.txt: Removed. > + * http/tests/websocket/tests/hybi/url-with-empty-query.html: Removed. > + * http/tests/websocket/tests/hybi/url-with-query-expected.txt: Removed. > + * http/tests/websocket/tests/hybi/url-with-query.html: Removed. Wait, why is this not supported? This has nothing to do with compression, please explain.

The commit-queue encountered the following flaky tests while processing attachment 201248 [details]: media/track/track-remove-crash.html bug 115892 (author: eric.carlson@apple.com) The commit-queue is continuing to process your patch.

(In reply to comment #3) > (From update of attachment 201248 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=201248&action=review > > > LayoutTests/ChangeLog:21 > > + * http/tests/websocket/tests/hybi/url-with-empty-query-expected.txt: Removed. > > + * http/tests/websocket/tests/hybi/url-with-empty-query.html: Removed. > > + * http/tests/websocket/tests/hybi/url-with-query-expected.txt: Removed. > > + * http/tests/websocket/tests/hybi/url-with-query.html: Removed. > > Wait, why is this not supported? This has nothing to do with compression, please explain. Blink's developers deleted them because they failed after pywebsocket update. I have just researched and the tests were restored in a later commit [1]. I need to figure out what changes are necessary to make them work properly (they still does not work here). [1] https://code.google.com/p/chromium/issues/detail?id=234541

Created attachment 201413 [details] Patch Fix url-with-empty-query.html and url-with-query.html instead of removing them. The fix came from the patch in https://code.google.com/p/chromium/issues/detail?id=234541 . The fix does some changes that I think are not completely necessary, but for the sake of future updates coming from Blink I did not remove them.

(In reply to comment #5) > (In reply to comment #3) > > Wait, why is this not supported? This has nothing to do with compression, please explain. > > Blink's developers deleted them because they failed after pywebsocket update. I have just researched and the tests were restored in a later commit [1]. I need to figure out what changes are necessary to make them work properly (they still does not work here). Yes. They were originally testing the Sec-WebSocket-Location header of HyBi 00 that no longer exists in RFC 6455. As side effect, they were testing that the string passed to the WebSocket constructor as url argument are correctly parsed (in order to build the Sec-WebSocket-Location header). To keep this code path tested for RFC 6455 protocol, too, we were setting the ws_location attribute of _StandaloneRequest object using one manually built from the host header and the resource inside the _wsh.py file. Since Chromium no longer supports HyBi 00, we've cleaned up these Sec-WebSocket-Location related files and code, and added back some new layout test items that directly check the host header and the resource. http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/websocket/tests/hybi/echo-host_wsh.py http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/websocket/tests/hybi/echo-path_wsh.py

(In reply to comment #7) > (In reply to comment #5) > > (In reply to comment #3) > > > Wait, why is this not supported? This has nothing to do with compression, please explain. > > > > Blink's developers deleted them because they failed after pywebsocket update. I have just researched and the tests were restored in a later commit [1]. I need to figure out what changes are necessary to make them work properly (they still does not work here). > > Yes. They were originally testing the Sec-WebSocket-Location header of HyBi 00 that no longer exists in RFC 6455. As side effect, they were testing that the string passed to the WebSocket constructor as url argument are correctly parsed (in order to build the Sec-WebSocket-Location header). To keep this code path tested for RFC 6455 protocol, too, we were setting the ws_location attribute of _StandaloneRequest object using one manually built from the host header and the resource inside the _wsh.py file. Thank you for the explanation. > Since Chromium no longer supports HyBi 00, we've cleaned up these Sec-WebSocket-Location related files and code, and added back some new layout test items that directly check the host header and the resource. The remove of support for HyBi 00 was done before the WebKit fork (or Blink was created), correct? The last mention of Sec-WebSocket-Location in WebKit is from last year by what I can see in git's log.

Comment on attachment 201413 [details] Patch rs=me

Comment on attachment 201413 [details] Patch Clearing flags on attachment: 201413 Committed r150225: <http://trac.webkit.org/changeset/150225>

(In reply to comment #8) > The remove of support for HyBi 00 was done before the WebKit fork (or Blink was created), correct? The last mention of Sec-WebSocket-Location in WebKit is from last year by what I can see in git's log. Right. We added a flag to select the protocol version in July 2011 and stopped supporting HyBi 00 for Chromium. http://trac.webkit.org/browser/trunk/Source/WebCore/websockets/WebSocketChannel.cpp?rev=90704 and completely removed HyBi 00 code in this change made in July 2012. http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/websockets/WebSocketChannel.cpp?rev=122199