Bug 170760 – Web Inspector: WebSockets: messages with non-latin letters are displayed incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
1. Open Network tab.
2. Go to http://www.websocket.org/echo.html.
3. Click "Connect" button.
4. Type "Привет Мир" in the message field.
5. In the Network's tab left sidebar, select the newly created WebSocket connection.

Expected:
"Привет Мир" should be present in the message log.

Actual:
"ÐÑÐ¸Ð²ÐµÑ ÐиÑ" is shown instead.

[Nikita Vasilyev]

I have a fix, it will be in a patch for Bug 169175 - Web Inspector: Add tests for webSocketFrameSent and webSocketFrameReceived.

[Nikita Vasilyev]

Created attachment 307063 [details]
Patch

This patch should fix:
Bug 170609 - Web Inspector: WebSockets: Transferred size is incorrect
Bug 169175 - Web Inspector: Add tests for webSocketFrameSent and webSocketFrameReceived

Posting the patch here because I think this bug has the most user impact.

[Nikita Vasilyev]

Created attachment 307065 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 307065 [details]
Patch

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

r=me

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary-expected.txt:11
> +PASS: Resource size should increase by 21 byte.

Typo: "byte" => "bytes"

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:55
> +                resolve();
> +                InspectorTest.evaluateInPage("closeWebSocketConnection()");

Nit: Flip these lines. resolve should be the last action performed on behave of this test, because what if it synchronously triggered the next test and then the evaluate in page happened with the next test.
Style: I suggest using template strings anytime we are creating a string of code (such as the values passed to evaluateInPage).

> LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html:9
> +    let webSocket = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/inspector/send-and-receive");

This could get garbage collected and never do anything, right? I'd prefer if we assigned to a global to prevent any possible flakiness.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html:93
> +<p>Tests sending and receiving WebSocket messages.</p>

We should also test sending binary data. That is the one path not tested in these changes.

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:76
> +        if (typeof payloadLength === "undefined")

Style: No need to typeof, you can just compare to undefined:

    if (payloadLength === undefined)

[Nikita Vasilyev]

Created attachment 307180 [details]
Patch

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

>> LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html:9
>> +    let webSocket = new WebSocket("ws://127.0.0.1:8880/websocket/tests/hybi/inspector/send-and-receive");
> 
> This could get garbage collected and never do anything, right? I'd prefer if we assigned to a global to prevent any possible flakiness.

It hasn't been the case for me, but I made it global just in case.

[WebKit Commit Bot]

Comment on attachment 307180 [details]
Patch

Clearing flags on attachment: 307180

Committed r215388: <http://trac.webkit.org/changeset/215388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.