now that we have `Network.interceptRequestWithError`, we should have UI to use it
Created attachment 458183 [details] Patch
Created attachment 458184 [details] [Image] after Patch is applied
Comment on attachment 458183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458183&action=review > Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:1 > +<?xml version="1.0" encoding="utf-8"?> This isn't needed. > Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:3 > +<svg xmlns="http://www.w3.org/2000/svg" id="root" version="1.1" viewBox="0 0 16 16"> version="1.1" and id="root" aren't needed. > Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:4 > + <defs> You don't need a <defs>, just make style a direct child. > Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:20 > + <symbol id="i" viewBox="0 0 16 16" stroke="none" stroke-width="1" fill="none"> stroke="var(--stroke)" and then you should be able to remove the stroke attributes on the paths and circle. > Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:26 > + <circle fill="none" stroke="var(--stroke)" cx="8" cy="8" r="7.5"/> > + <path fill="none" stroke="var(--stroke)" d="M 5.5 1.0 L 5.5 10.5"/> > + <path fill="none" stroke="var(--stroke)" d="M 3.0 9.0 L 5.5 11.0 L 8.0 9.0"/> > + <path fill="none" stroke="var(--stroke)" d="M 10.5 15.0 L 10.5 5.5"/> > + <path fill="none" stroke="var(--stroke)" d="M 13.0 7.0 L 10.5 5.0 L 8.0 7.0"/> > + <path fill="none" stroke="var(--stroke)" d="M 13.3 2.7 L 2.7 13.3"/> You should be able to remove fill="none" since it's set on the parent. > Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:30 > + <g id="light"><use href="#i"/></g> > + <g id="dark"><use href="#i"/></g> <use href="#i" id="light"/> <use href="#i" id="dark"/> and `use:not(:target)` above
Comment on attachment 458183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458183&action=review r=me neat stuff! > Source/WebCore/ChangeLog:31 > + Add a `virtual` method so that `InspectorNetworkAgent` can log to the (correct) console. Just to make sure I understand correctly, by "correct" you mean the console associated with Web Inspector where the Block override is configured? > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1318 > + auto resourceErrorType = ResourceError::Type::Null; > + switch (protocolResourceErrorType) { Might make more sense for this switch statement to be its own static method since it is just here to convert from a protocol value to a backend value now. > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:527 > + requestURLRow.element.hidden = !isRequest || isBlock; Is `|| isBlock` actually necessary? Shouldn't just making sure it isn't a request cover any case where it would be block? > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:529 > + methodRowElement.hidden = !isRequest || isBlock; Ditto :527 > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverrideRequestContentView.js:75 > + message = document.createDocumentFragment(); Nit: I know we don't declare this here, but it still might be nice for this assignment to be down just before :94 where it is used.
Comment on attachment 458183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458183&action=review >> Source/WebCore/ChangeLog:31 >> + Add a `virtual` method so that `InspectorNetworkAgent` can log to the (correct) console. > > Just to make sure I understand correctly, by "correct" you mean the console associated with Web Inspector where the Block override is configured? i meant it more as "if the request comes from a page, use the `Document`. if the request comes from a `Worker`, use the `WorkerOrWorkletGlobalScope`. etc." i think the word "correct" is kinda unnecessary/confusing, so i'll just remove it :) >> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:1318 >> + switch (protocolResourceErrorType) { > > Might make more sense for this switch statement to be its own static method since it is just here to convert from a protocol value to a backend value now. heh i was going back and forth about this. i'll separate it out >> Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:1 >> +<?xml version="1.0" encoding="utf-8"?> > > This isn't needed. we do this for all the other icons in Web Inspector, so I'd rather have a separate change that removes them all then rather than having a one-off here (in case this does break something a rollout is much easier) >> Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:20 >> + <symbol id="i" viewBox="0 0 16 16" stroke="none" stroke-width="1" fill="none"> > > stroke="var(--stroke)" and then you should be able to remove the stroke attributes on the paths and circle. ooo good call! =D >> Source/WebInspectorUI/UserInterface/Images/SkipNetwork.svg:30 >> + <g id="dark"><use href="#i"/></g> > > <use href="#i" id="light"/> > <use href="#i" id="dark"/> > > and `use:not(:target)` above in this case, yes, but we generally try to keep the overall structure of our icons the same. we use `<g>` because icons might have more than one `<use>` (see `Source/WebInspectorUI/UserInterface/Images/DocumentIcons.svg`) >> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:527 >> + requestURLRow.element.hidden = !isRequest || isBlock; > > Is `|| isBlock` actually necessary? Shouldn't just making sure it isn't a request cover any case where it would be block? oops i actually meant to change the `!isRequest` to `isResponse` i'm trying to change from these negative checks to more positive ones (see `Source/WebInspectorUI/ChangeLog` for more explanation)
Comment on attachment 458183 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458183&action=review >>> Source/WebCore/ChangeLog:31 >>> + Add a `virtual` method so that `InspectorNetworkAgent` can log to the (correct) console. >> >> Just to make sure I understand correctly, by "correct" you mean the console associated with Web Inspector where the Block override is configured? > > i meant it more as "if the request comes from a page, use the `Document`. if the request comes from a `Worker`, use the `WorkerOrWorkletGlobalScope`. etc." > > i think the word "correct" is kinda unnecessary/confusing, so i'll just remove it :) Or keep the word "correct" and say these same words to make clear what the correct console is for different contexts. >>> Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.js:527 >>> + requestURLRow.element.hidden = !isRequest || isBlock; >> >> Is `|| isBlock` actually necessary? Shouldn't just making sure it isn't a request cover any case where it would be block? > > oops i actually meant to change the `!isRequest` to `isResponse` > > i'm trying to change from these negative checks to more positive ones (see `Source/WebInspectorUI/ChangeLog` for more explanation) Even better!
Created attachment 458307 [details] Patch
Created attachment 458317 [details] Patch
Committed r293409 (249975@main): <https://commits.webkit.org/249975@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458317 [details].
<rdar://problem/92311892>