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

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied 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 375778: Patch

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




--- Comment #11 from Brian Burg <bburg at apple.com> ---
Comment on attachment 375778
  --> https://bugs.webkit.org/attachment.cgi?id=375778
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