Bug 170568 - Web Inspector: WebSockets: frame context menu items should content sniff to be more relevant
Summary: Web Inspector: WebSockets: frame context menu items should content sniff to b...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-06 14:34 PDT by Brian Burg
Modified: 2017-04-06 15:22 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-04-06 14:34:14 PDT
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".
Comment 1 Brian Burg 2017-04-06 14:35:34 PDT
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.
Comment 2 Radar WebKit Bug Importer 2017-04-06 14:35:53 PDT
<rdar://problem/31487006>
Comment 3 Nikita Vasilyev 2017-04-06 14:44:54 PDT
(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.
Comment 4 Joseph Pecoraro 2017-04-06 14:54:47 PDT
(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
Comment 5 Joseph Pecoraro 2017-04-06 14:57:28 PDT
(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?
Comment 6 Nikita Vasilyev 2017-04-06 15:02:17 PDT
(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.
Comment 7 Joseph Pecoraro 2017-04-06 15:05:05 PDT
> 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?
Comment 8 Nikita Vasilyev 2017-04-06 15:18:36 PDT
(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.
Comment 9 Brian Burg 2017-04-06 15:22:04 PDT
(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.