[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 #137919|1                           |0
        is obsolete|                            |




--- Comment #21 from Pavel Feldman <pfeldman at chromium.org>  2012-04-19 10:51:22 PST ---
(From update of attachment 137919)
View in context: https://bugs.webkit.org/attachment.cgi?id=137919&action=review

Looks good overall. A bunch of style nits.

> Source/WebCore/ChangeLog:11
> +        * English.lproj/localizedStrings.js:

You should describe what has changed and why either via putting a paragraph of text above or inline methods summary below.

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

I wonder if you should move this below or into the call site: there is a number of guards and I am not sure you want inspector to have entries for the invalid data entries without declaring them invalid.

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

Same as above, you could fail below and inspector would show the frame as successfully sent.

> Source/WebCore/inspector/Inspector.json:901
> +                "name": "webSocketFrameErrorReceived",

webSocketFrameError ?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:993
> +        if (InspectorResourceAgent* resourceAgent = instrumentingAgents->inspectorResourceAgent())

Here and below in the file: wrong indent.

> Source/WebCore/inspector/front-end/NetworkManager.js:468
> +        if (!networkRequest.frames)

You should not create fields on the object of another class. You should instead introduce NetworkRequest.prototype.addFrame() and frames() methods and annotate them for the compiler.

> Source/WebCore/inspector/front-end/NetworkManager.js:474
> +        if (networkRequest.frames.length >= 100)

You could encapsulate this into addFrame as well.

> Source/WebCore/inspector/front-end/NetworkManager.js:475
> +            networkRequest.frames.shift();

This looks inefficient. You should shift array in chunks, otherwise it'll be called for each frame.

> Source/WebCore/inspector/front-end/NetworkManager.js:524
> +        var errorFrame = {};

encapsulating it into NetworkRequest and creating a small class for the error frames there is the preferred way.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:25
> +    WebInspector.View.call(this);

This looks dense. Could you group the lines below via inserting blank lines?

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:29
> +    var table = document.createElement('table');

Here and below: please use double quotations.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:33
> +    headerRow.appendChild(empty);

Note that DOMExtensions declares convenient Element.prototype.createChild for code like below.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:50
> +    for (var i = 0; i < this.resource.frames.length; i++) {

blank line above

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:55
> +        count.innerText = (i + 1);

No need for ()

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:61
> +        dateCell.innerHTML = String.sprintf("%s %s", (payload.sent ? "→" : (payload.errorMessage ? "" : "←")), date.toISOString());

Assign to textContent, not innerHTML, otherwise, web socket would be able to inject javascript into the front-end page.

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