[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 bugzilla-daemon at webkit.org
Thu Apr 13 18:35:18 PDT 2017

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

Attachment 307065: Patch


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

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


> 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

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

> +<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)

More information about the webkit-reviews mailing list