Bug 89461 – Web Inspector: Design WebSockets panel

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Nikita Vasilyev]

Created attachment 148313 [details]
Mockup

I’ve been using WebSockets inspection for some time and I’d like to propose some changes. 99% of the time I only need to see the incoming/outgoing data. That’s all. The current UI cluttered with low-level details which I never use. The most significant column—data column—is the last.

Time. Do you remember the last time you needed an exact time of a request? I do and I just used console.log(Date.now()).

OpCode. http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-17#section-11.8
How often do you need it? For me It’s "1" (text frame) all the time. When Opcode is different from 1 we might want to display it in the data column. Otherwise just don’t show it.

Mask. It seems to be always false for incoming data and true for incoming.

Length. Do you remember the last time you needed an exact length? I don’t, but even if I did I would just get if from the data.

I’m open to discussion.

[Nikita Vasilyev]

Created attachment 148314 [details]
WIP

[Konrad Piascik]

Even if not everyone wants to see all the details, I'd rather no remove them entirely.  We could have the extra data hidden or shown based on a setting instead, with a default to show all.

[Pavel Feldman]

Suggested decorations looks weird to me. Could we use color coding (light green / red background colors) instead? I would also format the data as a table (DataGrid) and would leave the metainformation columns. You could reorder the columns so that the data comes first.

[Nikita Vasilyev]

The time. It currently shows the time of opening WebSocket Frames panel instead of the time of receiving/sending data. It’s not only useless but misleading.

[Nikita Vasilyev]

Created attachment 148569 [details]
DataGrid

