[webkit-reviews] review canceled: [Bug 40768] Web Inspector: WebSocket in Resources tab : [Attachment 59642] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has canceled Yuta Kitamura
<yutak at chromium.org>'s request for review:
Bug 40768: Web Inspector: WebSocket in Resources tab
https://bugs.webkit.org/show_bug.cgi?id=40768

Attachment 59642: Patch
https://bugs.webkit.org/attachment.cgi?id=59642&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Looks sane. Please find a number of nits below. I don't like the way m_frame
and m_loader are now nullable, but maybe it is Ok for now.

WebCore/inspector/InspectorController.cpp:1441
 +  void InspectorController::didCreateWebSocket(unsigned long identifier,
const KURL& url)
Do we really need to distinguish between didCreateWebSocket and
willSendWebSocketHandshake? Can socket lag in between of those on its own or
these are just corresponding js calls?

WebCore/inspector/InspectorController.cpp:1470
 +	// FIXME: Record timing?
I think you should mark response received time here.

WebCore/inspector/InspectorResource.cpp:199
 +		jsonObject.set("documentURL", m_requestURL.string());
Not sure this is entirely correct. Frameless resource should not reference any
document.

WebCore/inspector/InspectorResource.cpp:327
 +	if (!m_loader)
I think this should be based on Key challenge, not the loader existence.

WebCore/websockets/WebSocketChannel.cpp:370
 +		m_identifierAssigned = true;
Isn't it equivalent to m_identifier != 0?


WebCore/inspector/front-end/ResourceView.js:283
 +		headers.push({header: "(Key3)", value:
this.resource.webSocketRequestKey3});
This formally is not a header. We do have special treatment of form parameters
and such. This key could have dedicated place in UI too, no need for hack.

WebCore/inspector/front-end/ResourceView.js:292
 +		headers.push({header: "(Challenge Response)", value:
this.resource.webSocketChallengeResponse});
Ditto.