Bug 169011 – Web Inspector: Show individual messages in the content pane for a WebSocket

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

[Nikita Vasilyev]

Once WebSockets show up in the Network pane, clicking on one should show individual messages in the content view.

<rdar://problem/30456092>

[Nikita Vasilyev]

Created attachment 303792 [details]
Patch

[Nikita Vasilyev]

Created attachment 303794 [details]
[Image] websocket.org/echo.html

[Nikita Vasilyev]

Created attachment 303795 [details]
[Image] pywebsocket console page with patch applied

To play around with different message types (text/binary), pywebsocket can be installed locally.

https://github.com/google/pywebsocket/wiki/TestingYourWebSocketImplementation#testing-your-client-implementation-using-standalonepy-in-pywebsocket

[Devin Rousso]

Comment on attachment 303792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303792&action=review

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:216
> +        if (!NetworkAgent.hasEventParameter("webSocketWillSendHandshakeRequest", "walltime")) {

I think a comment explaining why this is necessary would be useful.

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:44
> +    get openTime() { return this._walltime - this._timestamp; }

I think this is not "simple" enough to warrant a single line.  My understanding was that basic return/assignment was the only acceptable situation for single line getters/setters

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:65
> +        let frame = {data, isOutgoing, opcode, timestamp: this.openTime + timestamp};

I think you should rename this "timestamp" key in `frame` to something else, as it seems confusing to have two different values using the same name with different meanings.  From what I understand, `timestamp` is supposed to represent the number of seconds since the socket was opened, so reusing "timestamp" here as the key to mean the UNIX timestamp is weird.  Maybe you should use "walltime" as above instead.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:87
>              // FIXME: <webkit.org/b/169011> Web Inspector: Show individual messages in the content pane for a WebSocket

I think you can remove this FIXME now.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.css:42
> +    padding-left: 18px;

I realize this is aggravating, but since we now are able to work with RTL, I believe we should either add support while doing new features or create bugs explaining what needs to be done to add support.  This seems pretty simple, so it may be worth adding RTL support right now (and for the rest of the file).

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:40
> +        let columns = {data: {}};
> +

NIT: newline should be before `columns`, not after

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:91
> +        let framesLength = this._resource._frames.length;

NIT: You should use the "public" getter instead

this._resource.frames

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:96
> +        for (let index = this._framesRendered; index < framesLength; index++) {
> +            let frame = this._resource._frames[index];
> +            let {data, isOutgoing, opcode, timestamp, elapsedTime} = frame;
> +            this.addFrame(data, isOutgoing, opcode, timestamp, elapsedTime);
> +        }

I think you can make this a for-of loop, and maybe even do the destructure inside the loop statement.

for (let {data, isOutgoing, opcode, timestamp, elapsedTime} of this._resource.frames)

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:119
> +    _textForOpcode(opcode)

NIT: I feel like this should be a static function, similar to what we do with `WebInspector.Resource.displayNameForType`.

[Nikita Vasilyev]

Created attachment 303843 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 303843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303843&action=review

r=me

> Source/JavaScriptCore/inspector/protocol/Network.json:24
> +            "description": "Elapsed time since opened."

This needs units. "Elapsed seconds since frontend connected".

> Source/JavaScriptCore/inspector/protocol/Network.json:273
> +                { "name": "timestamp", "$ref": "Timestamp", "description": "Elapsed time since opened." },

All of these parameters don't even need a description, since they are no different from the types!

> Source/WebCore/ChangeLog:10
> +        No new tests. Tests will be added in a follow up patch.

We should get in the habit of having tests as soon as possible!

> Source/WebInspectorUI/ChangeLog:26
> +        Open time is the time when Web Inspector was opened.

This doesn't sound right.

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:216
> +        // When remote debugging an older versions of iOS, walltime parameter isn't present.

Use the "// COMPATIBILITY(iOS 10.3): ..." comment style. That makes it very easy for us to find and know when to delete these.

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:218
> +            request = walltime;

Probably easier to read as `request = arguments[2]`.

> Source/WebInspectorUI/UserInterface/Images/ArrowUp.svg:1
> +<?xml version="1.0"?>

What do we do for Images/gtk

Based on the screenshot this icon doesn't really fit in with the rest of our iconography. Compare to other arrows (download arrow, step arrows, gc button arrows, network tab icon arrows). We should file a follow-up to improve on this icon.

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:49
> +    get openTime()
> +    {
> +        return this._walltime - this._timestamp;
> +    }

This seems incorrect to me. Why isn't this just walltime?

Test scenario:

    1. Open a web page
    2. Inspect the page
    3. Wait 1 minute
    4. Create a Web Socket on the page
        => Seems like "openTime" would be 1 minute behind because of this subtraction.

Also I'd suggest adding something like this instead of using "openTime" which is quite vague.

    walltimeForWebSocketTimestamp(timestamp)
    {
        return this._walltime + timestamp;
    }

> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:95
> +    PongFrame: 10

Style: Add the trailing comma to make future additions cleaner.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.css:43
> +.web-socket.content-view > .data-grid .data-column > div {
> +    padding-left: 18px;
> +}

This likely needs to be fixed for RTL. See recent patches. You can use the Web Inspector Settings pane to force changing the direction.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:36
> +        this._showTimeColumn = !isNaN(resource.walltime);

I'd prefer if we had the COMPATIBILITY comment here so that we know eventually we can remove this sometime in the future when dropping legacy backends:

    // COMPATIBILITY (iOS 10.3): `walltime` did not exist in 10.3 and earlier.
    this._showTimeColumn = NetworkAgent.hasEventParameter("webSocketWillSendHandshakeRequest", "walltime");

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:53
> +        this._addRow(WebInspector.UIString("WebSocket connection established"), this._resource.walltime, ["non-text-frame"]);

The casing of this string does not match the casing of other strings. I'm not sure what would be better, each word capitalized or sentence structure. But they should all be consistent.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:64
> +        const opCodesDescription = {
> +            [WebInspector.WebSocketResource.OpCodes.ContinuationFrame]: WebInspector.UIString("Continuation Frame"),
> +            [WebInspector.WebSocketResource.OpCodes.TextFrame]: WebInspector.UIString("Text Frame"),

Style: I think a switch would be clearer.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:127
> +        if (classNames && classNames.length > 0)
> +            node.element.classList.add(...classNames);

classNames is guaranteed by all users of this private function so you can drop the if check.

> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:133
> +    _timeStringFromTimestamp(timestamp)
> +    {
> +        return new Date(timestamp * 1000).toLocaleTimeString();
> +    }

I think we will want to show at least seconds. Minute granularity is not nearly as useful as seconds.

[Nikita Vasilyev]

Comment on attachment 303843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303843&action=review

>> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:49
>> +    }
> 
> This seems incorrect to me. Why isn't this just walltime?
> 
> Test scenario:
> 
>     1. Open a web page
>     2. Inspect the page
>     3. Wait 1 minute
>     4. Create a Web Socket on the page
>         => Seems like "openTime" would be 1 minute behind because of this subtraction.
> 
> Also I'd suggest adding something like this instead of using "openTime" which is quite vague.
> 
>     walltimeForWebSocketTimestamp(timestamp)
>     {
>         return this._walltime + timestamp;
>     }

This scenario works as expected. webSocketWillSendHandshakeRequest's `timestamp` is elapsed seconds since Web Inspector was opened. It's the same for every NetworkObserver#webSocket* method, in fact.

If I just use walltime (instead of `this._walltime - this._timestamp`) for your given scenario the time would be showing as 1 minute ahead of what it supposed to be.

I don't mind adding walltimeForWebSocketTimestamp method. However, it would have to be something like this:

    walltimeForWebSocketTimestamp(timestamp)
    {
        return this._walltime - this._timestamp + timestamp;
    }

[Joseph Pecoraro]

(In reply to comment #7)
> Comment on attachment 303843 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303843&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:49
> >> +    }
> > 
> > This seems incorrect to me. Why isn't this just walltime?
> > 
> > Test scenario:
> > 
> >     1. Open a web page
> >     2. Inspect the page
> >     3. Wait 1 minute
> >     4. Create a Web Socket on the page
> >         => Seems like "openTime" would be 1 minute behind because of this subtraction.
> > 
> > Also I'd suggest adding something like this instead of using "openTime" which is quite vague.
> > 
> >     walltimeForWebSocketTimestamp(timestamp)
> >     {
> >         return this._walltime + timestamp;
> >     }
> 
> This scenario works as expected. webSocketWillSendHandshakeRequest's
> `timestamp` is elapsed seconds since Web Inspector was opened. It's the same
> for every NetworkObserver#webSocket* method, in fact.
> 
> If I just use walltime (instead of `this._walltime - this._timestamp`) for
> your given scenario the time would be showing as 1 minute ahead of what it
> supposed to be.

Okay, then I do not understand why. Can you explain why?

[Nikita Vasilyev]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 303843 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=303843&action=review
> > 
> > >> Source/WebInspectorUI/UserInterface/Models/WebSocketResource.js:49
> > >> +    }
> > > 
> > > This seems incorrect to me. Why isn't this just walltime?
> > > 
> > > Test scenario:
> > > 
> > >     1. Open a web page
> > >     2. Inspect the page
> > >     3. Wait 1 minute
> > >     4. Create a Web Socket on the page
> > >         => Seems like "openTime" would be 1 minute behind because of this subtraction.
> > > 
> > > Also I'd suggest adding something like this instead of using "openTime" which is quite vague.
> > > 
> > >     walltimeForWebSocketTimestamp(timestamp)
> > >     {
> > >         return this._walltime + timestamp;
> > >     }
> > 
> > This scenario works as expected. webSocketWillSendHandshakeRequest's
> > `timestamp` is elapsed seconds since Web Inspector was opened. It's the same
> > for every NetworkObserver#webSocket* method, in fact.
> > 
> > If I just use walltime (instead of `this._walltime - this._timestamp`) for
> > your given scenario the time would be showing as 1 minute ahead of what it
> > supposed to be.
> 
> Okay, then I do not understand why. Can you explain why?

