Bug 213283 – Web Inspector: request / response interception trims resource type

• Component: Web Inspector
• Status: NEW

[Pavel Feldman]

Will add failing test shortly.

[Pavel Feldman]

Created attachment 402079 [details]
Patch

[Pavel Feldman]

Created attachment 402232 [details]
Patch

[Pavel Feldman]

Created attachment 402234 [details]
Patch

[Pavel Feldman]

Created attachment 402247 [details]
Patch

[Pavel Feldman]

Created attachment 402265 [details]
Patch

[Pavel Feldman]

Created attachment 402340 [details]
Patch

[Pavel Feldman]

Created attachment 402359 [details]
Patch

[Devin Rousso]

Comment on attachment 402359 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402359&action=review

What type is shown for cases like `<script src="https://example.org/definitely-not-a-script.html">`?  I would expect it to still have the requester type (e.g. script), which also matches the current functionality AFAICT.

Have you audited all of the places where a `ResourceRequest` is created to see if it should set a `ResourceRequest::Requester`?  Perhaps we should also add a `ResourceRequest::Requester` as a constructor parameter so that future manually created requests have to think about that too.

> Source/WebCore/inspector/NetworkResourcesData.h:71
> +        ResourceRequest::Requester requester() const { return m_requester; }

we should add `#include "ResourceRequest.h"`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:-448
> -        else if (loader && equalIgnoringFragmentIdentifier(request.url(), loader->url()) && !loader->isCommitted())
> -            type = InspectorPageAgent::DocumentResource;
> -        else if (loader) {
> -            for (auto& linkIcon : loader->linkIcons()) {
> -                if (equalIgnoringFragmentIdentifier(request.url(), linkIcon.url)) {
> -                    type = InspectorPageAgent::ImageResource;
> -                    break;
> -                }
> -            }
> -        }

Are these paths also covered by the new logic?

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:216
> +Inspector::Protocol::Page::ResourceType InspectorPageAgent::resourceTypeJSON(ResourceRequest::Requester requester)

We should rename this to something more explicit/understandable, like `InspectorPageAgent::protocolResourceTypeForResourceRequester`.

> Source/WebCore/loader/PingLoader.cpp:211
> +        InspectorInstrumentation::willSendRequest(&frame, identifier, frame.loader().activeDocumentLoader(), request, ResourceResponse());

It seems like not all paths to this function have set a `ResourceRequest::Requester::Ping`.  Are we sure that those cases will still be covered after removing this code?

> Source/WebCore/loader/cache/CachedResource.cpp:311
> +        InspectorInstrumentation::willSendRequest(&frame, identifier, frameLoader.activeDocumentLoader(), request, ResourceResponse());

Do we know with certainty that `ResourceRequest::Requester::Beacon` has been set by this point?

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:203
> +    default:
> +        // Only cherry-pick the ones below, leave the rest unspecified.
> +        return;

NIT: personally, I'd rather not use a `default` and instead list the remaining `CachedResource::Type` values so that future changes must also considered for this codepath

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:31
> +        name: "Network.requestIntercepted.resourceType baseline",

NIT: we usually don't use spaces in test case names, preferring camel casing and periods.

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:37
> +                RuntimeAgent.evaluate("loadSubresource('resources/stylesheet.css?baseline')"),

NIT: `InspectorTest.evaluateInPage`?

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:51
> +                RuntimeAgent.evaluate("loadSubresource('resources/stylesheet.css?request')"),

Ditto (:37)

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:55
> +        }

NIT: I don't think it matters, but should we continue the interception so that it's not just left hanging?

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:59
> +        name: "Network.responseIntercepted.resourceType",

NIT: we usually prefix the test case name with the suite name (though I get what you're trying to go for here, so perhaps just change the suite name to be more general instead of specific to "request" such as "Network.InterceptionPreservesType")

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:65
> +                RuntimeAgent.evaluate("loadSubresource('resources/stylesheet.css?response')"),

Ditto (:37)

> LayoutTests/http/tests/inspector/network/interception-preserves-type.html:69
> +        }

Ditto (:55)

[Blaze Burg]

Comment on attachment 402359 [details]
Patch

Clearing r? as there is unaddressed review feedback and the patch is now stale.