[webkit-reviews] review granted: [Bug 122898] REGRESSION (Safari 6): Web Inspector: JSON may not be pretty printed if served as text/html : [Attachment 375857] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 122898: REGRESSION (Safari 6): Web Inspector: JSON may not be pretty
printed if served as text/html
https://bugs.webkit.org/show_bug.cgi?id=122898

Attachment 375857: Patch

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




--- Comment #15 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375857
  --> https://bugs.webkit.org/attachment.cgi?id=375857
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.