(In reply to comment #3)
> ... and would leave the metainformation columns.

Even Length? I can hardly imagine how it might be useful.

[Konrad Piascik]

(In reply to comment #5)
> Created an attachment (id=148569) [details]
> DataGrid
> 
> (In reply to comment #3)
> > ... and would leave the metainformation columns.
> 
> Even Length? I can hardly imagine how it might be useful.

Some developers may want to know how much data they're sending back and forth without having to manually count the bytes.

[Nikita Vasilyev]

Created attachment 148774 [details]
DataGrid

[Nikita Vasilyev]

(In reply to comment #3)
> Could we use color coding (light green / red background colors) instead?

I used a light green background for outgoing messages and I kept white background for incoming ones. Red color could be used for errors some later on.

> I would also format the data as a table (DataGrid) and would leave the metainformation columns. You could reorder the columns so that the data comes first.

Done.

Could you please post a screen shot of the current patch?

[Nikita Vasilyev]

(In reply to comment #9)
> Could you please post a screen shot of the current patch?

https://bug-89461-attachments.webkit.org/attachment.cgi?id=148569

[Konrad Piascik]

Comment on attachment 148774 [details]
DataGrid

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

> Source/WebCore/ChangeLog:10
> +        Remove Time column since it shows the time of opening WebSocket Frames panel,

On Chrome Canary I'm seeing the time properly reflected.  I still think the column should be kept and a bug report filed about the time not being reported correctly.  This is especially important since we're getting rid of the message count indicator.  Having a timestamp to track for when the 100 messages limit rolls over is useful imho.

[Nikita Vasilyev]

(In reply to comment #11)
> (From update of attachment 148774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148774&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Remove Time column since it shows the time of opening WebSocket Frames panel,
> 
> On Chrome Canary I'm seeing the time properly reflected.  I still think the column should be kept and a bug report filed about the time not being reported correctly.  This is especially important since we're getting rid of the message count indicator.  Having a timestamp to track for when the 100 messages limit rolls over is useful imho.

I’m sorry. The time property does work properly. I’ll put it back. (I was testing on a dirty repository that was missing `date.setTime(payload.time * 1000)`)

Wouldn’t it be more convenient to use toLocaleTimeString() instead of instead toISOString()? I.e. "18:06:03" instead of "2012-06-21T14:06:03.536Z".

I don't think it's clear that green means sent and white means received.

+1 on a nicer date format.

Re: length, you could try displaying it in parentheses, using a light grey color, immediately after the data. That would free up the space taken up by the column.

If you're going to show the opcode, would it make sense to label it as "Frame type" and put the actual textual description? I.e. "Text", "Continuation", etc.

I think we can probably drop the mask. The spec seems to say just what you did -- always 1 for outgoing, 0 for incoming. It might be useful if the client dropped the connection due to the server sending a masked message, but in that case I think it would be better to show an error icon or something.

[Pavel Feldman]

(In reply to comment #13)
> I don't think it's clear that green means sent and white means received.
> 
> +1 on a nicer date format.
> 

Sure, +1.

> Re: length, you could try displaying it in parentheses, using a light grey color, immediately after the data. That would free up the space taken up by the column.
> 

I am strongly for the length column. In my experience, many packets have 4-6 digit lengths and I can figure out which one i need by seeking through the length column. The content itself is not that valuable to me: we always use our own protocols / methods to serialize the data we send. So all prefixes (these 80+ characters) are going to be pretty much the same (some generic headers in messages). So I'm voting for the
- length column,
- truncated one line data previews with ellipsis by default
- ability to get full data preview upon action

> I think we can probably drop the mask. The spec seems to say just what you did -- always 1 for outgoing, 0 for incoming. It might be useful if the client dropped the connection due to the server sending a masked message, but in that case I think it would be better to show an error icon or something.

Spec has been shaky here: both were optional originally, then outgoing became must-have. I agree that we should remove the column until we hear from the users that they need it.

[Nikita Vasilyev]

Created attachment 148816 [details]
Outgoing Arrows

(In reply to comment #13)
> I don't think it's clear that green means sent and white means received.

This is why I originally made icons. I attached another variation.

[Konrad Piascik]

(In reply to comment #14)
> (In reply to comment #13)
> > I don't think it's clear that green means sent and white means received.
> > 
> > +1 on a nicer date format.
> > 
> 
> Sure, +1.
Ditto
> 
> > Re: length, you could try displaying it in parentheses, using a light grey color, immediately after the data. That would free up the space taken up by the column.
> > 
> 
> I am strongly for the length column. In my experience, many packets have 4-6 digit lengths and I can figure out which one i need by seeking through the length column. The content itself is not that valuable to me: we always use our own protocols / methods to serialize the data we send. So all prefixes (these 80+ characters) are going to be pretty much the same (some generic headers in messages). So I'm voting for the
> - length column,
> - truncated one line data previews with ellipsis by default
> - ability to get full data preview upon action
> 
I also think that the ellipsis and full data upon action is better.

> > I think we can probably drop the mask. The spec seems to say just what you did -- always 1 for outgoing, 0 for incoming. It might be useful if the client dropped the connection due to the server sending a masked message, but in that case I think it would be better to show an error icon or something.
> 
> Spec has been shaky here: both were optional originally, then outgoing became must-have. I agree that we should remove the column until we hear from the users that they need it.
As long as we don't modify what the event sends us I'm OK with removing the mask from the UI, so we can add it in later once the spec decides what it's for.

We can have the OpCode use an enum of sorts but it would just need to be maintained both in WebCore and Web Inspector front-end.  That's why I didn't include it in the initial design.

[Pavel Feldman]

Comment on attachment 148774 [details]
DataGrid

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

r- for the missing time column, otherwise looks good.

> Source/WebCore/inspector/front-end/NetworkItemView.js:46
> +        this.appendTab("webSocketFrames", WebInspector.UIString("WebSocket Frames"), frameView);

I actually think that "Frames" should be sufficient. You should also do "return;" after this line since no further views apply to sockets.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:30
> +    var dataGrid = new WebInspector.DataGrid([

This is a nice refactoring, thank you!

> Source/WebCore/inspector/front-end/networkPanel.css:308
> +    border-bottom: 1px solid #EEE;

we typically use rgb() notation for colors.

[Pavel Feldman]

> This is why I originally made icons. I attached another variation.

I don't like the new image - it looks alien. I suggested color coding becase wireshark is using it extensively. Google wireshark for images to see the examples. It seems pretty standard.

Charles proxy does not have a similar view to incoming and outgoing packets, so there is nothing similar.

[Nikita Vasilyev]

(In reply to comment #17)
> > Source/WebCore/inspector/front-end/NetworkItemView.js:46
> > +        this.appendTab("webSocketFrames", WebInspector.UIString("WebSocket Frames"), frameView);
> 
> I actually think that "Frames" should be sufficient.

Should I also remove "WebSocket Frames" from English.lproj/localizedStrings.js?

[Nikita Vasilyev]

Created attachment 149370 [details]
Put back Date column, drop Mask column

[Nikita Vasilyev]

Created attachment 149728 [details]
Rebaseline with ToT

(In reply to comment #13)
> If you're going to show the opcode, would it make sense to label it as "Frame type" and put the actual textual description? I.e. "Text", "Continuation", etc.

I will try it out in a separate bug.

(In reply to comment #14)
> I am strongly for the length column. In my experience, many packets have 4-6 digit lengths and I can figure out which one i need by seeking through the length column. The content itself is not that valuable to me: we always use our own protocols / methods to serialize the data we send. So all prefixes (these 80+ characters) are going to be pretty much the same (some generic headers in messages). So I'm voting for the
> - length column,
> - truncated one line data previews with ellipsis by default
> - ability to get full data preview upon action

Ditto.

[Nikita Vasilyev]

Created attachment 149745 [details]
Remove Opcode column, put Opcode’s textual description into data column

(In reply to comment #21)
> (In reply to comment #13)
> > If you're going to show the opcode, would it make sense to label it as "Frame type" and put the actual textual description? I.e. "Text", "Continuation", etc.
> 
> I will try it out in a separate bug.

Made it here :)

[Nikita Vasilyev]

Created attachment 149746 [details]
Remove Opcode column

[Pavel Feldman]

Comment on attachment 149746 [details]
Remove Opcode column

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

> Source/WebCore/ChangeLog:6
> +        Use DataGrid to display the data.

I'd keep these details more dense (i.e. no blank line separators)

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:58
> +            row.data = WebInspector.ResourceWebSocketFrameView.OpCodes[payload.opcode] + " (Opcode " + payload.opcode + (payload.mask ? ", mask" : "") + ")";

You should use formatting methods on String.prototype for that (sprintf, etc.). You also want to add new strings to the English.lproj.

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:72
> +    0: "Continuation Frame",

I'd rather use a standard programming pattern for implementing it, i.e.

WebInspector.ResourceWebSocketFrameView.OpCodes = {
    ContinuationFrame: 0,
   ...
}

I would use it everywhere in the logic code and would choose the localized string using "switch" upon UI creation.

[Nikita Vasilyev]

Created attachment 150264 [details]
Opcodes enum, String.sprintf

All done.

[WebKit Review Bot]

Comment on attachment 150264 [details]
Opcodes enum, String.sprintf

Attachment 150264 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13114554

New failing tests:
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html

[WebKit Review Bot]

Created attachment 150290 [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

[Pavel Feldman]

Comment on attachment 150264 [details]
Opcodes enum, String.sprintf

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

Looks good! Please fix the indent and I will cq+ it!

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:60
> +                case WebInspector.ResourceWebSocketFrameView.OpCodes.ContinuationFrame:

nit: WebKit does not indent case:

> Source/WebCore/inspector/front-end/ResourceWebSocketFrameView.js:76
> +            row.data = String.sprintf("%s (Opcode %d%s)", opcodeMeaning, payload.opcode, (payload.mask ? ", mask" : ""));

Nit: You might want to do WebInspector.UIString("%s (Opcode %d%s)", ...) and put %s (Opcode %d%s) into the localized strings.

[Nikita Vasilyev]

Created attachment 150316 [details]
Code style: no indent for `case`

[WebKit Review Bot]

Comment on attachment 150316 [details]
Code style: no indent for `case`

Clearing flags on attachment: 150316

Committed r121666: <http://trac.webkit.org/changeset/121666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.