Bug 167520 – Web Inspector: Show Web Socket connections in Network tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
1. Go to http://www.websocket.org/echo.html
2. Open Network tab in Web Inspector
3. Click "Connect" button to establish a Web Socket connection

Expected:
A new item for a Web Socket connection appears in the Network Tab.

Actual:
Nothing changes in Network tab.

Notes:
Web Inspector even has a filter for "Sockets" in Network's left sidebar, which never shows anything.

[Nikita Vasilyev]

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.

[Blaze Burg]

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)

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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).

[Devin Rousso]

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

[Devin Rousso]

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

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

Created attachment 301851 [details]
Patch

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

(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

[Nikita Vasilyev]

Created attachment 302065 [details]
Patch

Rebaselined after Bug 168549 - Web Inspector: Use Maps in FrameResourceManager instead of objects.

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 303030 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Nikita Vasilyev]

(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.

[Joseph Pecoraro]

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
> [phone]  Nikita Vasilyev  <[e-mail]>

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: ===

[Joseph Pecoraro]

> 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 ]

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

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?

[Nikita Vasilyev]

Created attachment 303177 [details]
WIP

I addressed issues found in the review but there're no new tests yet.

[Nikita Vasilyev]

Created attachment 303366 [details]
Patch

This patch has tests for client and server connection termination. More tests will be added in patches to follow.

[Nikita Vasilyev]

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.

[Build Bot]

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

[Build Bot]

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

[Nikita Vasilyev]

Created attachment 303374 [details]
Patch

Could be an unrelated flaky test, uploading the same patch again.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Comment on attachment 303374 [details]
Patch

See my earlier comments. Yes, the failure is unrelated.

[Nikita Vasilyev]

Created attachment 303391 [details]
Patch

[Nikita Vasilyev]

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.

[WebKit Commit Bot]

Comment on attachment 303391 [details]
Patch

Clearing flags on attachment: 303391

Committed r213421: <http://trac.webkit.org/changeset/213421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

<rdar://problem/30454357>

[Nikita Vasilyev]

Ping webkit-bug-importer.

[Nikita Vasilyev]

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

[Nikita Vasilyev]

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