Bug 122898 – REGRESSION (Safari 6): Web Inspector: JSON may not be pretty printed if served as text/html

• Component: Web Inspector
• Status: RESOLVED FIXED

[Dave Coffin]

JSON is not available to be pretty printed if its MIME type is text/html.

[Radar WebKit Bug Importer]

<rdar://problem/15241419>

[Antoine Quint]

Not sure this is actually a bug, the Web Inspector relies on mime-types to identify what the resource type is. In other words, content served as text/html is treated as HTML and not JSON.

[Dave Coffin]

I take your point, but this is an example of a significant disconnect from the old web inspector. The old inspector ignored the MIME Type, so if your web app had incorrect MIME Types, you could still debug until you fixed it (because it would pretty print was is obviously JSON). I don't see a downside to pretty printing regardless of the MIME Type, and in my case it would be pretty helpful, and I'm probably not the only one. This particular issue would be a reason why I would not switch back to Safari from Chrome (I inspect JSON API responses all day long), to which your point would be "Fix your headers", and my rebuttal is that its never that simple. 

Especially if you're using a public API and don't have control over the content type headers.

[Antoine Quint]

Thanks for pointing this out Dave.

[Timothy Hatcher]

An option to fixing this would be to allow the user to change/force a different MIME-type in the UI, which will affect the syntax highlighting and in turn the pretty printing.

That would be useful for subtle dialects like SASS or LESS that might get the wrong MIME-type on the server.

The next obvious approach (which Dave mentions) would be to second guess the server MIME-type and sniff the content to make the guess about the correct MIME-type.

I think we should consider doing both of these things (user selectable type in the sidebar and content sniffing for obvious content types).

[Dave Coffin]

That'd be awesome Tim.

[Timothy Hatcher]

The same issue happens with Request Data. If the mime-type is application/x-www-form-urlencoded we will not show the data in a content view, but try to parse the key/values pairs. You should be able to force the request and/or response content-type.

[Timothy Hatcher]

See bug 123026.

[Timothy Hatcher]

*** Bug 123026 has been marked as a duplicate of this bug. ***

[Devin Rousso]

Created attachment 375778 [details]
Patch

[Blaze Burg]

Comment on attachment 375778 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:21
> +        (WI.ResourceClusterContentView.prototype._canShowCustomResponseContentView):

Per other comments it's not really clear what the design is here. Is it supporting lazy loading of the content view?

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:54
> +        this._customRequestPathComponent = createPathComponent.call(this, WI.UIString("Custom"), WI.ResourceClusterContentView.RequestIconStyleClassName, WI.ResourceClusterContentView.CustomRequestIdentifier);

