Bug 169649 – Web Inspector: WebSockets: Don't store binary frames in memory since they are never shown

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

[Nikita Vasilyev]

Web Socket binary frames are never displayed in the UI. There's no reason to clog Web Inspector's front-end memory with them.

[Radar WebKit Bug Importer]

<rdar://problem/31053069>

[Nikita Vasilyev]

Created attachment 304454 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 304454 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:78
> +        // Binary data is never shown in the UI, don't clog memory with it.
> +        if (opcode === WebInspector.WebSocketResource.OpCodes.BinaryFrame)
> +            return "";

Can we show the binary data? A hex view would be neat. Instead of the empty string maybe we can show a stylized localized string, such as "(Binary Data)" or at least an emDash?

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:79
> +        else

Style: We normally do not include an else when the previous branch always returns.

[Nikita Vasilyev]

Comment on attachment 304454 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:78
>> +            return "";
> 
> Can we show the binary data? A hex view would be neat. Instead of the empty string maybe we can show a stylized localized string, such as "(Binary Data)" or at least an emDash?

We can show binary data, but there could be several megabytes of it. If we make a UI for showing binary data, I'd make it available for Resources as well, not just WebSocketContentView. I'm curious to explore this but I think it's a low priority now.

We currently show "Binary Frame", not an empty string: https://github.com/WebKit/webkit/blob/master/Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js#L95-L98

[Build Bot]

Comment on attachment 304454 [details]
Patch

Attachment 304454 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3326029

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/fire-success-event-exception.html

[Build Bot]

Created attachment 304463 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Nikita Vasilyev]

Created attachment 304466 [details]
Patch

The test fail seems unrelated to my patch.

[Blaze Burg]

Comment on attachment 304466 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:74
> +    _dataForWebSocketFrame(data, opcode)

I don't see any reason to make this a private member. Let's just inline this into addFrame.

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:78
> +            return "";

This should be null, not an empty string.

[Nikita Vasilyev]

Created attachment 306130 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306130 [details]
Patch

Clearing flags on attachment: 306130

Committed r214853: <http://trac.webkit.org/changeset/214853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.