Bug 167520

WebInspector.Resource assumes that a resource is an HTTP resource. A WebSocket connection is different from an HTTP connection in many ways: — Several incoming and outgoing messages in a single connection; — No headers, except for the initial handshake HTTP request (https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#Client_Handshake_Request); — Different status codes (opcode); — No MIME types; — Can't be cached. I'm planning on creating a new resource model, WebInspector.StreamResource. It won't inherit from WebInspector.Resource. WebInspector.Resource extends WebInspector.SourceCode, which provides methods for source maps, editing revisions, and other things that wouldn't make sense for WebSockets. I don't see a reason for WebInspector.StreamResource to extend WebInspector.SourceCode.

Some thoughts: (In reply to comment #1) > WebInspector.Resource assumes that a resource is an HTTP resource. A > WebSocket connection is different from an HTTP connection in many ways: > — Several incoming and outgoing messages in a single connection; > — No headers, except for the initial handshake HTTP request > (https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/ > Writing_WebSocket_servers#Client_Handshake_Request); > — Different status codes (opcode); > — No MIME types; > — Can't be cached. > > I'm planning on creating a new resource model, WebInspector.StreamResource. > It won't inherit from WebInspector.Resource. This name is confusing. I would assume that it is a subclass based on the name. How about about `WI.Stream extends WI.Object` for now, which could be shared in the future with other persistent data frame based connections like Media loading and WebRTC. We can figure out the similarities later, for now you can just use Stream and later move some code to WebSocketStream, MediaStream, etc. > > WebInspector.Resource extends WebInspector.SourceCode, which provides > methods for source maps, editing revisions, and other things that wouldn't > make sense for WebSockets. I don't see a reason for > WebInspector.StreamResource to extend WebInspector.SourceCode. Right, I don't think we want a text editor as the main content view. However WI.Resource is deeply baked into the inspector so there will be some difficulties fitting this into the existing system. One thing that might arise when using a new class inside Resource tab that's not a resource: some parts of the code will probably have to special-case Stream. Some examples I found with a quick search: - WI.Frame.EventResourceWasAdded is used in a lot of places - WI.Resource.Type.* enum is referenced in many places, some might want to handle streams - WI.ResourceCollection assumes items are Resource subclasses - Explicit type checks in ResourceSidebarPanel and friends (Resource || Frame || Collection etc)

Created attachment 301652 [details] [Image] Web Socket handshake HTTP headers Web Sockets handshake is done via HTTP. Given that, it does make sense to use Resource class for Web Sockets. On this screenshot the right sidebar shows handshake HTTP headers.

Created attachment 301724 [details] Patch A sample page that uses Web Sockets: http://www.websocket.org/echo.html A screenshot with patch applied is attached above (https://bug-167520-attachments.webkit.org/attachment.cgi?id=301652).

Comment on attachment 301724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301724&action=review > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:220 > + let frameIdentifier = WebInspector.frameResourceManager.mainFrame.id; Is it not possible for a websocket to be loaded via an iframe (or some other frame), or am I misunderstanding what this code does? > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:222 > + let frame = this.frameForIdentifier(frameIdentifier); Wouldn't this just be `WebInspector.frameResourceManager.mainFrame`? > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:223 > + let targetId; I think this should be initialized to something. > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:237 > + var originalRequestWillBeSentTimestamp = timestamp; NIT: let instead of var This is also for multiple places below. > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:256 > + delete this._resourceRequestIdentifierMap[requestIdentifier]; I think we are trying to avoid this pattern in new code, so it might be worth refactoring `this._resourceRequestIdentifierMap` to be an actual Map. > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:77 > + let {payloadData, opcode, mask} = response; Considering that you aren't using the response variable anywhere else, why not just put the destructure in the parenthesis? > Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:26 > +WebInspector.WebSocketContentView = class TextResourceContentView extends WebInspector.ContentView s/TextResourceContentView/WebSocketContentView Also, is there a reason to not implement this as a subclass of ResourceContentView? I realize that this is a stub for now, but I think that it makes more sense to keep it in line with the rest of WebInspector.Resource.Type

Comment on attachment 301724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301724&action=review >> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:26 >> +WebInspector.WebSocketContentView = class TextResourceContentView extends WebInspector.ContentView > > s/TextResourceContentView/WebSocketContentView > > Also, is there a reason to not implement this as a subclass of ResourceContentView? I realize that this is a stub for now, but I think that it makes more sense to keep it in line with the rest of WebInspector.Resource.Type I forgot to add this, but what happens when you the user clicks the go-to arrow for the WebSocket resource in the sidebar? Does it display a blank content view, or is some text shown? Can you screenshot that please :D

Comment on attachment 301724 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301724&action=review I'd like to see an updated patch before a complete review given the existing comments. > Source/WebInspectorUI/ChangeLog:17 > + webSocketWillSendHandshakeRequest doesn't recieve a request URL as one of its parameters. Typo: recieve > Source/WebInspectorUI/ChangeLog:18 > + Store URLs recived from webSocketCreated method in _webSocketIdentifierToUrl map. Typo: recived > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:52 > + this._webSocketIdentifierToUrl = new Map; Style: The suffix should be "toURL" to match all existing code. >> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:220 >> + let frameIdentifier = WebInspector.frameResourceManager.mainFrame.id; > > Is it not possible for a websocket to be loaded via an iframe (or some other frame), or am I misunderstanding what this code does? Yeah, this is the first go at WebSockets. There should absolutely be a FIXME and a bugzilla bug here about making it work properly across frames. (The same is true of Web Workers, but we didn't realize that until now and thats a bit unfortunate). > Source/WebInspectorUI/UserInterface/Main.html:677 > <script src="Views/WorkerTreeElement.js"></script> > + <script src="Views/WebSocketContentView.js"></script> Nit: Order >> Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:77 >> + let {payloadData, opcode, mask} = response; > > Considering that you aren't using the response variable anywhere else, why not just put the destructure in the parenthesis? Our Observer should as close as possible match the protocol and just pass through to the manager. It is meant to be a layer to be a buffer between the frontend code and the remote connection in case of protocol changes. So almost all just pass directly through to a manager (which I think is what we should do here). I don't think we should do the destructure here. If we want we should do it down in the manager.

(In reply to comment #5) > Comment on attachment 301724 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301724&action=review > > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:220 > > + let frameIdentifier = WebInspector.frameResourceManager.mainFrame.id; > > Is it not possible for a websocket to be loaded via an iframe (or some other > frame), or am I misunderstanding what this code does? It is indeed possible. With this patch those WebSocket connections are displayed under "Sockets" of the top level resource and not under "Frames". > > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:222 > > + let frame = this.frameForIdentifier(frameIdentifier); > > Wouldn't this just be `WebInspector.frameResourceManager.mainFrame`? It would, but I'd prefer to keep it `this.frameForIdentifier` so I don't have to change it again once we add a proper frameIdentifier. > > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:223 > > + let targetId; > > I think this should be initialized to something. Other than undefined? > > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:237 > > + var originalRequestWillBeSentTimestamp = timestamp; > > NIT: let instead of var > > This is also for multiple places below. Done. > > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:256 > > + delete this._resourceRequestIdentifierMap[requestIdentifier]; > > I think we are trying to avoid this pattern in new code, so it might be > worth refactoring `this._resourceRequestIdentifierMap` to be an actual Map. I think it's better to be done in a follow up patch. I generally don't like mixing new features with refactoring — it's a lot of code changes. When it introduces a regression, it's harder to pinpoint. > > > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:77 > > + let {payloadData, opcode, mask} = response; > > Considering that you aren't using the response variable anywhere else, why > not just put the destructure in the parenthesis? > > > Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:26 > > +WebInspector.WebSocketContentView = class TextResourceContentView extends WebInspector.ContentView > > s/TextResourceContentView/WebSocketContentView > > Also, is there a reason to not implement this as a subclass of > ResourceContentView? I realize that this is a stub for now, but I think > that it makes more sense to keep it in line with the rest of > WebInspector.Resource.Type ResourceContentView assumes that the content is a single object, not a stream. I'll look into this more, but it seems ResourceContentView methods aren't useful for stream data.

Created attachment 301851 [details] Patch

Created attachment 301852 [details] [Image] Web Socket content view is blank (In reply to comment #6) > Comment on attachment 301724 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301724&action=review > > >> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:26 > >> +WebInspector.WebSocketContentView = class TextResourceContentView extends WebInspector.ContentView > > > > s/TextResourceContentView/WebSocketContentView > > > > Also, is there a reason to not implement this as a subclass of ResourceContentView? I realize that this is a stub for now, but I think that it makes more sense to keep it in line with the rest of WebInspector.Resource.Type > > I forgot to add this, but what happens when you the user clicks the go-to > arrow for the WebSocket resource in the sidebar? Does it display a blank > content view, or is some text shown? Can you screenshot that please :D It's a blank content view. Once this lands, the next patch will be to display actual incoming and outgoing messages.

(In reply to comment #8) > > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:256 > > > + delete this._resourceRequestIdentifierMap[requestIdentifier]; > > > > I think we are trying to avoid this pattern in new code, so it might be > > worth refactoring `this._resourceRequestIdentifierMap` to be an actual Map. > > I think it's better to be done in a follow up patch. I generally don't like > mixing > new features with refactoring — it's a lot of code changes. When it > introduces a regression, > it's harder to pinpoint. I wrote a separate patch for it: https://bugs.webkit.org/show_bug.cgi?id=168549

Created attachment 302065 [details] Patch Rebaselined after Bug 168549 - Web Inspector: Use Maps in FrameResourceManager instead of objects.

Comment on attachment 302065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302065&action=review I'd suggest putting up a patch that includes a ContentView. Rather then leave the frontend in a half implemented state where Web Sockets show blank ContentViews. This is a small patch so far, so adding a ContentView and related logic to it will be fine. Finally, this needs tests. You are testing a part of the protocol that is untested, so treat it as new. There should be tests for: Network.webSocketCreated Network.webSocketWillSendHandshakeRequest Network.webSocketHandshakeResponseReceived Network.webSocketClosed I would expect to see tests for WebSocket handshake successes and failures. I would expect to see tests for WebSockets closed on the page side and the server side. > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:224 > + // HTTP Post data. Comment seems unnecessary > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:235 > + // COMPATIBILITY (iOS 8): Timeline timestamps for legacy backends are computed > + // dynamically from the first backend timestamp received. For navigations we > + // need to reset that base timestamp, and an appropriate timestamp to use is > + // the new main resource's will be sent timestamp. So save this value on the > + // resource in case it becomes a main resource. > + let originalRequestWillBeSentTimestamp = timestamp; This resource will never become a main resource. This doesn't make sense to me, why is it here? > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:240 > + // Associate the resource with the requestIdentifier so it can be found in future loading events. What does this comment mean? > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:346 > + webSocketClosed(requestIdentifier, timestamp) Style: This should be moved up next to all of the other webSocketFoo methods. > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:72 > + WebInspector.frameResourceManager.webSocketWillSendHandshakeRequest(requestId, timestamp, request.headers); Why not just send the entire request object? Currently it only has headers, but in the future it might have more.

Comment on attachment 302065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302065&action=review > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:225 > + let requestData = null; I didn't understand at first what requestData is. It seemed ambiguous, so I added a comment. >> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:235 >> + let originalRequestWillBeSentTimestamp = timestamp; > > This resource will never become a main resource. This doesn't make sense to me, why is it here? This is copy/paste from resourceRequestWillBeSent method. I'll remove this comment. >> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:240 >> + // Associate the resource with the requestIdentifier so it can be found in future loading events. > > What does this comment mean? _resourceRequestIdentifierMap is used by webSocketClosed and webSocketHandshakeResponseReceived methods, that may get called after this method (webSocketWillSendHandshakeRequest). Now I think this comment isn't needed. >> Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:72 >> + WebInspector.frameResourceManager.webSocketWillSendHandshakeRequest(requestId, timestamp, request.headers); > > Why not just send the entire request object? Currently it only has headers, but in the future it might have more. Makes sense.

Comment on attachment 302065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302065&action=review >>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:240 >>> + // Associate the resource with the requestIdentifier so it can be found in future loading events. >> >> What does this comment mean? > > _resourceRequestIdentifierMap is used by webSocketClosed and webSocketHandshakeResponseReceived methods, that may get called after this method (webSocketWillSendHandshakeRequest). Now I think this comment isn't needed. This is also copy/paste from resourceRequestWillBeSent.

Created attachment 303030 [details] Patch

Comment on attachment 303030 [details] Patch Attachment 303030 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3212917 New failing tests: http/tests/websocket/tests/hybi/inspector-send.html

Created attachment 303035 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

(In reply to comment #18) > Created attachment 303035 [details] > Archive of layout-test-results from ews121 for ios-simulator-wk2 > > The attached test failures were seen while running run-webkit-tests on the > ios-sim-ews. > Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6 Regressions: Unexpected timeouts (1) http/tests/websocket/tests/hybi/inspector-send.html [ Timeout ] I wonder why it happened only for iOS simulator.

Comment on attachment 303030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303030&action=review r- because this needs more tests. The current test is a good start, but it needs to be greatly expanded to test all of the cases that this code claims to cover. See comments below. > LayoutTests/ChangeLog:12 > + * http/tests/websocket/tests/hybi/inspector-send-expected.txt: Added. > + * http/tests/websocket/tests/hybi/inspector-send.html: Added. > + * http/tests/websocket/tests/hybi/inspector-send_wsh.py: Added. Lets make an entire inspector directory: http/tests/websocket/tests/hybi/inspector/resource-basic.html http/tests/websocket/tests/hybi/inspector/resource-handshake-failure.html http/tests/websocket/tests/hybi/inspector/resource-server-terminated.html ... This still doesn't cover all of the ways that WebSocket connections can terminate. Namely my earlier comments: * I would expect to see tests for WebSocket handshake successes and failures. * I would expect to see tests for WebSockets closed on the page side and the server side. Also, now that you added code for handling messages I'd expect tests for: * WebSocketResource FrameAdded events - Message Received + Contents - Message Sent + Contents - Timestamps increase - opcode (I'm curious to know what this is) * Ideally tests for frame errors (this would be critical for developers) > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Nit: We've been using the HTML5 Doctype in all our tests. > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:11 > +<p>Tests WebSockets network inspection.</p> > +<div id="description"></div> > +<div id="console"></div> > +<script type="text/javascript"> The description/console stuff should not be necessary. And these elements are bit jumbled. Our normal template for inspector tests is like: <!DOCTYPE html> <html> <head> <script src="...../http/tests/inspector/resources/inspector-test.js"></script> <script> // Inspected page JavaScript goes here at the top. function triggerSomeAction() { ... } // Inspector JavaScript test function goes at the end. function test() { let suite = InspectorTest.createAsyncSuite("Domain.Something"); suite.addTestCase({ name: "SyntaxErrorType.None", description: "Description of individual test.", test(resolve, reject) { ... } }); suite.runTestCasesAndFinish(); } </script> </head> <body onload="runTest()"> <p>Description of complete test.</p> </body> </html> This should be reworked a bit to match our style. Given our tests have above average complexity, eliminating complexity by following common patterns makes it much easier to quickly understand a test. > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:14 > + let suite = InspectorTest.createAsyncSuite("WebInspectorWebSockets"); The WebInspector part of this name is redundant. How about: "WebSocket.Resource" or "WebSocket.Basic". I'm much rather see tests specifically around Open/Close scenarios. There is a lot that is not being tested here. > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:17 > + name: "WebSocketResourceIsCreated", This is more of a test of "WebSocketResource.ReadyState" so that may be the most appropriate name. > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:24 > + InspectorTest.expectThat(resource instanceof WebInspector.WebSocketResource, "Resource should be created."); Nit: "Resource" => "WebSocketResource" > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:32 > + InspectorTest.expectEqual(data.previousState, WebInspector.WebSocketResource.ReadyState.Connecting, "Resource should start from a Closed state."); > + InspectorTest.expectEqual(data.state, WebInspector.WebSocketResource.ReadyState.Open, "Resource state should change to Connecting."); These pass messages do not match what is being checked. The top one starts from "Connecting" and the new state is "Open". > LayoutTests/http/tests/websocket/tests/hybi/inspector-send.html:44 > + InspectorTest.evaluateInPage("createWebSocketConnection()"); Style: This should be dedented. > Source/WebInspectorUI/ChangeLog:62 > +2017-02-18 Nikita Vasilyev <nvasilyev@apple.com> Double ChangeLog > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:231 > + // COMPATIBILITY (iOS 8): Timeline timestamps for legacy backends are computed > + // dynamically from the first backend timestamp received. For navigations we > + // need to reset that base timestamp, and an appropriate timestamp to use is > + // the new main resource's will be sent timestamp. So save this value on the > + // resource in case it becomes a main resource. > + let originalRequestWillBeSentTimestamp = timestamp; My earlier comment still applies. This will never be a Main Resource, so why is this here? > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:233 > + let resource = new WebInspector.WebSocketResource(url, loaderIdentifier, targetId, requestIdentifier, request.headers, requestData, elapsedTime , initiatorSourceCodeLocation, originalRequestWillBeSentTimestamp); Style: "elapsedTime ," => "elapsedTime," > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:253 > + //resource.updateForResponse(cachedResourcePayload.url, response.mimeType, cachedResourcePayload.type, response.headers, response.status, response.statusText, elapsedTime, response.timing); Should this be commented out? > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:270 > + webSocketClosed(requestIdentifier, timestamp) Nit: Sometimes you use "requestIdentifier" and other times you use "requestId". Pick one and be consistent throughout the patch. > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved.. Typo: ".." at the end should just be a single "." > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:28 > + constructor(url, loaderIdentifier, targetId, requestIdentifier, requestHeaders, requestData, requestSentTimestamp, initiatorSourceCodeLocation, originalRequestWillBeSentTimestamp) originalRequestWillBeSentTimestamp doesn't make sense for this kind of resource. It should be removed. > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:35 > + this._readyState = WebInspector.WebSocketResource.ReadyState.Closed; The initial state should be Connecting. You can then remove the line setting the initial state to Connecting in FrameResourceManager.prototype.webSocketWillSendHandshakeRequest. Also, it seems _state would be appropriate. Is there a reason to prefer _readyState? > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:50 > + get readyState() { return this._readyState; } See the Web Inspector style guidelines: <https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide> * When paired, getters should always go before setters. * If there are single line getters without setters, they should go at the top. * If there are getters and setters that are non-trivial they should both be together and both be multi-line > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:59 > + this.dispatchEventToListeners(WebInspector.WebSocketResource.Event.FrameAdded, frame); r- here because this deserves some kind of test. We cannot be adding more and more untested code when it would be easy to test. > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:88 > + WebInspector.frameResourceManager.webSocketFrameReceived(requestId, timestamp, response); > // FIXME: Not implemented. Remove the FIXME. > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:94 > + > // FIXME: Not implemented. Why isn't this handled? > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:100 > + WebInspector.frameResourceManager.webSocketFrameSent(requestId, timestamp, response); > // FIXME: Not implemented. Remove the FIXME. > Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:94 > + if (this._resource.type == WebInspector.Resource.Type.WebSocket) Style: ===

> Regressions: Unexpected timeouts (1) > http/tests/websocket/tests/hybi/inspector-send.html [ Timeout ] > > I wonder why it happened only for iOS simulator. We skip all inspector tests on ios-simulator because there is no local inspector. Therefore we should skip these tests as well. That is another +1 for making an inspector directory here, so we can then skip the entire directory. See: LayoutTests/platform/ios-simulator/TestExpectations # iOS doesn't have a local inspector inspector/ [ Skip ] http/tests/inspector/ [ Skip ]

(In reply to comment #20) > Comment on attachment 303030 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=303030&action=review Thank you for the prompt and detailed review! I answered your questions below. > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:231 > > + // COMPATIBILITY (iOS 8): Timeline timestamps for legacy backends are computed > > + // dynamically from the first backend timestamp received. For navigations we > > + // need to reset that base timestamp, and an appropriate timestamp to use is > > + // the new main resource's will be sent timestamp. So save this value on the > > + // resource in case it becomes a main resource. > > + let originalRequestWillBeSentTimestamp = timestamp; > > My earlier comment still applies. This will never be a Main Resource, so why > is this here? I'll remove it. > > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:253 > > + //resource.updateForResponse(cachedResourcePayload.url, response.mimeType, cachedResourcePayload.type, response.headers, response.status, response.statusText, elapsedTime, response.timing); > > Should this be commented out? It shouldn't. I'll remove it. > > Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:35 > > + this._readyState = WebInspector.WebSocketResource.ReadyState.Closed; > > The initial state should be Connecting. You can then remove the line setting > the initial state to Connecting in > FrameResourceManager.prototype.webSocketWillSendHandshakeRequest. > > Also, it seems _state would be appropriate. Is there a reason to prefer > _readyState? It was influenced by the ready state constants: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket#Ready_state_constants > > Source/WebInspectorUI/UserInterface/Protocol/NetworkObserver.js:94 > > + > > // FIXME: Not implemented. > > Why isn't this handled? I'm working on this.

Comment on attachment 303030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303030&action=review >> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:270 >> + webSocketClosed(requestIdentifier, timestamp) > > Nit: Sometimes you use "requestIdentifier" and other times you use "requestId". Pick one and be consistent throughout the patch. Tangentially: It's always requestId in NetworkObserver but in FrameResourceManager it's always requestIdentifier. Should I unify these as well?

Created attachment 303177 [details] WIP I addressed issues found in the review but there're no new tests yet.

Created attachment 303366 [details] Patch This patch has tests for client and server connection termination. More tests will be added in patches to follow.

Comment on attachment 303366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303366&action=review > LayoutTests/http/tests/websocket/tests/hybi/inspector/client-close.html:58 > + }); I'm not sure how to do this using promises. I'd need to add two event listeners at the same time (and not when the first one fires) and ensure that one fires after another.

Comment on attachment 303366 [details] Patch Attachment 303366 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3233429 New failing tests: media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html

Created attachment 303371 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 303374 [details] Patch Could be an unrelated flaky test, uploading the same patch again.

Comment on attachment 303366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303366&action=review r=me, we should strive to add more tests for this, such as messages soon. > LayoutTests/http/tests/websocket/tests/hybi/inspector/client-close.html:9 > +<body onload="runTest()"> > +<p>Tests WebSockets connection closed by the client.</p> > +<script> This still is not the normal template for inspector tests. Please clean it up. >> LayoutTests/http/tests/websocket/tests/hybi/inspector/client-close.html:58 >> + }); > > I'm not sure how to do this using promises. I'd need to add two event listeners at the same time (and not when the first one fires) and ensure that one fires after another. I think the current approach is fine. The reject is not going to happen after we have already resolved, so you can just do: if (count === 1) { ... return; } if (count === 2) { ... resolve(); } InspectorTest.fail("Unexpected ReadyStateChanged"); > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:221 > + let targetId; Lets move this up by the FIXME for frameId and loaderId, since this is another one that will need to be fixed for Workers WebSockets. > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:244 > + // FIXME: Unimplemented for Web Sockets. Maybe this should have a bugzilla bug. We might want some timing information to handshake setup or the initial connection setup. It is certainly fine to be null for now.

Comment on attachment 303374 [details] Patch See my earlier comments. Yes, the failure is unrelated.

Created attachment 303391 [details] Patch

Comment on attachment 303366 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303366&action=review >>> LayoutTests/http/tests/websocket/tests/hybi/inspector/client-close.html:58 >>> + }); >> >> I'm not sure how to do this using promises. I'd need to add two event listeners at the same time (and not when the first one fires) and ensure that one fires after another. > > I think the current approach is fine. The reject is not going to happen after we have already resolved, so you can just do: > > if (count === 1) { > ... > return; > } > > if (count === 2) { > ... > resolve(); > } > > InspectorTest.fail("Unexpected ReadyStateChanged"); If we don't return after the second if clause, InspectorTest.fail("Unexpected ReadyStateChanged") will still run. I didn't include it in my patch.

Comment on attachment 303391 [details] Patch Clearing flags on attachment: 303391 Committed r213421: <http://trac.webkit.org/changeset/213421>

All reviewed patches have been landed. Closing bug.

<rdar://problem/30454357>

Ping webkit-bug-importer.

*** Bug 165068 has been marked as a duplicate of this bug. ***

*** Bug 34565 has been marked as a duplicate of this bug. ***