Bug 115863 - [WebSocket] Update pywebsocket to r760
Summary: [WebSocket] Update pywebsocket to r760
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 98840
  Show dependency treegraph
 
Reported: 2013-05-09 09:45 PDT by Lamarque V. Souza
Modified: 2013-05-19 19:18 PDT (History)
6 users (show)

See Also:


Attachments
Patch (154.47 KB, patch)
2013-05-09 10:18 PDT, Lamarque V. Souza
ap: review+
Details | Formatted Diff | Diff
Patch (158.53 KB, patch)
2013-05-10 13:32 PDT, Lamarque V. Souza
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lamarque V. Souza 2013-05-09 09:45:24 PDT
This pywebsocket version supports draft-ietf-hybi-permessage-compression-08. I am going to port the patch in https://code.google.com/p/chromium/issues/detail?id=229290 to WebKit. I plan to also port the permessage-deflate extension from there.
Comment 1 Lamarque V. Souza 2013-05-09 10:18:27 PDT
Created attachment 201248 [details]
Patch

Proposed patch
Comment 2 Alexey Proskuryakov 2013-05-09 22:17:58 PDT
Comment on attachment 201248 [details]
Patch

rs=me
Comment 3 Alexey Proskuryakov 2013-05-09 22:19:27 PDT
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.
Comment 4 WebKit Commit Bot 2013-05-09 22:49:59 PDT
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.
Comment 5 Lamarque V. Souza 2013-05-10 06:01:05 PDT
(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
Comment 6 Lamarque V. Souza 2013-05-10 13:32:06 PDT
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.
Comment 7 Takeshi Yoshino 2013-05-14 21:56:20 PDT
(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
Comment 8 Lamarque V. Souza 2013-05-15 17:06:19 PDT
(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 9 Alexey Proskuryakov 2013-05-15 18:52:34 PDT
Comment on attachment 201413 [details]
Patch

rs=me
Comment 10 WebKit Commit Bot 2013-05-16 17:33:26 PDT
Comment on attachment 201413 [details]
Patch

Clearing flags on attachment: 201413

Committed r150225: <http://trac.webkit.org/changeset/150225>
Comment 11 Takeshi Yoshino 2013-05-19 19:18:35 PDT
(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