Bug 239674 – Web Inspector: add UI for blocking requests

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

now that we have `Network.interceptRequestWithError`, we should have UI to use it

[Devin Rousso]

Created attachment 458183 [details]
Patch

[Devin Rousso]

Created attachment 458184 [details]
[Image] after Patch is applied

[Tim Nguyen (:ntim)]

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

[Patrick Angle]

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.

[Devin Rousso]

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)

[Patrick Angle]

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!

[Devin Rousso]

Created attachment 458307 [details]
Patch

[Devin Rousso]

Created attachment 458317 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/92311892>