# STEPS TO REPRODUCE: 1. inspect any page 2. click the + in the bottom of the navigation sidebar of the Sources Tab 3. select "Local Override..." 4. delete the contents of the MIME type, status code, or status text inputs => URL is shown, instead of the default value for those inputs
Created attachment 383807 [details] Patch
Created attachment 383808 [details] [Image] After Patch is applied
Comment on attachment 383807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383807&action=review Some questions. > Source/WebInspectorUI/ChangeLog:14 > + (.popover .local-resource-override-popover-content .data-grid tr.header-content-type > :matches(.name-column, .value-column)): Added. > + Replace the hardcoded `placeholder` with an optional `options` object that can include a The user experience of only placeholders seems weird. I'd prefer it include values where it has values. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:52 > - let url = this._urlCodeMirror.getValue(); > + let url = this._urlCodeMirror.getValue() || this._urlCodeMirror.getOption("placeholder"); > if (!url) > return null; Not allowing a path and having it fallback to the placeholder seems bad. In the other cases it seems fine because those are reasonable default values, but this one ending in "/path" seems dumb. I think we should continue to disallow an empty url. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:119 > if (!mimeType) > - mimeType = "text/javascript"; > + mimeType = "text/plain"; I'd rather we prefer the default be "text/javascript" since text files are very rare on web pages. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:336 > - value, > + ...options, Why did we get rid of the initial value? value: value Unless I'm missing something, this doesn't seem preferable to setting this value because you can't edit / copy the placeholder. If you want, maybe the URL field could start with an empty value but have a placeholder to force the user to type in a URL, but that seems weird because it requires typing a full URL path when we could have pre-generated part of it.
Comment on attachment 383807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383807&action=review >> Source/WebInspectorUI/ChangeLog:14 >> + Replace the hardcoded `placeholder` with an optional `options` object that can include a > > The user experience of only placeholders seems weird. I'd prefer it include values where it has values. I think we should do this if we're given a resource to copy/override, but not if we're creating an "empty" local override. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:52 >> return null; > > Not allowing a path and having it fallback to the placeholder seems bad. In the other cases it seems fine because those are reasonable default values, but this one ending in "/path" seems dumb. > > I think we should continue to disallow an empty url. That makes sense. Agreed. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:119 >> + mimeType = "text/plain"; > > I'd rather we prefer the default be "text/javascript" since text files are very rare on web pages. I feel like that's a bit "biased" towards users mostly using this to override JavaScript vs other types (e.g. CSS, HTML, etc.), but I don't particularly care that much. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:336 >> + ...options, > > Why did we get rid of the initial value? > > value: value > > Unless I'm missing something, this doesn't seem preferable to setting this value because you can't edit / copy the placeholder. > > If you want, maybe the URL field could start with an empty value but have a placeholder to force the user to type in a URL, but that seems weird because it requires typing a full URL path when we could have pre-generated part of it. We didn't so much "get rid of it" as we allow the caller to decide whether they want to supply it or not. As I wrote above, I think it makes a lot of sense to set a value when we know there is a value (e.g. overriding an existing resource), but if we're creating a brand new local override I think having them be placeholders is more "inviting" in that it encourages the user to fill in their own value.
Created attachment 383833 [details] Patch
Comment on attachment 383833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383833&action=review rs=me > Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:37 > + this._selectable = selectable !== undefined ? selectable : true; > + this._copyable = copyable !== undefined ? copyable : true; > + this._editable = editable !== undefined ? editable : true; Can we use nullish operators now? selectable ?? true
Comment on attachment 383833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383833&action=review >> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:37 >> + this._editable = editable !== undefined ? editable : true; > > Can we use nullish operators now? > > selectable ?? true I'd rather wait for nullish coalescing to reach Stage 4 before starting to use it. This would be a perfect use case though :)
Comment on attachment 383833 [details] Patch Clearing flags on attachment: 383833 Committed r252652: <https://trac.webkit.org/changeset/252652>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57338064>