[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 bugzilla-daemon at webkit.org
Fri Aug 9 14:03:57 PDT 2019

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

Attachment 375857: Patch


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

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


> 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()

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

    `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)) {

    if (this._canUseCustomMultiPartContentView(requestData) {

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.

More information about the webkit-reviews mailing list