Web Inspector: CSP request payload on medium.com is unreadable, should be pretty-printed

[BJ Burg]

It appears to be JSON-like, but it's formatted on a single line and pretty printing is not available.

[Radar WebKit Bug Importer]

<rdar://problem/45090894>

[BJ Burg]

Created attachment 351779 [details]
[SCREENSHOT]

[Joseph Pecoraro]

This may be because it is the Request data.

[Joseph Pecoraro]

The request content view is created as a TextContentView:

    this._requestContentView = new WI.TextContentView(this._resource.requestData || "", this._resource.requestDataContentType);

In particular here the request is a Beacon, with request data. I'm unable to reproduce it in particular, but I can create a test page.

[Devin Rousso]

A test page would be very helpful.  I cannot reproduce the CSP request shown in attachment 351779 [details].

[Joseph Pecoraro]

You can just do a fetch to a server with JSON data without specifying a JSON mime type to reproduce:

    fetch("data.json?" + Math.random(), {
        method: "POST",
        // headers: {"Content-Type": "application/json"},
        body: `{"a":1,"b":2,"c":3,"d":4,"e":5,"f":6}`,
    });

Indeed it was the case that a non-JSON/JavaScript mime type we don't attempt to format. I suppose we can, and I believe we did that in the past for text that might be JSON like.

Heck we could just attempt a JSON.parse() of the contents to decide whether or not to enable the format button.

[Devin Rousso]

Created attachment 351946 [details]
Patch

[BJ Burg]

Comment on attachment 351946 [details]
Patch

I don't like the data flow here. The patch pretends the content is formattable but canBeFormatted() is still false. Then it adds a new event that is only used in this one case for one subclass. It would make more sense, IMO, to alter TextEditor.canBeFormatted() to return true in this case. Then call TextEditor._format(true), which will pretty-print the text again, fire WI.TextEditor.Event.FormattingDidChange, and that will cause the button to be updated appropriately.

[Joseph Pecoraro]

(In reply to Brian Burg from comment #8)
> Comment on attachment 351946 [details]
> Patch
> 
> I don't like the data flow here. The patch pretends the content is
> formattable but canBeFormatted() is still false. Then it adds a new event
> that is only used in this one case for one subclass. It would make more
> sense, IMO, to alter TextEditor.canBeFormatted() to return true in this
> case. Then call TextEditor._format(true), which will pretty-print the text
> again, fire WI.TextEditor.Event.FormattingDidChange, and that will cause the
> button to be updated appropriately.

I agree. I would expect canBeFormatted to return true after determining the content can be formatted. I also don't like the idea of a separate event for this special case.

[Joseph Pecoraro]

Comment on attachment 351946 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:128
> +                if (!this.canBeFormatted())
> +                    this._attemptToFormatAsJavaScript();

Lets treat this as a one-time auto-detect a better mode.

On initial content population if the CodeMirror mode is null then we can attempt to determine a better mode (for example trying to detect if the content looks like JavaScript, or CSS, etc) and if successful change the mimeType and dispatch an event that the MIME Type changes.  Changing TextEditor.mimeType gets the CodeMirror mode and syntax highlighting. The event allows ContentView to update their UI such as the buttons since canBeFormatted() should now return true.

This seems like a useful thing in general for TextEditor to do and something we might allow users to explicit do later (explicitly select a mimeType / mode for a resource) and the same event would be useful then.

[Devin Rousso]

Created attachment 351985 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 351985 [details]
Patch

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

We could try to write a test for this. We don't have any TextEditor tests right now. I'm not sure how tangled of a mess that will be considering this is View code. But I could imagine just creating a TextEditor, populating it, and watching for the event + mime type change.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:144
> +                if (this._codeMirror.getMode().name === "null")

Lets give this a comment. Maybe something like:

    // Attempt to determine a better mode / mime type for this content
    // to enable syntax highlighting and formatting features.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:903
> +            this.mimeType = "application/javascript";

Because this is asynchronous, I don't think we will auto-format content that is lazily detected as JavaScript/JSON. We may want to consider that here, since we know that _attemptToDetermineMIMEType is done on initial content load, we could attempt to auto format here.

[Devin Rousso]

Created attachment 353046 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353046 [details]
Patch

Clearing flags on attachment: 353046

Committed r237396: <https://trac.webkit.org/changeset/237396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.