It's strange that we don't have this.
Ping for import.
<rdar://problem/26516684>
Created attachment 286022 [details] Patch
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?
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
Created attachment 286025 [details] Patch
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.
Created attachment 286032 [details] Patch
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.
Created attachment 286051 [details] Patch
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.
Created attachment 286284 [details] Patch
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.
(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.
Created attachment 286681 [details] Patch
Created attachment 286685 [details] Patch
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.
Created attachment 286714 [details] Patch
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".
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.
Created attachment 286756 [details] Patch
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.
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.
Created attachment 286775 [details] Patch
Comment on attachment 286775 [details] Patch Clearing flags on attachment: 286775 Committed r204862: <http://trac.webkit.org/changeset/204862>
All reviewed patches have been landed. Closing bug.