Summary: | Web Inspector: Local Overrides: the placeholder for the MIME type, status code, and status text is the same as the placeholder URL | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | 201262 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Devin Rousso
2019-11-18 16:34:07 PST
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. |