Say, I opened Web Inspector at 1:00pm.
I waited for 1 minute, and opened a Web Socket connection at 1:01pm.

    m_frontendDispatcher->webSocketWillSendHandshakeRequest(IdentifiersFactory::requestId(identifier), timestamp(), currentTime(), WTFMove(requestObject));

timestamp() here should be 60 (it's in seconds).
currentTime() is a number of seconds for 1:01pm, NOT 1:00pm.

Say, I received a WebSocket frame at 1:02pm.
timestamp() here should be 120 (two minutes since Web Inspector opened).

How do I get 1:02pm?
walltime (currentTime() we sent) - webSocketWillSendHandshakeRequest's timestamp + frame's timestamp

[Nikita Vasilyev]

Comment on attachment 303843 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303843&action=review

>> Source/WebInspectorUI/UserInterface/Views/WebSocketContentView.js:133
>> +    }
> 
> I think we will want to show at least seconds. Minute granularity is not nearly as useful as seconds.

We do show seconds! The format is hh:mm:ss, e.g. 1:12:21 AM.

[Nikita Vasilyev]

Created attachment 303966 [details]
Patch

[Joseph Pecoraro]

> m_frontendDispatcher->webSocketWillSendHandshakeRequest(IdentifiersFactory::
> requestId(identifier), timestamp(), currentTime(), WTFMove(requestObject));
> 
> timestamp() here should be 60 (it's in seconds).
> currentTime() is a number of seconds for 1:01pm, NOT 1:00pm.
> 
> Say, I received a WebSocket frame at 1:02pm.
> timestamp() here should be 120 (two minutes since Web Inspector opened).
> 
> How do I get 1:02pm?
> walltime (currentTime() we sent) - webSocketWillSendHandshakeRequest's
> timestamp + frame's timestamp

I see, thanks for explaining.

I was confused by the order of operations and the "openTime" name.

Writing it like this would make it clearer to me:

    walltimeForWebSocketTimestamp(timestamp)
    {
        return this._walltime + (timestamp - this._timestamp);
    }

[Nikita Vasilyev]

Created attachment 303974 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 303974 [details]
Patch

Clearing flags on attachment: 303974

Committed r213666: <http://trac.webkit.org/changeset/213666>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.