Summary: | Web Inspector: Local Resource Overrides: allow substitution based on a url pattern | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bburg, commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, 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-09-30 15:34:57 PDT
Created attachment 382434 [details]
Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Created attachment 382436 [details]
Patch
Comment on attachment 382436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382436&action=review I have questions about fragment identifier behavior. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:846 > +bool InspectorNetworkAgent::hasIntercept(URL url) ::shouldIntercept ? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:848 > + url.removeFragmentIdentifier(); I wonder if users find it surprising that fragments are silently dropped. Are they also stripped from the displayed URL in the overrides UI? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1044 > + if (!m_intercepts.appendIfNotContains(Intercept { url, isRegex && *isRegex })) Nice. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1045 > + errorString = "Intercept for given url and given isRegex already exists"_s; "Intercept URL or pattern already registered" > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:-1028 > -void InspectorNetworkAgent::removeInterception(ErrorString& errorString, const String& url, const String* networkStageString) Ugh, someday I'll fix the argument types here. > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1061 > + errorString = "Missing intercept for given url and given isRegex"_s; Ditto as above. > Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:170 > + target.NetworkAgent.addInterception.invoke(commandArguments); Nice. > Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:49 > + url: isRegex ? url : WI.urlWithoutFragment(url), I see, we had already stripped any fragment in the provided intercept URL prior to saving it. > Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:70 > + return new WI.LocalResourceOverride(WI.LocalResource.fromJSON(localResource), {isRegex, disabled}); Nice, this should just fallback on older JSONs without any work. > Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:125 > + let regex = new RegExp(this.url, "i"); Does the user know their regex will always run case-insensitively? Maybe worth mentioning in a tips and tricks article. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:153 > + this._urlCodeMirror.setOption("mode", isRegex ? "text/x-regex" : "text/x-local-override-url"); I don't think this will work because you've captured isRegex instead of checking the value of the checkbox (either via the event or by updating the model directly). I would expect the 'change' event listener to set LocalResourceOverride.isRegex, then an event listener for WI.LocalResourceOverride.Event.IsRegexChanged will update CodeMirror mode. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:46 > + return "/" + this.resource.url + "/"; Nit: template literal? > LayoutTests/http/tests/inspector/network/local-resource-override-basic.html:172 > + url: "http://127.0.0.1:8000/inspector/network/resources/override.txt?t=*", so... is this a glob or a real regex? I wouldn't have expected this to work. Comment on attachment 382436 [details]
Patch
Clearing r? since there are open issues.
Comment on attachment 382436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382436&action=review >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:848 >> + url.removeFragmentIdentifier(); > > I wonder if users find it surprising that fragments are silently dropped. Are they also stripped from the displayed URL in the overrides UI? Yeah, we strip it in the frontend. Additionally, there's a special CodeMirror mode `text/x-local-override-url` that changes the text color of any invalid schemes and/or fragments to red so it's very visible. >> Source/WebInspectorUI/UserInterface/Models/LocalResourceOverride.js:125 >> + let regex = new RegExp(this.url, "i"); > > Does the user know their regex will always run case-insensitively? Maybe worth mentioning in a tips and tricks article. Actually, this is my mistake. We should probably provide another option to match case sensitivity, in addition to regular expression. >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:153 >> + this._urlCodeMirror.setOption("mode", isRegex ? "text/x-regex" : "text/x-local-override-url"); > > I don't think this will work because you've captured isRegex instead of checking the value of the checkbox (either via the event or by updating the model directly). > > I would expect the 'change' event listener to set LocalResourceOverride.isRegex, then an event listener for WI.LocalResourceOverride.Event.IsRegexChanged will update CodeMirror mode. Good catch! Created attachment 383198 [details]
Patch
Comment on attachment 383198 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383198&action=review r=me > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideTreeElement.js:49 > + text += "i"; Good attention to detail. Thank you! Comment on attachment 383198 [details] Patch Clearing flags on attachment: 383198 Committed r252614: <https://trac.webkit.org/changeset/252614> All reviewed patches have been landed. Closing bug. |