[Webkit-unassigned] [Bug 83282] Web Inspector: Allow inspection of Web Socket Frames

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=83282


Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #135828|1                           |0
        is obsolete|                            |




--- Comment #5 from Pavel Feldman <pfeldman at chromium.org>  2012-04-05 08:37:49 PST ---
(From update of attachment 135828)
View in context: https://bugs.webkit.org/attachment.cgi?id=135828&action=review

Your general approach looks good. Note that you need to provide change logs with your changes per WebKit policy.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:556
> +    InspectorInstrumentation::didReceiveWebSocketFrame(m_document, m_identifier, frame);

I think you should report error frames as well.

> Source/WebCore/inspector/Inspector.json:685
> +                    { "name": "payloadLength", "type": "number", "description": "WebSocke frame payload length." },

Payload data length can be derived from the data in case you are sending the whole body. You should either sent truncated frames or skip this field.

> Source/WebCore/inspector/Inspector.json:891
> +                "name": "webSocketFrameResponseReceived",

webSocketFrameReceived? There are no requests / responses in WebSocket land. What about the webSocketFrameSent? You probably want to instrument frames going in both directions.

> Source/WebCore/inspector/Inspector.json:892
> +                "description": "Fired when WebSocket handshake response becomes available.",

Is this description accurate?

> Source/WebCore/inspector/Inspector.json:896
> +                    { "name": "response", "$ref": "WebSocketFrame", "description": "WebSocket response data." }

name: frame

> Source/WebCore/inspector/InspectorResourceAgent.cpp:460
> +    RefPtr<InspectorObject> responseObject = InspectorObject::create();

You should use the new type builder where possible:
RefPtr<TypeBuilder::Network::WebSocketFrame> frame = TypeBuilder::Network::WebSocketFrame::create()
    .setOpcode()
    .setMask()...

> Source/WebCore/inspector/front-end/NetworkItemView.js:62
> +        this.appendTab("frames", WebInspector.UIString("WebSocket Frames"), frameView);

appendTab("webSocketFrames", ...)

Please add new strings into English.lproj/localizedStrings.js in utf-16 encoding

> Source/WebCore/inspector/front-end/NetworkManager.js:454
> +            resource.payloads = {};

You should use array here:
resource.payloads = []. 
Also "payload" seems to be overloaded in the inspector world, so I would call it "frames" or "framePayloads"

> Source/WebCore/inspector/front-end/NetworkManager.js:455
> +            resource.payloads.size = 0;

size is always resource.frames.length

> Source/WebCore/inspector/front-end/NetworkManager.js:458
> +        resource.payloads[resource.payloads.size++] = response

resource.frames.push(response); This also is unbound growth, so you will drive the front-end out of memory eventually. Consider dropping frames.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:14
> + * You should have received a copy of the GNU Lesser General Public

You should provide BSD license with everything you contribute to the WebKit.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:33
> +        item.innerText = payload.time + '\t' + payload.opcode + ' ' + payload.mask + ' ' + payload.payloadLength + ' ' + payload.payloadData.substring(0, payload.payloadLength);

Please use String.sprintf defined in utilities.js

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:66
> +            content += payload.opcode + ' ' + payload.mask ? 'true' : 'false' + ' ' + payload.payloadLength + ' ' + payload.payloadData + '\t' + payload.time + '<br>';

Please provide screenshots with the patches that contain UI changes.

> Source/WebCore/inspector/front-end/inspector.html:147
> +    <script type="text/javascript" src="ResourceWebSocketFrameView.js"></script>

You should also include this new file into the following projects / lists:
WebKit.qrc,
WebCore.gypi,
WebCore.vcproj,
compile-front-end.py

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.