Bug 170568 – Web Inspector: WebSockets: frame context menu items should content sniff to be more relevant

• Status: NEW
• Component: Web Inspector

[Blaze Burg]

Currently we have "Log Frame Text" and "Log Frame Value". The latter logs the frame as JSON, whether or not it actually is JSON. We should sniff the values prior to appending the menu items. If it is JSON, have "Log Frame as JSON" and "Log Frame as Text". If it's not JSON, then the option should be "Log Frame Value".

[Blaze Burg]

Since this uses RemoteObject to get the data, this change may require to sniff the content in the model side since we don't want to block showing the context menu on a round trip to get the remote object.

[Radar WebKit Bug Importer]

<rdar://problem/31487006>

[Nikita Vasilyev]

(In reply to Brian Burg from comment #1)
> Since this uses RemoteObject to get the data, this change may require to
> sniff the content in the model side since we don't want to block showing the
> context menu on a round trip to get the remote object.

Are you suggesting to JSON.parse every message when it's added? If so, I'd have to test how much performance penalty it adds for big JSON messages.

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #3)
> (In reply to Brian Burg from comment #1)
> > Since this uses RemoteObject to get the data, this change may require to
> > sniff the content in the model side since we don't want to block showing the
> > context menu on a round trip to get the remote object.
> 
> Are you suggesting to JSON.parse every message when it's added? If so, I'd
> have to test how much performance penalty it adds for big JSON messages.

We already do this when the context menu is triggered.

See the discussion and patches on bug 169945:
https://bugs.webkit.org/show_bug.cgi?id=169945

[Joseph Pecoraro]

(In reply to Brian Burg from comment #0)
> Currently we have "Log Frame Text" and "Log Frame Value". The latter logs
> the frame as JSON, whether or not it actually is JSON.

I'm having trouble understanding this. We do know it is actually JSON, we've done a JSON.parse() on the contents to prove it.

> We should sniff the values prior to appending the menu items. If it is
> JSON, have "Log Frame as JSON" and "Log Frame as Text". If it's not
> JSON, then the option should be "Log Frame Value".

My expectations are:

   (1) if the frame is text => have a Log Text option
   (2) if the text is JSON  => have a Log Value option

which currently matches the code.

Is there a specific case where the context menus were confusing?

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #4)
> (In reply to Nikita Vasilyev from comment #3)
> > (In reply to Brian Burg from comment #1)
> > > Since this uses RemoteObject to get the data, this change may require to
> > > sniff the content in the model side since we don't want to block showing the
> > > context menu on a round trip to get the remote object.
> > 
> > Are you suggesting to JSON.parse every message when it's added? If so, I'd
> > have to test how much performance penalty it adds for big JSON messages.
> 
> We already do this when the context menu is triggered.
> 
> See the discussion and patches on bug 169945:
> https://bugs.webkit.org/show_bug.cgi?id=169945

Correct. We do this when we display the context menu, but not when a message is added.

[Joseph Pecoraro]

> Correct. We do this when we display the context menu, but not when a message
> is added.

And this bug is talking about the context menu. I do not think we would want or need to do this when the message is added. Did you have a specific case in mind?

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #7)
> > Correct. We do this when we display the context menu, but not when a message
> > is added.
> 
> And this bug is talking about the context menu. I do not think we would want
> or need to do this when the message is added. Did you have a specific case
> in mind?

No, I wanted to clarify what Brian meant in comment 1, that's all.

[Blaze Burg]

(In reply to Joseph Pecoraro from comment #5)
> (In reply to Brian Burg from comment #0)
> > Currently we have "Log Frame Text" and "Log Frame Value". The latter logs
> > the frame as JSON, whether or not it actually is JSON.
> 
> I'm having trouble understanding this. We do know it is actually JSON, we've
> done a JSON.parse() on the contents to prove it.
> 
> > We should sniff the values prior to appending the menu items. If it is
> > JSON, have "Log Frame as JSON" and "Log Frame as Text". If it's not
> > JSON, then the option should be "Log Frame Value".
> 
> My expectations are:
> 
>    (1) if the frame is text => have a Log Text option
>    (2) if the text is JSON  => have a Log Value option
> 
> which currently matches the code.
> 
> Is there a specific case where the context menus were confusing?

"Value" is kinda confusing IMO since the text representation *is* the value. as JSON or as String would be clearer to me.