Bug 177641 – Web Inspector: Include Beacon and Ping requests in Network tab

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

[Joseph Pecoraro]

Include Beacon and Ping requests in Network tab.

These go down a different loader path then normal page subresources, so we will have to do our own inspector instrumentation hooks for these requests.

[Joseph Pecoraro]

<rdar://problem/33086839>

[Joseph Pecoraro]

Created attachment 322173 [details]
[IMAGE] Beacon Icon - Small

[Joseph Pecoraro]

Created attachment 322174 [details]
[IMAGE] Beacon Icon - Large

[Joseph Pecoraro]

Created attachment 322175 [details]
[IMAGE] Beacon Icon - Small (Alternative)

[Joseph Pecoraro]

Created attachment 322176 [details]
[IMAGE] Beacon Icon - Large (Alternative)

[Joseph Pecoraro]

Created attachment 322177 [details]
[IMAGE] Beacon in Network Table

[Joseph Pecoraro]

Created attachment 322178 [details]
[IMAGE] Beacon Response

[Joseph Pecoraro]

Created attachment 322179 [details]
[IMAGE] Beacon Request

[Joseph Pecoraro]

Patches to come tomorrow because I just rebased and there are conflicts. Sorry!

[Joseph Pecoraro]

Test page I used for the images:
https://tests.caniuse.com/?feat=beacon

[Matt Baker]

These icons all look pretty great. I like the first version best, although I have to admit to liking the alternative (w/ background) version more than I expected.

[Joseph Pecoraro]

