Add Web Socket Frame inspection to the network item view.
Created attachment 135826 [details] Prototype
Created attachment 135828 [details] patch
Attachment 135828 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Modules/websockets/WebSocke..." exit_code: 1 Source/WebCore/Modules/websockets/WebSocketChannel.cpp:555: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Source/WebCore/inspector/InspectorResourceAgent.cpp:64: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 135832 [details] updated patch
Comment on attachment 135828 [details] patch 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
Comment on attachment 135832 [details] updated patch As per comments above.
Comment on attachment 135828 [details] patch Attachment 135828 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12341367
Comment on attachment 135828 [details] patch Attachment 135828 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12339341
Comment on attachment 135828 [details] patch Attachment 135828 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12337326
Comment on attachment 135828 [details] patch Attachment 135828 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12264335
Comment on attachment 135828 [details] patch Attachment 135828 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12340362
Created attachment 135864 [details] Screenshot. I'm open to suggestions as to how else this can be visualized.
is there a template for testing web inspector front-end changes?
(In reply to comment #13) > is there a template for testing web inspector front-end changes? All web inspector tests reside in LayoutTests/inspector, LayoutTests/http/tests/inspector, and LayoutTests/http/tests/inspector-enabled. The latter two are for testing against a web server (which I guess is what you need), the former one is for the code that can deal with a local page. Find a test that is close to what you need and copy it! However, I'm not sure if our test web server supports web sockets...
Preview and Response tabs always show "This request has no preview available" and "This request has no response data available" respectively. Perhaps we can remove them. > (In reply to comment #12) > Created an attachment (id=135864) [details] > Screenshot. > > I'm open to suggestions as to how else this can be visualized. How does it distinguish sent/received data?
(In reply to comment #15) > Preview and Response tabs always show "This request has no preview available" and "This request has no response data available" respectively. Perhaps we can remove them. > > > (In reply to comment #12) > > Created an attachment (id=135864) [details] [details] > > Screenshot. > > > > I'm open to suggestions as to how else this can be visualized. > > How does it distinguish sent/received data? The above example, and code has only received data. I've updated my patch locally to show sent data as well, but am still looking as to how to distinguish them. I'll post another screenshot with an example soon...I'm also still working on tests.
Created attachment 137144 [details] Updated Screenshot Bi-directional text frames tracked. Currently I don't distinguish between what is a sent vs received frame. However the sent frames all seem to be masked while the received do not. Will post updated patch with tests once I figure out how to write them.
Created attachment 137905 [details] Patch
Created attachment 137919 [details] Patch
Created attachment 137922 [details] Patch rebased patch so ews and style checker will apply/run correctly
Comment on attachment 137919 [details] Patch 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.
Comment on attachment 137922 [details] Patch Clearing r? to remove this from the dashboard. I reviewed the previous version of the patch already.
Comment on attachment 137922 [details] Patch Attachment 137922 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12437264
>> 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. This isn't really a valid concern since the string being created is not comprised of user input of any kind. I'm simply putting a left arrow (←) or right arrow (→) to indicate whether it is a sent or received frame and then putting the date in an ISOString format. The frame playload isn't being set as innerHTML. It is set as innerText below.
Comment on attachment 137922 [details] Patch Attachment 137922 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12345355 New failing tests: http/tests/inspector/web-socket-frame.html
Created attachment 137943 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 137969 [details] Patch
Comment on attachment 137969 [details] Patch Attachment 137969 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12428717
Comment on attachment 137969 [details] Patch Attachment 137969 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12297444 New failing tests: http/tests/inspector/web-socket-frame.html
Created attachment 137988 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 137969 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137969&action=review Few nits and it is good to go. > Source/WebCore/inspector/front-end/NetworkRequest.js:810 > + /** You could narrow the API surface down via introducing single frames() method. > Source/WebCore/inspector/front-end/NetworkRequest.js:830 > + getFrame: function(position) WebKit guidelines discourages use of get prefixes. frame: function(position). I would strongly encourage you to simply expose frames: array here even if it returns live collection. > Source/WebCore/inspector/front-end/NetworkRequest.js:863 > + this._frames.splice(0,10); Space before 10. > Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:67 > + dateCell.innerHTML = String.sprintf("%s %s", (payload.sent ? "→" : (payload.errorMessage ? "" : "←")), date.toISOString()); It does not really matter whether this is user data or not, you should only use innerHTML = for the (a) sanitized (b) HTML content. Otherwise it instantiates HTML parser, works slowly and results in vulnerabilities. You should look up \u symbol codes for the arrows and append them to your text content. > Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:73 > + spanCell.innerText = payload.errorMessage; innerText is not standard, uses weird semantics. You want .textContent = here.
Created attachment 138080 [details] Patch
Comment on attachment 138080 [details] Patch Attachment 138080 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12476169
Comment on attachment 138080 [details] Patch Attachment 138080 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12470236 New failing tests: http/tests/inspector/web-socket-frame.html
Created attachment 138094 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 138080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138080&action=review One nit and I think it is ready. > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:932 > + InspectorInstrumentation::didSendWebSocketFrame(m_document, m_identifier, frame); The m_deflateFramer.deflate call above will mutate frame's data and, as a result, you will get garbled data instrumented. You could move this instrument call back to before deflate and assume that deflate is always successful. When we get a bug report claiming otherwise, we'll start instrumenting raw data*.
Created attachment 138389 [details] Patch
Comment on attachment 138389 [details] Patch Needs to be rebased against ToT
Created attachment 138396 [details] Patch
Comment on attachment 138396 [details] Patch Attachment 138396 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12490016
Comment on attachment 138396 [details] Patch Attachment 138396 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12524231 New failing tests: http/tests/inspector/web-socket-frame.html
Created attachment 138527 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 138548 [details] Patch
Comment on attachment 138548 [details] Patch Attachment 138548 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12522229
Comment on attachment 138548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138548&action=review > Source/WebCore/inspector/InspectorAllInOne.cpp:26 > +// This all-in-one cpp file cuts down on template bloat to allow us to build our Windows release builds. You shouldn't need to do this once you fix the actual causes of the build errors (see below). > Source/WebCore/inspector/InspectorInstrumentation.h:74 > +class WebSocketFrame; This is causing the build errors on Windows and I am not sure why it isn't causing build errors on other platforms. WebSocketFrame is a struct, not a class, so the linker was trying to find a function that took a *class* WebSocketFrame when there was only one that took a *struct* WebSocketFrame. Please forward declare it as a struct. > Source/WebCore/inspector/InspectorResourceAgent.h:70 > +class WebSocketFrame; This is causing the build errors on Windows. WebSocketFrame is a struct, not a class.
Created attachment 139053 [details] Patch
Comment on attachment 139053 [details] Patch Attachment 139053 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12551091 New failing tests: fast/images/gif-large-checkerboard.html
Created attachment 139130 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 139053 [details] Patch Clearing flags on attachment: 139053 Committed r115427: <http://trac.webkit.org/changeset/115427>
All reviewed patches have been landed. Closing bug.