[webkit-reviews] review denied: [Bug 172312] Web Inspector: Should see active Web Sockets when opening Web Inspector : [Attachment 311560] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 172312: Web Inspector: Should see active Web Sockets when opening Web
Inspector
https://bugs.webkit.org/show_bug.cgi?id=172312

Attachment 311560: Patch

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




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

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

> Source/WebCore/Modules/websockets/WebSocket.cpp:195
> +HashSet<WebSocket*>& WebSocket::allActiveWebSockets()
> +{
> +    static NeverDestroyed<HashSet<WebSocket*>> activeWebSockets;
> +    return activeWebSockets;
> +}

I think this may need to be threadsafe, since Worker contexts may create these.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:837
> +unsigned WebSocketChannel::identifier() const
> +{
> +    return m_identifier;
> +}

Lets inline this in the header.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:664
> +	   Document& document =
downcast<Document>(*webSocket->scriptExecutionContext());
> +	   if (document.page() != &m_pageAgent->page())
> +	       continue;

You cannot assume the scriptExecutionContext here is a Document. It might be a
WorkerGlobalScope since a Worker can create a WebSocket. You will need to
is<Document> check first.