(In reply to Matt Baker from comment #11)
> These icons all look pretty great. I like the first version best, although I
> have to admit to liking the alternative (w/ background) version more than I
> expected.

I like the first version best as well and that is what I will be sending patches out for. But I'll still keep the document style icon, because it is SVG based unlike the other PNGs so might be something we will want to use in the future.

[Joseph Pecoraro]

Created attachment 322233 [details]
[PATCH] Proposed Fix

[Build Bot]

Attachment 322233 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/PingLoader.cpp:43:  "InspectorInstrumentation.h" already included at Source/WebCore/loader/PingLoader.cpp:42  [build/include] [4]
ERROR: LayoutTests/platform/mac-wk2/TestExpectations:728:  Path does not exist.  [test/expectations] [5]
ERROR: LayoutTests/platform/win/TestExpectations:3684:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/loader/cache/CachedResource.cpp:278:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/mac-wk1/TestExpectations:376:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/mac-wk2/TestExpectations:728:  Path does not exist.  [test/expectations] [5]
ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/win/TestExpectations:3684:  Path does not exist.  [test/expectations] [5]
Total errors found: 7 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 322234 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 322236 [details]
[PATCH] Proposed Fix

Address GTK / WPE build issues.

[Joseph Pecoraro]

Created attachment 322245 [details]
[PATCH] Proposed Fix

Addressed a last style issue. Looks like this is good for review!

[Devin Rousso]

Comment on attachment 322245 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/loader/PingLoader.cpp:208
> +    platformStrategies()->loaderStrategy()->startPingLoad(frame, request, WTFMove(originalRequestHeaders), options, [framePtr = WTFMove(framePtr), identifier = identifier] (const ResourceError& error, const ResourceResponse& response) {

Is there no way to simplify the `identifier = identifier`?

> Source/WebCore/loader/PingLoader.cpp:210
> +            InspectorInstrumentation::didReceiveResourceResponse(*framePtr.get(), identifier, framePtr->loader().activeDocumentLoader(), response, nullptr);

Would just `*framePtr` not suffice?

> Source/WebCore/loader/cache/CachedResource.cpp:280
> +            platformStrategies()->loaderStrategy()->startPingLoad(frame, request, *m_originalRequestHeaders, m_options, [this, protectedThis = WTFMove(protectedThis), framePtr = framePtr, identifier = identifier] (const ResourceError& error, const ResourceResponse& response) {

You `WTFMove(framePtr)` in PingLoader.cpp.  Is there a reason you don't here?

Also, you don't use `protectedThis`.  Is it necessary?

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:505
> +            if (type === "Document" && frame.mainResource.url === url && frame.loaderIdentifier === loaderIdentifier)

Can you compare to `PageAgent.ResourceType.Document` instead?

> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:507
> +            else if (type === "Document" && frame.provisionalMainResource && frame.provisionalMainResource.url === url && frame.provisionalLoaderIdentifier === loaderIdentifier)

Ditto (505).

[Chris Dumez]

Comment on attachment 322245 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:379
> +    if (CachedResource* cachedResource = InspectorPageAgent::cachedResource(loader.frame(), request.url()))

auto*

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:388
> +    if (loadType == InspectorInstrumentation::LoadType::Ping)

seems like we could use a switch statement without default: to easily catch if somebody adds a new enum value.

>> Source/WebCore/loader/PingLoader.cpp:208
>> +    platformStrategies()->loaderStrategy()->startPingLoad(frame, request, WTFMove(originalRequestHeaders), options, [framePtr = WTFMove(framePtr), identifier = identifier] (const ResourceError& error, const ResourceResponse& response) {
> 
> Is there no way to simplify the `identifier = identifier`?

identifier would be enough.

also [protectedFrame = makeRef(frame)]

>> Source/WebCore/loader/PingLoader.cpp:210
>> +            InspectorInstrumentation::didReceiveResourceResponse(*framePtr.get(), identifier, framePtr->loader().activeDocumentLoader(), response, nullptr);
> 
> Would just `*framePtr` not suffice?

Just protectedFrame if you include my change above.

> Source/WebCore/loader/PingLoader.cpp:213
> +            InspectorInstrumentation::didFinishLoading(framePtr.get(), framePtr->loader().activeDocumentLoader(), identifier, emptyMetrics, nullptr);

protectedFrame.ptr()

> Source/WebCore/loader/PingLoader.cpp:215
> +            InspectorInstrumentation::didFailLoading(framePtr.get(), framePtr->loader().activeDocumentLoader(), identifier, error);

protectedFrame.ptr()

>> Source/WebCore/loader/cache/CachedResource.cpp:280
>> +            platformStrategies()->loaderStrategy()->startPingLoad(frame, request, *m_originalRequestHeaders, m_options, [this, protectedThis = WTFMove(protectedThis), framePtr = framePtr, identifier = identifier] (const ResourceError& error, const ResourceResponse& response) {
> 
> You `WTFMove(framePtr)` in PingLoader.cpp.  Is there a reason you don't here?
> 
> Also, you don't use `protectedThis`.  Is it necessary?

Same comments as above.

[Build Bot]

Comment on attachment 322245 [details]
[PATCH] Proposed Fix

Attachment 322245 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4702466

New failing tests:
http/tests/inspector/network/ping-type.html

[Build Bot]

Created attachment 322251 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Comment on attachment 322245 [details]
[PATCH] Proposed Fix

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

I'll try to figure out what happened on the bot.

>>> Source/WebCore/loader/PingLoader.cpp:208
>>> +    platformStrategies()->loaderStrategy()->startPingLoad(frame, request, WTFMove(originalRequestHeaders), options, [framePtr = WTFMove(framePtr), identifier = identifier] (const ResourceError& error, const ResourceResponse& response) {
>> 
>> Is there no way to simplify the `identifier = identifier`?
> 
> identifier would be enough.
> 
> also [protectedFrame = makeRef(frame)]

Excellent! I saw code that looked like and forgot to go back to using it.

>>> Source/WebCore/loader/cache/CachedResource.cpp:280
>>> +            platformStrategies()->loaderStrategy()->startPingLoad(frame, request, *m_originalRequestHeaders, m_options, [this, protectedThis = WTFMove(protectedThis), framePtr = framePtr, identifier = identifier] (const ResourceError& error, const ResourceResponse& response) {
>> 
>> You `WTFMove(framePtr)` in PingLoader.cpp.  Is there a reason you don't here?
>> 
>> Also, you don't use `protectedThis`.  Is it necessary?
> 
> Same comments as above.

protectedThis (not added by my patch) is necessary to keep `this` alive since it is used in the lambda called at some later time (`this->error`).

>> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:505
>> +            if (type === "Document" && frame.mainResource.url === url && frame.loaderIdentifier === loaderIdentifier)
> 
> Can you compare to `PageAgent.ResourceType.Document` instead?

Sure. I think ultimately we want this code to go away and instead use Page.frameNavigated / frameScheduledLoad and all those.

[Joseph Pecoraro]

Created attachment 322258 [details]
[PATCH] Proposed Fix

If ElCap fails, I'll put up a debug patch to see where the ElCap (non-network session PingHandle.h impl) is completing the ping load. I removed didReceiveBuffer since maybe that happens before didReceiveResponse and we don't really need to complete it early.

[Build Bot]

Comment on attachment 322258 [details]
[PATCH] Proposed Fix

Attachment 322258 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4703738

New failing tests:
http/tests/inspector/network/ping-type.html

[Build Bot]

Created attachment 322260 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 322265 [details]
[PATCH] For Bots - Backtrace from ElCapitan

[Joseph Pecoraro]

Windows:
> c:\cygwin\home\buildbot\webkit\source\webcore\inspector\inspectornetworkagent.cpp(391): warning C4715: 'WebCore::resourceTypeForLoadType': not all control paths return a value 

Ugh.

[Build Bot]

Comment on attachment 322265 [details]
[PATCH] For Bots - Backtrace from ElCapitan

Attachment 322265 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4704484

New failing tests:
http/tests/inspector/network/ping-type.html

[Build Bot]

Created attachment 322269 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Latest patch with debugging shows:

> pingLoadComplete
> 1   0x10b47e843 WebCore::PingHandle::pingLoadComplete(WebCore::ResourceError const&, WebCore::ResourceResponse const&)
> 2   0x10b47e15a WebCore::PingHandle::didFinishLoading(WebCore::ResourceHandle*)
> 3   0x7fff9532c93d _dispatch_call_block_and_release
> 4   0x7fff9532140b _dispatch_client_callout
> 5   0x7fff95334c1c _dispatch_main_queue_callback_4CF
> 6   0x7fff98192949 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
> 7   0x7fff9815182d __CFRunLoopRun
> 8   0x7fff98150e28 CFRunLoopRunSpecific
> 9   0x7fff911b8f99 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
> 10  0x7fff911b8e78 -[NSRunLoop(NSRunLoop) run]
> 11  0x7fff8e1f2168 _xpc_objc_main
> 12  0x7fff8e1f0bbe xpc_main
> 13  0x10b3ad702 main
> 14  0x7fff9c56a5ad start
> 15  0x1

So it looks like didFinishLoading happens before didReceiveResponse on El Capitan's legacy loading path. I'll try to include El Cap specific results, otherwise skip on El Cap.

[Joseph Pecoraro]

Created attachment 322272 [details]
[PATCH] Proposed Fix

[Build Bot]

Comment on attachment 322272 [details]
[PATCH] Proposed Fix

Attachment 322272 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4705084

New failing tests:
http/tests/inspector/network/ping-type.html

[Build Bot]

Created attachment 322275 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322272 [details]
[PATCH] Proposed Fix

Attachment 322272 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4705248

New failing tests:
http/tests/inspector/network/ping-type.html

[Build Bot]

Created attachment 322280 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Okay. I'll try skipping on El Cap. If it has the same problem on later OSes (I haven't reproduced) then I can allow NaN or #, given it is possible but I don't expect that behavior if the network layer produces delegates in the expected order.

[Sam Weinig]

This probably isn't specific to Beacon, but does the preview of the request differ based on what type is being sent? e.g. what body type - https://fetch.spec.whatwg.org/#bodyinit and / or content type?

[youenn fablet]

(In reply to Sam Weinig from comment #37)
> This probably isn't specific to Beacon, but does the preview of the request
> differ based on what type is being sent? e.g. what body type -
> https://fetch.spec.whatwg.org/#bodyinit and / or content type?

What use case do you have in mind?

I would think that inspector is getting a ResourceRequest which has almost no info on the body type (although it might be able to detect blob or form data)
body type is something internal to our fetch implementation really and a fetch request as per spec has no body type really.

The content-type should be available as a HTTP header.

[Sam Weinig]

(In reply to youenn fablet from comment #38)
> (In reply to Sam Weinig from comment #37)
> > This probably isn't specific to Beacon, but does the preview of the request
> > differ based on what type is being sent? e.g. what body type -
> > https://fetch.spec.whatwg.org/#bodyinit and / or content type?
> 
> What use case do you have in mind?
> 
> I would think that inspector is getting a ResourceRequest which has almost
> no info on the body type (although it might be able to detect blob or form
> data)
> body type is something internal to our fetch implementation really and a
> fetch request as per spec has no body type really.
> 
> The content-type should be available as a HTTP header.

Given that the API the developer calls to send a beacon takes a BodyInit, it seems somewhat reasonable that we could maintain some of that information for view by the inspector. The premise being, that how one displays an ArrayBuffer of data may be quite different than a file with a content-type, or an array of form data pairs.

[Sam Weinig]

(In reply to Sam Weinig from comment #39)
> (In reply to youenn fablet from comment #38)
> > (In reply to Sam Weinig from comment #37)
> > > This probably isn't specific to Beacon, but does the preview of the request
> > > differ based on what type is being sent? e.g. what body type -
> > > https://fetch.spec.whatwg.org/#bodyinit and / or content type?
> > 
> > What use case do you have in mind?
> > 
> > I would think that inspector is getting a ResourceRequest which has almost
> > no info on the body type (although it might be able to detect blob or form
> > data)
> > body type is something internal to our fetch implementation really and a
> > fetch request as per spec has no body type really.
> > 
> > The content-type should be available as a HTTP header.
> 
> Given that the API the developer calls to send a beacon takes a BodyInit, it
> seems somewhat reasonable that we could maintain some of that information
> for view by the inspector. The premise being, that how one displays an
> ArrayBuffer of data may be quite different than a file with a content-type,
> or an array of form data pairs.

The use case is just better presentation of the data.

[Maciej Stachowiak]

(In reply to Sam Weinig from comment #39)
> (In reply to youenn fablet from comment #38)
> > (In reply to Sam Weinig from comment #37)
> > > This probably isn't specific to Beacon, but does the preview of the request
> > > differ based on what type is being sent? e.g. what body type -
> > > https://fetch.spec.whatwg.org/#bodyinit and / or content type?
> > 
> > What use case do you have in mind?
> > 
> > I would think that inspector is getting a ResourceRequest which has almost
> > no info on the body type (although it might be able to detect blob or form
> > data)
> > body type is something internal to our fetch implementation really and a
> > fetch request as per spec has no body type really.
> > 
> > The content-type should be available as a HTTP header.
> 
> Given that the API the developer calls to send a beacon takes a BodyInit, it
> seems somewhat reasonable that we could maintain some of that information
> for view by the inspector. The premise being, that how one displays an
> ArrayBuffer of data may be quite different than a file with a content-type,
> or an array of form data pairs.

There's no way to view the request body in current Web Inspector. Your thought is an interesting suggestion for when we do. Not sure if there's a bugzilla bug for that but in Radar it is <rdar://problem/34071925>.

(It might still be better to preview request bodies based on request Content-type header, with a specific specialization for the content-type of form data, rather than based on the type of object passed to the API that initiated the load, but that's better discussed when/if we add the feature.)

[youenn fablet]

I would much prefer having better presentation based on the content-type.
I guess text/arrayBuffer specialization might be nice.

The fetch spec request has no persistent notion of body type.
The implementation does not always keep that information either (when cloning for instance) so I would not try to base things on it.

[Sam Weinig]

(In reply to Maciej Stachowiak from comment #41)
> (In reply to Sam Weinig from comment #39)
> > (In reply to youenn fablet from comment #38)
> > > (In reply to Sam Weinig from comment #37)
> > > > This probably isn't specific to Beacon, but does the preview of the request
> > > > differ based on what type is being sent? e.g. what body type -
> > > > https://fetch.spec.whatwg.org/#bodyinit and / or content type?
> > > 
> > > What use case do you have in mind?
> > > 
> > > I would think that inspector is getting a ResourceRequest which has almost
> > > no info on the body type (although it might be able to detect blob or form
> > > data)
> > > body type is something internal to our fetch implementation really and a
> > > fetch request as per spec has no body type really.
> > > 
> > > The content-type should be available as a HTTP header.
> > 
> > Given that the API the developer calls to send a beacon takes a BodyInit, it
> > seems somewhat reasonable that we could maintain some of that information
> > for view by the inspector. The premise being, that how one displays an
> > ArrayBuffer of data may be quite different than a file with a content-type,
> > or an array of form data pairs.
> 
> There's no way to view the request body in current Web Inspector. Your
> thought is an interesting suggestion for when we do. Not sure if there's a
> bugzilla bug for that but in Radar it is <rdar://problem/34071925>.
> 
> (It might still be better to preview request bodies based on request
> Content-type header, with a specific specialization for the content-type of
> form data, rather than based on the type of object passed to the API that
> initiated the load, but that's better discussed when/if we add the feature.)

I was commenting based on the the attachment with the title "[IMAGE] Beacon Request", which has a "Preview" pane.

[Sam Weinig]

(In reply to youenn fablet from comment #42)
> I would much prefer having better presentation based on the content-type.
> I guess text/arrayBuffer specialization might be nice.
> 
> The fetch spec request has no persistent notion of body type.
> The implementation does not always keep that information either (when
> cloning for instance) so I would not try to base things on it.

Just because Fetch doesn't maintain the information, doesn't mean we can't. Many things in the inspector are looking at more detailed representations than the specs might have.

[Joseph Pecoraro]

(In reply to Sam Weinig from comment #37)
> This probably isn't specific to Beacon, but does the preview of the request
> differ based on what type is being sent? e.g. what body type -
> https://fetch.spec.whatwg.org/#bodyinit and / or content type?
>
> --
>
> Given that the API the developer calls to send a beacon takes a BodyInit, it
> seems somewhat reasonable that we could maintain some of that information
> for view by the inspector. The premise being, that how one displays an
> ArrayBuffer of data may be quite different than a file with a content-type,
> or an array of form data pairs.

Correct this isn't something specific to Beacon. Web Inspector currently defaults to text for many things that may not be text (such as XHR/Fetch response content and any request bodies). That is something I would very much like to improve.

There are existing bugs about displaying content better for non-text data:
<https://webkit.org/b/177404> Web Inspector: Network tab doesn't show the actual data in multipart/form-data
<https://webkit.org/b/172742> Web Inspector: Image data renders as binary string in Network preview when loaded via XHR

We've discussed better presentations for data:

  • an object tree graph for JSON data (allow expanding / collapsing sections)
  • an image view if we determine the binary data is an image (if we have content type)
  • a generic hex view for binary data (many text editors have such a view)

I hadn't considered something for ArrayBuffer, thanks for pointing that out. Especially since this is initiated on the client side we have all the data that we would need to provide a better presentation.


(In reply to Maciej Stachowiak from comment #41)
> There's no way to view the request body in current Web Inspector. Your
> thought is an interesting suggestion for when we do. Not sure if there's a
> bugzilla bug for that but in Radar it is <rdar://problem/34071925>.

Web Inspector does show the request (POST) data. I also included a screenshot above of what it looks like in the new network tab in the attachments.

[Joseph Pecoraro]

Created attachment 322395 [details]
[PATCH] Proposed Fix

[Chris Dumez]

Comment on attachment 322395 [details]
[PATCH] Proposed Fix

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

r=me with suggestions.

> Source/WebCore/inspector/InspectorNetworkAgent.cpp:368
> +    Inspector::Protocol::Page::ResourceType protocolResourceType = InspectorPageAgent::resourceTypeJson(type);

auto ?

> Source/WebCore/inspector/InspectorPageAgent.cpp:278
> +InspectorPageAgent::ResourceType InspectorPageAgent::cachedResourceType(CachedResource::Type type)

Method is confusingly named since it does not return a CachedResource::Type? Maybe it should be named inspectorResourceType() ?

> Source/WebCore/loader/LoaderStrategy.h:68
> +    using PingLoadCompletionHandler = WTF::Function<void(const ResourceError&, const ResourceResponse&)>;

The parameters are mutually exclusive it seems. This should probably be a WTF::Expected<ResourceRespnse, ResourceError> instead.

> Source/WebKit/NetworkProcess/PingLoad.cpp:80
> +void PingLoad::didFinish(const ResourceError& error, const ResourceResponse& response)

The parameters are mutually exclusive it seems. This should probably be a WTF::Expected<ResourceRespnse, ResourceError> instead.

> Source/WebKit/NetworkProcess/PingLoad.h:85
> +    WTF::CompletionHandler<void(const WebCore::ResourceError&, const WebCore::ResourceResponse&)> m_completionHandler;

The parameters are mutually exclusive it seems. This should probably be a WTF::Expected<ResourceRespnse, ResourceError> instead.

[Joseph Pecoraro]

Comment on attachment 322395 [details]
[PATCH] Proposed Fix

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

>> Source/WebCore/inspector/InspectorNetworkAgent.cpp:368
>> +    Inspector::Protocol::Page::ResourceType protocolResourceType = InspectorPageAgent::resourceTypeJson(type);
> 
> auto ?

I'm about to rename resourceTypeJson to resourceTypeJSON in a follow-up, so I'll do this with that.

>> Source/WebCore/inspector/InspectorPageAgent.cpp:278
>> +InspectorPageAgent::ResourceType InspectorPageAgent::cachedResourceType(CachedResource::Type type)
> 
> Method is confusingly named since it does not return a CachedResource::Type? Maybe it should be named inspectorResourceType() ?

I agree, I'll make this rename.

>> Source/WebCore/loader/LoaderStrategy.h:68
>> +    using PingLoadCompletionHandler = WTF::Function<void(const ResourceError&, const ResourceResponse&)>;
> 
> The parameters are mutually exclusive it seems. This should probably be a WTF::Expected<ResourceRespnse, ResourceError> instead.

Correct, in practice they are exclusive but they don't actually need to be. The PingLoaders could be extended to support getting both a response and continue to load to see if the full completes successfully or fails. I'm going to leave this as is. I think Ideally we move these loads to the common loader path and these comments just go ago.

[Joseph Pecoraro]

> go ago.

Err: go *away*.

[Joseph Pecoraro]

<https://trac.webkit.org/r222739>