Bug 158035 – Web Inspector: resource tree elements should provide "Download File" context menu items

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

It's strange that we don't have this.

[Blaze Burg]

Ping for import.

[Radar WebKit Bug Importer]

<rdar://problem/26516684>

[Devin Rousso]

Created attachment 286022 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 286022 [details]
Patch

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

Works as described!

You should add "Download File" to localizedStrings.js.

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:201
> +            return;

Why is this needed?

[Devin Rousso]

Comment on attachment 286022 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:201
>> +            return;
> 
> Why is this needed?

I don't actually think it is needed.  I think I just copied more than I should have :P

[Devin Rousso]

Created attachment 286025 [details]
Patch

[Matt Baker]

Comment on attachment 286025 [details]
Patch

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

In the Resources tab, the main resource tree element doesn't have a context menu.

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:94
> +        this.element.addEventListener("contextmenu", this._handleContextMenuEvent.bind(this));

It would be nice to add this to ResourceTimelineDataGridNode too, so it would work for the entire row (and in the Network timeline view).

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:202
> +        contextMenu.appendItem(WebInspector.UIString("Download File"), () => {

Maybe we should use "Save File" instead, since this also operates on local files. Chrome uses "Save".

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:205
> +                content: this._resource.content

WebInspector.saveDataToFile asserts for blob content (image files), doesn't save:

Main.js:806:
console.assert(typeof saveData.content === "string");

FWIW, Chrome dev tools doesn't show a context menu item for blob file types.

[Devin Rousso]

Created attachment 286032 [details]
Patch

[Matt Baker]

Comment on attachment 286032 [details]
Patch

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

This still needs to work for frame resources in the Resources tab. It looks like we don't get a context menu b/c FrameTreeElement overrides TreeElement.onattach, and skips the base class (intentionally, according to comment).

> Source/WebInspectorUI/ChangeLog:7
> +

Add a short description. I'd mention that this is for text resources only.

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:209
> +        }

It would be nice to break all this out into a helper function that both the data grid node and tree element use.

[Devin Rousso]

Created attachment 286051 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 286051 [details]
Patch

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

r- if we can't handle images. We should be able to write some tests too.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:685
> +    isTextWithUrl() {
> +        return this.url && typeof this._resource.content === "string";
> +    }

This function looks dangerous. The contents of resources (any WebInspector.SourceCode) are downloaded asynchronously, so a text file might not have been downloaded yet and so could fail this "content" check.

I'd expect something more like:

    canDownloadResource()
    {
        // Is there any reason we wouldn't want to download?
        return true;
    }

> Source/WebInspectorUI/UserInterface/Models/Resource.js:696
> +    saveContentsToFile()
> +    {
> +        if (!this.isTextWithUrl())
> +            return;
> +
> +        WebInspector.saveDataToFile({
> +            url: this.url,
> +            content: this.content
> +        });
> +    }

We should be able to handle binary data just fine! WebInspector.saveDataToFile calls InspectorFrontendHost.save currently always passes "false" for the base64 parameter. However, if hooked up properly it looks like it should just work. Implementations of InspectorFrontendHost.save looks like they would be able to handle converting base64 data to binary data when saving, and the filename should do the rest. For example WebInspectorProxy::platformSave in Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm.

This should be testable!

Also, the URL we are passing in seems poor. The URL is the suggested save dialog URL to use, which doesn't make much sense with the full URL of the resource. Other ContentViews get the display name which roughly matches the last path component. For example:

    get saveData()
    {
        var url = "web-inspector:///" + encodeURI(WebInspector.UIString("Untitled")) + ".txt";
        return {url, content: this._textEditor.string, forceSaveAs: true};
    }

Maybe we should do something like that here, using the URL's last path component.

And finally, to handle any resource, this should download the contents if it hasn't already downloaded the contents.

[Devin Rousso]

Created attachment 286284 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 286284 [details]
Patch

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

r- for a few things that should be easy to update. Also some open questions.

> Source/WebInspectorUI/ChangeLog:9
> +        Add a context menu item to elements representing text resources that
> +        allows initiates a download of that resource.

Grammaro: "allows initiates a"
Nit: This says "text resources" but supports non-text. Needs an update.

> Source/WebInspectorUI/UserInterface/Base/Main.js:813
> +    let url = parseURL(saveData.url).lastPathComponent;

Nit: This variable name is misleading. It should be lastPathComponent.

> Source/WebInspectorUI/UserInterface/Base/Main.js:815
> +    if (typeof saveData.content === "string") {

What does this do for a data URL? For example the data URL resources on:
<http://dopiaza.org/tools/datauri/examples/index.php>

I suspect the content in that case will be a string. Does this properly save an image, or a text file? And what is the suggested save name?

> Source/WebInspectorUI/UserInterface/Base/Main.js:823
> +        let base64 = parseDataURL(fileReader.result);

Nit: This variable name is poor. It should be something like dataURLComponents.

> Source/WebInspectorUI/UserInterface/Base/Main.js:824
> +        InspectorFrontendHost.save(url || "data", base64.data, true, forceSaveAs || saveData.forceSaveAs);

Nit: The `true` and `false` in these call sites should be nicely named locals, like: `const base64Encoded = false;`

I feel like there should be a better suggested name than "data", heh. Maybe we need a mapping from common mime types to extensions? E.g. "image/jpeg" => "jpg" to better support some cases like dataURLs?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:686
> +    saveContentsToFile()
> +    {
> +        this.requestContent().then(() => {
> +            WebInspector.saveDataToFile({

It feels like a layering violation to have the Resource model object know about WebInspector.saveDataToFile. Lets do this at the callsites.

> Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:161
> +    _handleContextMenuEvent(event)

Ugh seems we should have some shared place that adds context menu items for a resource. Having this code duplicated (and different, since one has cURL the other doesn't) kinda stinks.

[Devin Rousso]

(In reply to comment #13)
> > Source/WebInspectorUI/UserInterface/Base/Main.js:815
> > +    if (typeof saveData.content === "string") {
> 
> What does this do for a data URL? For example the data URL resources on:
> <http://dopiaza.org/tools/datauri/examples/index.php>
> 
> I suspect the content in that case will be a string. Does this properly save
> an image, or a text file? And what is the suggested save name?

Yes it works for both data objects.  It suggests "null" for both since there is no lastPathComponent.

> > Source/WebInspectorUI/UserInterface/Base/Main.js:824
> > +        InspectorFrontendHost.save(url || "data", base64.data, true, forceSaveAs || saveData.forceSaveAs);
> 
> I feel like there should be a better suggested name than "data", heh. Maybe
> we need a mapping from common mime types to extensions? E.g. "image/jpeg" =>
> "jpg" to better support some cases like dataURLs?

Good idea (☞ﾟヮﾟ)☞

> > Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js:161
> > +    _handleContextMenuEvent(event)
> 
> Ugh seems we should have some shared place that adds context menu items for
> a resource. Having this code duplicated (and different, since one has cURL
> the other doesn't) kinda stinks.

Other than creating a utility/static function somewhere that adds these items to a context menu, I'm not really sure how to solve this.  ¯\_(ツ)_/¯  I do agree that it is annoying though.

[Devin Rousso]

Created attachment 286681 [details]
Patch

[Devin Rousso]

Created attachment 286685 [details]
Patch

[WebKit Commit Bot]

Attachment 286685 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devin Rousso]

Created attachment 286714 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 286714 [details]
Patch

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

r=me, but you may want to put up a follow-up to fix the file extension handling for data URLs. At least file a bugzilla bug for it if you do not address it.

> Source/WebInspectorUI/UserInterface/Base/Main.js:817
> +        let dataURLTypeMatch = /^data:([^\/]+)\/([^;]+)/.exec(saveData.url);
> +        if (dataURLTypeMatch)
> +            suggestedName = `${dataURLTypeMatch[1]}.${dataURLTypeMatch[2]}`;

This might work okay for image types, but it won't work well for text mime types:

    text/plain
    text/javascript
    application/javascript

Turns out we already have a reasonable mapping from File Extension to MIME Type in MIMETypeUtilities.js. Can we flip that and create a WebInspector.fileExtensionForMIMEType(mimeType)?

> Source/WebInspectorUI/UserInterface/Base/Main.js:819
> +            suggestedName = "untitled";

Nit: On macOS the default is normally capitalized, "Untitled".

[Joseph Pecoraro]

Comment on attachment 286714 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:819
>> +            suggestedName = "untitled";
> 
> Nit: On macOS the default is normally capitalized, "Untitled".

And maybe this should be localized.

[Devin Rousso]

Created attachment 286756 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 286756 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/MIMETypeUtilities.js:82
> +WebInspector.fileExtensionForMIMEType = function(mimeType)

cq-. Shouldn't include the '.' in all of these. Should just add it at the end if not undefined.

[Joseph Pecoraro]

Comment on attachment 286756 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:819
> +        if (dataURLTypeMatch) {
> +            suggestedName += WebInspector.fileExtensionForMIMEType(dataURLTypeMatch[1]) || "";
> +        }

Style: No braces for single statement blocks.

[Devin Rousso]

Created attachment 286775 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286775 [details]
Patch

Clearing flags on attachment: 286775

Committed r204862: <http://trac.webkit.org/changeset/204862>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.