Bug 89461 - Web Inspector: Design WebSockets panel
Summary: Web Inspector: Design WebSockets panel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-19 05:07 PDT by Nikita Vasilyev
Modified: 2012-07-02 02:51 PDT (History)
14 users (show)

See Also:


Attachments
Mockup (150.09 KB, image/png)
2012-06-19 05:07 PDT, Nikita Vasilyev
no flags Details
WIP (8.06 KB, patch)
2012-06-19 05:08 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
DataGrid (109.43 KB, image/png)
2012-06-20 08:03 PDT, Nikita Vasilyev
no flags Details
DataGrid (9.05 KB, patch)
2012-06-21 05:34 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
Outgoing Arrows (21.56 KB, image/png)
2012-06-21 09:24 PDT, Nikita Vasilyev
no flags Details
Put back Date column, drop Mask column (8.69 KB, patch)
2012-06-25 15:22 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Rebaseline with ToT (9.83 KB, patch)
2012-06-27 04:38 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
Remove Opcode column, put Opcode’s textual description into data column (46.18 KB, image/png)
2012-06-27 07:32 PDT, Nikita Vasilyev
no flags Details
Remove Opcode column (11.69 KB, patch)
2012-06-27 07:34 PDT, Nikita Vasilyev
pfeldman: review-
Details | Formatted Diff | Diff
Opcodes enum, String.sprintf (11.60 KB, patch)
2012-06-29 15:32 PDT, Nikita Vasilyev
pfeldman: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (753.20 KB, application/zip)
2012-06-29 19:11 PDT, WebKit Review Bot
no flags Details
Code style: no indent for `case` (11.80 KB, patch)
2012-06-30 06:32 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2012-06-19 05:07:03 PDT
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.
Comment 1 Nikita Vasilyev 2012-06-19 05:08:08 PDT
Created attachment 148314 [details]
WIP
Comment 2 Konrad Piascik 2012-06-19 06:36:43 PDT
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.
Comment 3 Pavel Feldman 2012-06-19 06:39:11 PDT
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.
Comment 4 Nikita Vasilyev 2012-06-20 04:28:00 PDT
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.
Comment 5 Nikita Vasilyev 2012-06-20 08:03:58 PDT
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.
Comment 6 Konrad Piascik 2012-06-20 08:21:05 PDT
(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.
Comment 7 Nikita Vasilyev 2012-06-21 05:34:23 PDT
Created attachment 148774 [details]
DataGrid
Comment 8 Nikita Vasilyev 2012-06-21 05:37:31 PDT
(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.
Comment 9 dubroy 2012-06-21 05:39:36 PDT
Could you please post a screen shot of the current patch?
Comment 10 Nikita Vasilyev 2012-06-21 05:41:06 PDT
(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
Comment 11 Konrad Piascik 2012-06-21 06:27:11 PDT
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.
Comment 12 Nikita Vasilyev 2012-06-21 07:14:03 PDT
(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".
Comment 13 dubroy 2012-06-21 08:01:59 PDT
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.
Comment 14 Pavel Feldman 2012-06-21 08:24:22 PDT
(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.
Comment 15 Nikita Vasilyev 2012-06-21 09:24:30 PDT
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.
Comment 16 Konrad Piascik 2012-06-21 09:29:07 PDT
(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.
Comment 17 Pavel Feldman 2012-06-21 20:27:47 PDT
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.
Comment 18 Pavel Feldman 2012-06-21 20:36:54 PDT
> 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.
Comment 19 Nikita Vasilyev 2012-06-25 13:49:04 PDT
(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?
Comment 20 Nikita Vasilyev 2012-06-25 15:22:39 PDT
Created attachment 149370 [details]
Put back Date column, drop Mask column
Comment 21 Nikita Vasilyev 2012-06-27 04:38:40 PDT
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.
Comment 22 Nikita Vasilyev 2012-06-27 07:32:35 PDT
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 :)
Comment 23 Nikita Vasilyev 2012-06-27 07:34:46 PDT
Created attachment 149746 [details]
Remove Opcode column
Comment 24 Pavel Feldman 2012-06-29 07:34:51 PDT
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.
Comment 25 Nikita Vasilyev 2012-06-29 15:32:07 PDT
Created attachment 150264 [details]
Opcodes enum, String.sprintf

All done.
Comment 26 WebKit Review Bot 2012-06-29 19:11:16 PDT
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
Comment 27 WebKit Review Bot 2012-06-29 19:11:21 PDT
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
Comment 28 Pavel Feldman 2012-06-29 21:24:07 PDT
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.
Comment 29 Nikita Vasilyev 2012-06-30 06:32:58 PDT
Created attachment 150316 [details]
Code style: no indent for `case`
Comment 30 WebKit Review Bot 2012-07-02 02:50:54 PDT
Comment on attachment 150316 [details]
Code style: no indent for `case`

Clearing flags on attachment: 150316

Committed r121666: <http://trac.webkit.org/changeset/121666>
Comment 31 WebKit Review Bot 2012-07-02 02:51:01 PDT
All reviewed patches have been landed.  Closing bug.