Bug 80650 – [WebSocket] Add a layout test where server force to disable all extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Takashi Toyoshima]

Currently, WebSocket extensions which WebKit implement are all enabled by default.
But this enabled extension list could be changed when WebKit support other extensions or rename existing extensions.
It means that the expectation of a test which verify extensions property depends on WebKit.
This is not good for embedder's unit tests.

So, I'd like to add a layout test where server pyewebsocket just omit all extensions and the extensions property returns empty string.

[Takashi Toyoshima]

Created attachment 130937 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 130937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130937&action=review

> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension.html:25
> +    shouldBeEqualToString("ws.extensions", "x-webkit-deflate-frame");

shouldBeEqualToString("ws.extensions", ""); ?

[Takashi Toyoshima]

Comment on attachment 130937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130937&action=review

>> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension.html:25
>> +    shouldBeEqualToString("ws.extensions", "x-webkit-deflate-frame");
> 
> shouldBeEqualToString("ws.extensions", ""); ?

Oops, right.
I mistakenly overwrite the expectation.

[Takashi Toyoshima]

Created attachment 130940 [details]
Patch

[Kent Tamura]

Comment on attachment 130940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130940&action=review

> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension-expected.txt:7
> +PASS ws.extensions is ""
> +PASS ws.extensions is ""
> +PASS ws.extensions is ""

It's hard to understand what are tested here with this result. At least we should show:

Check the value before opening the connection:
PASS ws.extensions is ""
PASS ws.extensions = "foo"; ws.extensions is ""
Check the value after opening the connection:
PASS ws.extensions is ""

> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension.html:21
> +ws.extensions = "foo";
> +shouldBeEqualToString("ws.extensions", "");

I prefer shouldBeEqualToString("ws.extensions = 'foo'; ws.extensions", "");

> LayoutTests/http/tests/websocket/tests/hybi/extensions-expected.txt:7
>  PASS ws.extensions is ""
>  PASS ws.extensions is ""
> +PASS ws.extensions is "x-webkit-deflate-frame"

I'd like to make the same comment with echo-with-no-extension-expected.txt.

[Takashi Toyoshima]

Comment on attachment 130940 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130940&action=review

>> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension-expected.txt:7
>> +PASS ws.extensions is ""
> 
> It's hard to understand what are tested here with this result. At least we should show:
> 
> Check the value before opening the connection:
> PASS ws.extensions is ""
> PASS ws.extensions = "foo"; ws.extensions is ""
> Check the value after opening the connection:
> PASS ws.extensions is ""

Agreed.

>> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension.html:21
>> +shouldBeEqualToString("ws.extensions", "");
> 
> I prefer shouldBeEqualToString("ws.extensions = 'foo'; ws.extensions", "");

I see.

>> LayoutTests/http/tests/websocket/tests/hybi/extensions-expected.txt:7
>> +PASS ws.extensions is "x-webkit-deflate-frame"
> 
> I'd like to make the same comment with echo-with-no-extension-expected.txt.

Done.

[Takashi Toyoshima]

Created attachment 130975 [details]
Patch

[Kent Tamura]

Comment on attachment 130975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130975&action=review

> LayoutTests/http/tests/websocket/tests/hybi/echo-with-no-extension.html:21
> +// extensions attribute is read-only.
> +debug("Check if the value is read only:");

nit: The comment is not helpful.

> LayoutTests/http/tests/websocket/tests/hybi/extensions.html:20
>  // extensions attribute is read-only.

ditto.

[WebKit Review Bot]

Comment on attachment 130975 [details]
Patch

Clearing flags on attachment: 130975

Committed r110274: <http://trac.webkit.org/changeset/110274>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nandor Huszka]

This revision (r110274) causes fail on Qt. The diff:

--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/extensions-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/websocket/tests/hybi/extensions-actual.txt 
@@ -7,7 +7,7 @@
 Check if the value is read only:
 PASS ws.extensions = 'foo'; ws.extensions is ""
 Check the value after the connection is established:
-PASS ws.extensions is "x-webkit-deflate-frame"
+FAIL ws.extensions should be x-webkit-deflate-frame. Was .
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Kent Tamura]

(In reply to comment #11)
> This revision (r110274) causes fail on Qt. The diff:

It's the same reason with compressed-control-frame.html, deflate-frame-invalid-parameter.html, deflate-frame-parameter.html, and handshake-fail-by-extensions-header.html.  They are skipped in Qt.

[Nandor Huszka]

(In reply to comment #12)
> It's the same reason with compressed-control-frame.html, deflate-frame-invalid-parameter.html, deflate-frame-parameter.html, and handshake-fail-by-extensions-header.html.  They are skipped in Qt.

Thank you for the information, test will be inserted to that group.