Bug 198029

Created attachment 370231 [details] Patch

Created attachment 370232 [details] [Image] With patch applied

Comment on attachment 370231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370231&action=review > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:40 > + if (resource.url === "about:blank") { This feels like a hack. Do you know what the protocol error actually is? Could we modify the backend to treat this as an empty resource content instead of an error? Does this also need to be done for any `TextEditor` subclasses?

(In reply to Devin Rousso from comment #3) > Comment on attachment 370231 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370231&action=review > > > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:40 > > + if (resource.url === "about:blank") { > > This feels like a hack. Do you know what the protocol error actually is? Resource.js: requestContentFromBackend() { ... // There is no request identifier or frame to request content from. if (this._parentFrame) return PageAgent.getResourceContent(this._parentFrame.id, this._url); This results in the following error: column: 29 line: 158 message: "No resource with given URL found" sourceURL: "file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/Connection.js" stack: "_dispatchResponseToPromise@file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Proto…"

Comment on attachment 370231 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370231&action=review >>> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:40 >>> + if (resource.url === "about:blank") { >> >> This feels like a hack. Do you know what the protocol error actually is? Could we modify the backend to treat this as an empty resource content instead of an error? >> >> Does this also need to be done for any `TextEditor` subclasses? > > Resource.js: > > requestContentFromBackend() > { > ... > // There is no request identifier or frame to request content from. > if (this._parentFrame) > return PageAgent.getResourceContent(this._parentFrame.id, this._url); > > This results in the following error: > > column: 29 > line: 158 > message: "No resource with given URL found" > sourceURL: "file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/Connection.js" > stack: "_dispatchResponseToPromise@file:///Volumes/.../WebKitBuild/Release/WebInspectorUI.framework/Resources/Proto…" Ah, well I guess that makes sense. > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:42 > + return; I personally think we should avoid having early-returns in constructors, as that's kinda funky. It won't "mess up" the `ResourceContentView` (see <https://tc39.es/ecma262/#sec-ecmascript-function-objects-construct-argumentslist-newtarget>), but it's just an odd thing to see. It also won't prevent subclasses from executing, which may have issues (e.g. `TextResourceContentView` creates a `SourceCodeTextEditor`, which also calls `requestContent`). It may be easier/simpler to have `requestContent`/`requestContentFromBackend` have a special case for <about:blank> (but only if the resource is a `Resource.Type.Document`). We should also update `WI.appendContextMenuItemsForSourceCode` so that it doesn't provide a "Save File" in this case.

Comment on attachment 370231 [details] Patch r- until my comments/concerns are addressed

Created attachment 372981 [details] Patch

Created attachment 372982 [details] [Image] With patch applied

Created attachment 372985 [details] Patch

(In reply to Devin Rousso from comment #5) > It may be easier/simpler to have > `requestContent`/`requestContentFromBackend` have a special case for > <about:blank> (but only if the resource is a `Resource.Type.Document`). Somehow, about:blank iframes (without srcdoc) appear as `Resource.Type.Other`. You could see on the screenshot that it has a different icon in the navigation sidebar.

Created attachment 372989 [details] Patch Whoops, this wasn't my latest patch.

Comment on attachment 372989 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372989&action=review r-, as I don't agree with creating a special message just for <about:blank>. We should use one of the other paths for empty resources that don't error. > Source/WebInspectorUI/UserInterface/Models/Resource.js:825 > + message: WI.unlocalizedString("about:blank") Rather than pass along a special message, I think I'd rather just see either "Resource has no content" (from `ResourceContentView.prototype.showGenericNoContentMessage`) or just an empty text editor. It seems weird to have a special case message just for <about:blank>. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:69 > + if (sourceCodeOrLocation.url !== "about:blank") { If we override the content for <about:blank> resources to be "", then we should actually be able to save those files, as there is actual content (albeit just "") to save. As such, this restriction is not necessary.

Created attachment 373590 [details] Patch

Comment on attachment 373590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373590&action=review r-, due to the following: > Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:139 > + this.showGenericNoContentMessage(); After talking with @Joe, we agreed that it actually probably should just show "about:blank" as a message. The reasoning for this is that <about:blank> is this kinda special "resource" that doesn't actually go over the network, but has this "implicit" concept of "this is an empty resource". Along these lines, we also agreed that resources requested over the network that have no content (including <data:text/html,>) should show as an empty text editor, not "Resource has no content". The only time that we should show that message (if ever) is if somehow we no longer have the content (e.g. the resource cache ran out of room). Sorry for the back and forth :(

(In reply to Devin Rousso from comment #14) > ... > Along these lines, we also agreed that resources requested over the network > that have no content (including <data:text/html,>) should show as an empty > text editor, not "Resource has no content". I think it's fine to show an empty text editor. It especially makes sense if the resource is editable (e.g. CSS). "Resource has no content" does however make it crystal clear that there's no content here. If I see an empty text editor, I'd have to attempt to scroll it and select all text to see if there's white space somewhere.

(In reply to Nikita Vasilyev from comment #15) > (In reply to Devin Rousso from comment #14) > > ... > > Along these lines, we also agreed that resources requested over the network that have no content (including <data:text/html,>) should show as an empty text editor, not "Resource has no content". > > I think it's fine to show an empty text editor. It especially makes sense if the resource is editable (e.g. CSS). Agreed. I think I actually made a change like this somewhat recently, but I can't remember 🤔 > "Resource has no content" does however make it crystal clear that there's no content here. If I see an empty text editor, I'd have to attempt to scroll it and select all text to see if there's white space somewhere. You could also tell by looking to see if the file has any line numbers in it.

Created attachment 373900 [details] Patch

Comment on attachment 373900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373900&action=review > Source/WebInspectorUI/UserInterface/Models/Resource.js:820 > requestContentFromBackend() What about the other subclasses of `WI.SourceCode`? I realize that it's probably extremely rare for someone to `<script src="about:blank"></script>`, but it's still something we should be able to handle.

Comment on attachment 373900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373900&action=review >> Source/WebInspectorUI/UserInterface/Models/Resource.js:820 >> requestContentFromBackend() > > What about the other subclasses of `WI.SourceCode`? I realize that it's probably extremely rare for someone to `<script src="about:blank"></script>`, but it's still something we should be able to handle. `<script src="about:blank"></script>` displays an empty text editor, same as for about:blank iframe. I don't know why would somebody use about:blank script.

Comment on attachment 373900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373900&action=review >>> Source/WebInspectorUI/UserInterface/Models/Resource.js:820 >>> requestContentFromBackend() >> >> What about the other subclasses of `WI.SourceCode`? I realize that it's probably extremely rare for someone to `<script src="about:blank"></script>`, but it's still something we should be able to handle. > > `<script src="about:blank"></script>` displays an empty text editor, same as for about:blank iframe. > I don't know why would somebody use about:blank script. I'm suggesting that we make `<script src="about:blank"></script>` "special", just like we did for <iframe>, in that it would show a message "about:blank" instead of an empty text editor. For resources that _do_ go over the network (e.g. not "about:blank") and have no content, we _should_ show an empty content view (which is what this patch does right now, so that's awesome =D ). As mentioned in comment 14, "about:blank" is somewhat special, in that it doesn't go over the network and effectively _can't_ have any content whatsoever. As such, I believe that anywhere we could show a content view for an "about:blank" resource, we should instead show an "about:blank" message, not an empty content view. Personally, I'd go about doing this by having `WI.SourceCode` have a `specialContentForURL` method, that contains this code, and have each of the other `requestContentFromBackend` methods call it to check. SourceCode.js ```js generateSpecialContentForURL(url) { if (this._url === "about:blank") { return Promise.resolve({ content: "", message: WI.unlocalizedString("about:blank") }); } return null; } ``` {subclasses of `WI.SourceCode`}.js ```js let specialContentForURLPromise = this.generateSpecialContentForURL(this._url); if (specialContentForURLPromise) return specialContentForURLPromise; ... ```

Created attachment 374642 [details] Patch

Comment on attachment 374642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374642&action=review r=me > Source/WebInspectorUI/ChangeLog:20 > + * UserInterface/Views/TextResourceContentView.js: I think you mean `SourceCodeTextEditor.js`?

(In reply to Devin Rousso from comment #22) > Comment on attachment 374642 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=374642&action=review > > r=me > > > Source/WebInspectorUI/ChangeLog:20 > > + * UserInterface/Views/TextResourceContentView.js: > > I think you mean `SourceCodeTextEditor.js`? I uploaded the old changelog file 🤦‍♂️

Created attachment 374709 [details] Patch

Comment on attachment 374709 [details] Patch Clearing flags on attachment: 374709 Committed r247747: <https://trac.webkit.org/changeset/247747>

All reviewed patches have been landed. Closing bug.