Bug 190354 - Web Inspector: CSP request payload on medium.com is unreadable, should be pretty-printed
Summary: Web Inspector: CSP request payload on medium.com is unreadable, should be pre...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL: https://www.medium.com
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-08 09:46 PDT by BJ Burg
Modified: 2018-10-24 12:27 PDT (History)
6 users (show)

See Also:


Attachments
[SCREENSHOT] (472.03 KB, image/png)
2018-10-08 09:47 PDT, BJ Burg
no flags Details
Patch (8.74 KB, patch)
2018-10-09 22:48 PDT, Devin Rousso
bburg: review-
Details | Formatted Diff | Diff
Patch (13.99 KB, patch)
2018-10-10 14:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (14.68 KB, patch)
2018-10-24 11:25 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2018-10-08 09:46:20 PDT
It appears to be JSON-like, but it's formatted on a single line and pretty printing is not available.
Comment 1 Radar WebKit Bug Importer 2018-10-08 09:47:03 PDT
<rdar://problem/45090894>
Comment 2 BJ Burg 2018-10-08 09:47:29 PDT
Created attachment 351779 [details]
[SCREENSHOT]
Comment 3 Joseph Pecoraro 2018-10-08 13:19:27 PDT
This may be because it is the Request data.
Comment 4 Joseph Pecoraro 2018-10-08 13:23:52 PDT
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.
Comment 5 Devin Rousso 2018-10-08 19:56:19 PDT
A test page would be very helpful.  I cannot reproduce the CSP request shown in attachment 351779 [details].
Comment 6 Joseph Pecoraro 2018-10-08 23:35:21 PDT
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.
Comment 7 Devin Rousso 2018-10-09 22:48:56 PDT
Created attachment 351946 [details]
Patch
Comment 8 BJ Burg 2018-10-10 12:07:14 PDT
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.
Comment 9 Joseph Pecoraro 2018-10-10 13:06:04 PDT
(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.
Comment 10 Joseph Pecoraro 2018-10-10 13:59:07 PDT
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.
Comment 11 Devin Rousso 2018-10-10 14:32:15 PDT
Created attachment 351985 [details]
Patch
Comment 12 Joseph Pecoraro 2018-10-10 15:03:23 PDT
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.
Comment 13 Devin Rousso 2018-10-24 11:25:41 PDT
Created attachment 353046 [details]
Patch
Comment 14 WebKit Commit Bot 2018-10-24 12:27:34 PDT
Comment on attachment 353046 [details]
Patch

Clearing flags on attachment: 353046

Committed r237396: <https://trac.webkit.org/changeset/237396>
Comment 15 WebKit Commit Bot 2018-10-24 12:27:36 PDT
All reviewed patches have been landed.  Closing bug.