Bug 170760 - Web Inspector: WebSockets: messages with non-latin letters are displayed incorrectly
Summary: Web Inspector: WebSockets: messages with non-latin letters are displayed inco...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords:
Depends on:
Blocks: 169175
  Show dependency treegraph
 
Reported: 2017-04-11 18:51 PDT by Nikita Vasilyev
Modified: 2017-04-14 20:12 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.16 KB, patch)
2017-04-13 18:13 PDT, Nikita Vasilyev
nvasilyev: review-
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
Patch (22.16 KB, patch)
2017-04-13 18:14 PDT, Nikita Vasilyev
joepeck: review+
Details | Formatted Diff | Diff
Patch (23.31 KB, patch)
2017-04-14 19:27 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-04-11 18:51:58 PDT
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.
Comment 1 Nikita Vasilyev 2017-04-11 19:11:45 PDT
I have a fix, it will be in a patch for Bug 169175 - Web Inspector: Add tests for webSocketFrameSent and webSocketFrameReceived.
Comment 2 Nikita Vasilyev 2017-04-13 18:13:03 PDT
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.
Comment 3 Nikita Vasilyev 2017-04-13 18:14:54 PDT
Created attachment 307065 [details]
Patch
Comment 4 Joseph Pecoraro 2017-04-13 18:35:18 PDT
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)
Comment 5 Nikita Vasilyev 2017-04-14 19:27:50 PDT
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.
Comment 6 WebKit Commit Bot 2017-04-14 20:12:22 PDT
Comment on attachment 307180 [details]
Patch

Clearing flags on attachment: 307180

Committed r215388: <http://trac.webkit.org/changeset/215388>
Comment 7 WebKit Commit Bot 2017-04-14 20:12:23 PDT
All reviewed patches have been landed.  Closing bug.