Bug 158938 - Web Inspector: preview content view for MIME type application/json should be a collapsible tree outline
Summary: Web Inspector: preview content view for MIME type application/json should be ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-20 06:12 PDT by lepolt
Modified: 2017-10-20 22:00 PDT (History)
6 users (show)

See Also:


Attachments
Chrome vs. Safari (393.82 KB, image/png)
2016-06-20 06:12 PDT, lepolt
no flags Details
[IMAGE] Text - Default view is still the response text (130.51 KB, image/png)
2017-10-03 15:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] JSON - Initial View of a JSON Response (130.51 KB, image/png)
2017-10-03 15:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] JSON - Expanded View (535.18 KB, image/png)
2017-10-03 15:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] JSON - Expanded More (409.95 KB, image/png)
2017-10-03 15:50 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (20.36 KB, patch)
2017-10-03 15:54 PDT, Joseph Pecoraro
mattbaker: review-
Details | Formatted Diff | Diff
[IMAGE] Text - Default view is still the response text (266.72 KB, image/png)
2017-10-03 15:56 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (20.22 KB, patch)
2017-10-20 20:56 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lepolt 2016-06-20 06:12:23 PDT
Created attachment 281647 [details]
Chrome vs. Safari

Would like to see better formatting in the Network tab with JSON responses. Currently the payload is displayed as indented text, which can be cumbersome and difficult to parse especially if the response is a list of items. The dev tools in Chrome do a better job of handling this situation. See attached. Chrome displays each item in the list individually, so it's easy to drill down and find a specific record.
Comment 1 Radar WebKit Bug Importer 2016-06-20 06:13:28 PDT
<rdar://problem/26891128>
Comment 2 Joseph Pecoraro 2017-10-03 15:30:44 PDT
<rdar://problem/11279262>
Comment 3 Joseph Pecoraro 2017-10-03 15:49:30 PDT
Created attachment 322595 [details]
[IMAGE] Text - Default view is still the response text
Comment 4 Joseph Pecoraro 2017-10-03 15:49:48 PDT
Created attachment 322596 [details]
[IMAGE] JSON - Initial View of a JSON Response
Comment 5 Joseph Pecoraro 2017-10-03 15:50:07 PDT
Created attachment 322597 [details]
[IMAGE] JSON - Expanded View
Comment 6 Joseph Pecoraro 2017-10-03 15:50:28 PDT
Created attachment 322598 [details]
[IMAGE] JSON - Expanded More
Comment 7 Joseph Pecoraro 2017-10-03 15:54:20 PDT
Created attachment 322599 [details]
[PATCH] Proposed Fix
Comment 8 Joseph Pecoraro 2017-10-03 15:56:31 PDT
Comment on attachment 322595 [details]
[IMAGE] Text - Default view is still the response text

Oops this was clearly the wrong image!
Comment 9 Joseph Pecoraro 2017-10-03 15:56:57 PDT
Created attachment 322602 [details]
[IMAGE] Text - Default view is still the response text
Comment 10 Matt Baker 2017-10-03 16:14:13 PDT
Comment on attachment 322599 [details]
[PATCH] Proposed Fix

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

r=me, with minor nits.

> Source/WebInspectorUI/ChangeLog:23
> +        JSON view evaluates the json content on the page and shows an ObjectTree

Nit: json -> JSON

> Source/WebInspectorUI/ChangeLog:42
> +        In this case the only one we have is a JSON response can get a JSON view.

Nit: the second sentence is somewhat awkward.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:131
> +    get customResponseContentView()

This getter could be simplified a little:

get customResponseContentView()
{
    if (!this._canShowCustomResponseContentView())
        return null;

    if (!this._customResponseContentView)
        this._customResponseContentView = new this._customResponseContentViewConstructor(this._resource);

    return this._customResponseContentView;
}
Comment 11 Matt Baker 2017-10-03 16:28:18 PDT
Comment on attachment 322599 [details]
[PATCH] Proposed Fix

Okay I can reproduce this fairly often now:

1. Goto theverge.com
2. Open Network tab (legacy view)
3. Filter by XHRs, bring up a content view for one of the JSON resources
4. Change view to JSON
5. Click one of the other tree elements
  => Content view does not change

Debugging the Inspector shows that tree selection handler is bailing out early at NetworkGridContentView.js:393.
Comment 12 Matt Baker 2017-10-03 16:29:47 PDT
(In reply to Matt Baker from comment #11)
> Comment on attachment 322599 [details]
> [PATCH] Proposed Fix
> 
> Okay I can reproduce this fairly often now:
> 
> 1. Goto theverge.com
> 2. Open Network tab (legacy view)
> 3. Filter by XHRs, bring up a content view for one of the JSON resources
> 4. Change view to JSON
> 5. Click one of the other tree elements
>   => Content view does not change
> 
> Debugging the Inspector shows that tree selection handler is bailing out
> early at NetworkGridContentView.js:393.

Sometimes it is necessary to do steps 1-4, reload the page, and then repeat the steps again to trigger the bug.
Comment 13 Joseph Pecoraro 2017-10-04 11:21:36 PDT
I see, it may be worth just waiting until the legacy network tab goes away instead of spending time to change its behavior.
Comment 14 Joseph Pecoraro 2017-10-20 20:56:11 PDT
Created attachment 324484 [details]
[PATCH] Proposed Fix

Rebaselined. The legacy network tab doesn't exist anymore so its bugs are no longer relevant and don't need to be worked around!
Comment 15 BJ Burg 2017-10-20 21:36:13 PDT
Comment on attachment 324484 [details]
[PATCH] Proposed Fix

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

r=me

Do we want JSON trees in the request as well, if they payload contains JSON-like data? (This would be a followup patch)

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:509
> +localizedStrings["JSON"] = "JSON";

I don't think this is localizable.

> Source/WebInspectorUI/UserInterface/Views/JSONResourceContentView.js:47
> +            JSON.parse(content);

This all seems rather duplicative, but I guess it's fine?
Comment 16 WebKit Commit Bot 2017-10-20 22:00:03 PDT
Comment on attachment 324484 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 324484

Committed r223806: <https://trac.webkit.org/changeset/223806>
Comment 17 WebKit Commit Bot 2017-10-20 22:00:05 PDT
All reviewed patches have been landed.  Closing bug.