[webkit-reviews] review granted: [Bug 170760] Web Inspector: WebSockets: messages with non-latin letters are displayed incorrectly : [Attachment 307065] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 170760: Web Inspector: WebSockets: messages with non-latin letters are
displayed incorrectly
https://bugs.webkit.org/show_bug.cgi?id=170760

Attachment 307065: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 307065
  --> https://bugs.webkit.org/attachment.cgi?id=307065
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)