Can you please add a UIString label/description? "Custom" is quite vague.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:132
> +        if (!this._customResponseContentView) {

I strongly prefer holding onto the constructor rather than a new-instance-returning lambda that needs to be null'd out. What is the rationale for this change?

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:355
> +        if (this._resource.requestData.isJSON()) {

Please invert this to:

if (!this._resource.requestData.isJSON())
return;

Then you can remove if (!this._customRequestContentViewInitializer) later.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:372
> +        if (this._canShowCustomResponseContentView())

This code does not read well, or it is a bug. If we can't show a custom response view, then execution continues to set it up. Huh? Is this actually something like this._readyToShowCustomResponseContentView?

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:380
> +            if (error || this._canShowCustomResponseContentView())

Ditto. This seems like a bug or misnamed.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:386
> +            if (content.isJSON()) {

content could be undefined here for a page like about:blank. Thus throwing an exception:

[Error] Unhandled Promise Rejection: TypeError: undefined is not an object (evaluating 'content.isJSON')
	(anonymous function) (ResourceClusterContentView.js:386)
	promiseReactionJob

[Devin Rousso]

Comment on attachment 375778 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:21
>> +        (WI.ResourceClusterContentView.prototype._canShowCustomResponseContentView):
> 
> Per other comments it's not really clear what the design is here. Is it supporting lazy loading of the content view?

Lazy loading, but not in the "traditional" sense.  The main issue is that given a `WI.Resource`, we don't know (at construction time) the contents of the `WI.Resource`, as the only way to retrieve that is via a `Promise` (async).  Showing the request as JSON can be synchronous, as `WI.Resource.prototype.requestData` is known (and retrievable) from the moment the `WI.Resource` is constructed, so perhaps that could remove much of the `try*`/`can*`.  I wanted it to be consistent, but frankly it doesn't have to be.

>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:54
>> +        this._customRequestPathComponent = createPathComponent.call(this, WI.UIString("Custom"), WI.ResourceClusterContentView.RequestIconStyleClassName, WI.ResourceClusterContentView.CustomRequestIdentifier);
> 
> Can you please add a UIString label/description? "Custom" is quite vague.

This is overridden later on, so the user would never actually see it.  It probably shouldn't even be a `WI.UIString`.

>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:132
>> +        if (!this._customResponseContentView) {
> 
> I strongly prefer holding onto the constructor rather than a new-instance-returning lambda that needs to be null'd out. What is the rationale for this change?

Since I'm using `WI.JSONContentView` for both the request data (sync retrieval) and response content (async retrieval), I need a way of abstracting _how_ the JSON string is passed into the `WI.JSONContentView` when it's constructed.  This approach allows the `WI.JSONContentView` to be lazily created _after_ all of the data has been retrieved in _both_ cases.

>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:372
>> +        if (this._canShowCustomResponseContentView())
> 
> This code does not read well, or it is a bug. If we can't show a custom response view, then execution continues to set it up. Huh? Is this actually something like this._readyToShowCustomResponseContentView?

If we already can show a custom response content view, we don't need to try to "enable" the custom content view as it's already been "enabled", since we can show it.

The reason it uses "enable" (instead of "create" or "prepare") is because it's inside this function that the path components ("Request JSON" and "Response JSON") are added and made selectable and that's the only way to actually "show" the custom content view.

>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:386
>> +            if (content.isJSON()) {
> 
> content could be undefined here for a page like about:blank. Thus throwing an exception:
> 
> [Error] Unhandled Promise Rejection: TypeError: undefined is not an object (evaluating 'content.isJSON')
> 	(anonymous function) (ResourceClusterContentView.js:386)
> 	promiseReactionJob

Good catch!

[Devin Rousso]

Comment on attachment 375778 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:132
>>> +        if (!this._customResponseContentView) {
>> 
>> I strongly prefer holding onto the constructor rather than a new-instance-returning lambda that needs to be null'd out. What is the rationale for this change?
> 
> Since I'm using `WI.JSONContentView` for both the request data (sync retrieval) and response content (async retrieval), I need a way of abstracting _how_ the JSON string is passed into the `WI.JSONContentView` when it's constructed.  This approach allows the `WI.JSONContentView` to be lazily created _after_ all of the data has been retrieved in _both_ cases.

One other thing to mention is that we don't actually want/need to create the content view until the path component is selected, but we still need to know what to create when it is selected.

[Devin Rousso]

Created attachment 375857 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 375857 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:747
> +Object.defineProperty(String.prototype, "isJSON",
> +{
> +    value()
> +    {
> +        try {
> +            JSON.parse(this);
> +            return true;
> +        } catch { }
> +        return false;
> +    }
> +});

I wonder if we should have a variant that checks for types.

For example these are all true:

    `"this is the data"`.isJSON()
    `true`.isJSON()
    `false`.isJSON()
    `100`.isJSON()
    `null`.isJSON()
    ...

I think this ContentView case we probably would desire a JSON view if the data is an "object" or "array" JSON type:

    `100`.isJSON(["object", "array"]) // => false

That still doesn't do well on the `null` case.

So perhaps some code to work with the JSON to check its type / operate on it further:

    `100`.asJSON((json) => {
        ... // only called if text was parsed to json object
    });

For now it's fine because we don't do anything with it other than Boolean logic but I'm thinking we could improve this.

------

Side note: what does it look like if the response data is just `true`?

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:-277
> -        if (!contentViewToShow)
> -            contentViewToShow = this.responseContentView;

I find this easier to read, and safer, than the `default:` and chain...

I also think that we should put a comment about the switch to point out that this is expected to fall all the way through to the bottom, in case someone adds a new case in the future.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:345
> +        if (!this._resource.requestData.isJSON())
> +            return;

Currently the only custom content view is JSON. But we could add something in the future.

In such a future we probably wouldn't use early returns and instead a chain:

    if (this._canUseCustomJSONContentView(requestData)) {
        ...
        return;
    }

    if (this._canUseCustomMultiPartContentView(requestData) {
        ...;
        return;
    }

Okay to leave as is for now though.

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:364
> +            if (error || !content || !content.isJSON())
> +                return;

Same thing here with the early isJSON bail.

[Joseph Pecoraro]

Comment on attachment 375778 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:355
>> +        if (this._resource.requestData.isJSON()) {
> 
> Please invert this to:
> 
> if (!this._resource.requestData.isJSON())
> return;
> 
> Then you can remove if (!this._customRequestContentViewInitializer) later.

I didn't see this comment when I did my review.

This is an ideal case for a set of chains in case we support various custom views in the future and want to check if this satisfies each.

[Devin Rousso]

Created attachment 375957 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375957 [details]
Patch

Clearing flags on attachment: 375957

Committed r248485: <https://trac.webkit.org/changeset/248485>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.