Web Inspector: Sources: allow Response Local Overrides to map to a directory on disk #13547
Conversation
|
EWS run on previous version of this PR (hash 4248e9e)
|
|
new dropdown for creating a Local override required inputs for creating a "Directory" Local Override icons used for a "Directory" Local Override and any replaced responses |
dcrousso
changed the title
|
EWS run on previous version of this PR (hash 83a4776)
|
webkit-ews-buildbot
added
the
merging-blocked
dcrousso
removed
the
merging-blocked
|
EWS run on previous version of this PR (hash 0010664)
|
rcaliman-apple
left a comment
rcaliman-apple
left a comment
There was a problem hiding this comment.
This is neat! Thank you for working to implement this, @dcrousso!
I took a while to figure out how local overrides work and the patch seems to do the right things. I say "seems" because I couldn't get the file picker to actually pick a directory even though the right attributes are set on the <input type="file"> and the same code in isolation works fine. Left a comment inline.
I did validate, however, that single file local overrides mapped to a file on disk work as intended and extrapolated from there.
I didn't notice any obvious issues in the code. Though, I'd like to actually validate that the directory file picker works. Can you please verify and let me know if it works on your end?
There was a problem hiding this comment.
What do you think about storing these file paths in <input type="hidden"> and retrieving them from there?
I am acknowledging the duplication.
It's slightly odd that other data is collected from CodeMirror instances, input and select elements, while these file paths are plucked from textContent of elements, something generally used for display purposes.
There was a problem hiding this comment.
the main reason for this is that we're trying to mimic the UI of a <input type="file">, so the textContent is the "what file you have selected" display part. using an <input> for that doesn't really make sense as just that part is not meant to be editable
we have to do this because there's no way to prepopulate an <input type="file"> even if the path to the file is already known, which is required if the developer tries to edit a WI.LocalResourceOverride that already has a _mappedFilePath. so as a result ive mimicked the UI/structure/etc. of an <input type="file"> to allow Web Inspector to give the appearance of the <input type="file"> already having a value in that case instead of "No file(s) selected"
There was a problem hiding this comment.
Understood and agreed. The suggestion was just a nit / preference.
The argument for using an <input type=hidden> in addition to this._mappedFilePathValueElement.textContent was to meet the expectation that the serialized data to configure the LocalResourceOverride is collected from form fields.
It confused me for a bit that the output from the file picker didn't appear to be used anywhere, yet the feature did work (that's because I didn't expect this. _mappedFilePathValueElement.textContent to be used for anything other than display).
There was a problem hiding this comment.
The file picker for directories didn't work on my local build of this PR so I couldn't verify that the patch works as intended (it does work fine for local overrides for single files mapped to local files).
The same code extracted from WI.FileUtilities.import() works fine in a standalone test file; it shows the system file picker with the option to "upload" a directory. But in Web Inspector, it falls back to a single file picker.
I set breakpoints in WI.FileUtilities.import() and can confirm the right webkitdirectory and multiple attributes are set on the <input type="file">.
Am I holding it wrong? 🤔
See this screen recording:
input-file-webkitdirectory-local-override.mp4
There was a problem hiding this comment.
hmmm this was working when i first made this PR 🤔
ill do some digging and see what's up
There was a problem hiding this comment.
seems like i forgot to add one of the changed files when i committed 😅
There was a problem hiding this comment.
Thanks for adding that!
After a build and rebase off of main I can confirm that the local overrides with directory file path mapping works as intended. Nice feature! 👍
|
EWS run on previous version of this PR (hash fec52e0)
|
|
EWS run on current version of this PR (hash a94ef15) |
rcaliman-apple
left a comment
rcaliman-apple
left a comment
There was a problem hiding this comment.
Looks good to me 👍
Verified that the feature works as intended.
aproskuryakov
left a comment
aproskuryakov
left a comment
There was a problem hiding this comment.
rs=me based on Razvan's review and testing.
webkit-commit-queue
force-pushed
the
eng/256427
branch
from
a94ef15 to
1226f34
Compare
…rectory on disk https://bugs.webkit.org/show_bug.cgi?id=256427 Reviewed by Alexey Proskuryakov. This allows for developers who are trying to do the "simple" thing of "I have a local copy of my website on my computer and want to use those files instead" to do it with a single Local Override instead of having to create one for each file they want to replace. * Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js: (WI.LocalResourceOverride.create): (WI.LocalResourceOverride.displayNameForNetworkStageOfType): (WI.LocalResourceOverride.displayNameForType): (WI.LocalResourceOverride.prototype.get networkStage): Added. (WI.LocalResourceOverride.prototype.get displayType): Added. (WI.LocalResourceOverride.prototype.generateSubpathForMappedDirectory): Added. (WI.LocalResourceOverride.prototype.get canMapToFile): Introduce a new `WI.LocalResourceOverride.InterceptType.ResponseMappedDirectory` in order to make the logic of creating a Local Override that's mapped to a directory on disk easier. This is because in order to do directory mapping, we cannot have per-request response MIME type, status code, status text, or headers. Instead, we derive what we can from the ultimate file loaded from disk (see below) and pass through everything else (unless the file on disk doesn't exist, in which case everything is passed through). As such, we also cannot support skipping the network. * Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js: (WI.NetworkManager): (WI.NetworkManager.prototype.async requestIntercepted): (WI.NetworkManager.prototype.async responseIntercepted): (WI.NetworkManager.prototype._commandArgumentsForInterception): (WI.NetworkManager.prototype._handleFrameMainResourceDidChange): When intercepting requests using `WI.LocalResourceOverride.InterceptType.ResponseMappedDirectory`, attempt to load the corresponding file at ``` mappedFilePath + response.url.replace(localResourceOverride.url, localResourceOverride.localResource.url) ``` where the `response.url` is the intercepted URL, the `localResourceOverride.url` is the regex used to define the interception, and `localResourceOverride.localResource.url` is the subpath that uses capture groups from the `localResourceOverride.url`. If no file exists at that location, don't override the response. This allows for very general directory mapping without having to worry about every possible file needing to be defined inside that folder (and subfolders). Drive-by: Fix incorrect `Network.Response.statusCode` to be `Network.Response.status`. * Source/WebInspectorUI/UserInterface/Models/LocalResource.js: (WI.LocalResource): (WI.LocalResource.resetPathsThatFailedToLoadFromFileSystem): Added. (WI.LocalResource.prototype.set mappedFilePath): (WI.LocalResource.prototype.get isMappedToDirectory): Added. (WI.LocalResource.prototype.async requestContentFromMappedDirectory): Added. (WI.LocalResource.prototype.async requestContent): (WI.LocalResource.prototype.async _loadFromFileSystem): (WI.LocalResource.prototype.async _updateContentFromFileSystem): Added. Expose a way to manually load content from a file on disk using the `mappedFilePath` (with a required `subpath` if it's a directory). * Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js: (WI.LocalResourceOverridePopover): (WI.LocalResourceOverridePopover.prototype.get serializedData): (WI.LocalResourceOverridePopover.prototype.show): (WI.LocalResourceOverridePopover.prototype.show.createEditorId): Added. (WI.LocalResourceOverridePopover.prototype.show.updateMappedDirectoryPath): Added. (WI.LocalResourceOverridePopover.prototype.show.updateMappedFilePath): Added. * Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css: (.popover .local-resource-override-popover-content.request .editor:is(.url, .redirect), .popover .local-resource-override-popover-content.response-mapped-directory :is(.editor:is(.url, .mapped-directory-subpath), .mapped-directory-path)): Renamed from `.popover .local-resource-override-popover-content.request .editor:is(.url, .redirect)`. (.popover .local-resource-override-popover-content.block .editor.url, .popover .local-resource-override-popover-content.response :is(.editor.url, .mapped-file-path)): Renamed from `(.popover .local-resource-override-popover-content:is(.response, .block) .editor.url`. (.popover .local-resource-override-popover-content :is(.mapped-directory-path, .mapped-file-path)): Added. (.popover .local-resource-override-popover-content :is(.mapped-directory-path, .mapped-file-path) > .value:not(:empty) + .placeholder): Added. (.popover .local-resource-override-popover-content :is(.mapped-directory-path, .mapped-file-path) > button): Added. Rework the "Type" dropdown to have "Response" be a group name containing "File" and "Directory", the former being the existing "Response" and the latter being the way to create a `WI.LocalResourceOverride.InterceptType.ResponseMappedDirectory`. In order to successfully create a `WI.LocalResourceOverride.InterceptType.ResponseMappedDirectory` we must have a interception regex (with capture groups), a subpath string (using the capture groups), and an input to choose the mapped directory path. Drive-by: Also add an input to choose a mapped file path when creating a "File" Local Override. * Source/WebInspectorUI/UserInterface/Base/Main.js: * Source/WebInspectorUI/UserInterface/Views/ContentView.js: (WI.ContentView.resolvedRepresentedObjectForRepresentedObject): * Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js: (WI.ResourceContentView): (WI.ResourceContentView.prototype.async _handleMapLocalResourceOverrideToFile): Handle the new `WI.LocalResourceOverride.InterceptType.ResponseMappedDirectory`. * Source/WebInspectorUI/UserInterface/Models/Resource.js: (WI.Resource.classNamesForResource): * Source/WebInspectorUI/UserInterface/Views/ResourceTreeElement.js: (WI.ResourceTreeElement.prototype._updateTitles): (WI.ResourceTreeElement.prototype._updateIcon): * Source/WebInspectorUI/UserInterface/Views/ResourceIcons.css: (.resource-icon.override.mapped-file .icon): Added. (body:not(.window-inactive, .window-docked-inactive) :is(.table, .data-grid):focus-within .selected .resource-icon.override.mapped-file .icon, body:not(.window-inactive, .window-docked-inactive) .tree-outline:focus-within .selected.resource-icon.override.mapped-file .icon): Added. (@media (prefers-color-scheme: dark) .resource-icon.override.mapped-file .icon): Added. Add an icon for when the Local Override is mapped to a file/folder. * Source/WebInspectorUI/UserInterface/Base/FileUtilities.js: (WI.FileUtilities.prototype.longestCommonPrefix): Added. (WI.FileUtilities.prototype.import): Add a way to set `webkitdirectory` in order to allow selecting a directory. * Source/WebKit/UIProcess/Inspector/mac/WKInspectorViewController.mm: (-[WKInspectorViewController webView:runOpenPanelWithParameters:initiatedByFrame:completionHandler:]): Allow Web Inspector to upload directories if requested, such as for `<input type="file" multiple webkitdirectory>`. * Source/WebInspectorUI/UserInterface/Images/Disk.svg: Add light and dark variants. * Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js: * LayoutTests/TestExpectations: * LayoutTests/http/tests/inspector/network/local-resource-override-mapped-to-directory.html: Added. * LayoutTests/http/tests/inspector/network/local-resource-override-mapped-to-directory-expected.txt: Added. * LayoutTests/http/tests/inspector/network/local-resource-override-mapped-to-file.html: * LayoutTests/http/tests/inspector/network/local-resource-override-mapped-to-file-expected.txt: * LayoutTests/platform/mac-wk1/TestExpectations: * LayoutTests/platform/mac-wk2/TestExpectations: Canonical link: https://commits.webkit.org/266821@main
webkit-commit-queue
force-pushed
the
eng/256427
branch
from
1226f34 to
d2d000b
Compare
|
Committed 266821@main (d2d000b): https://commits.webkit.org/266821@main Reviewed commits have been landed. Closing PR #13547 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
6 participants
d2d000b
a94ef15