Bug 177641 - Web Inspector: Include Beacon and Ping requests in Network tab
Summary: Web Inspector: Include Beacon and Ping requests in Network tab
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 178427
  Show dependency treegraph
 
Reported: 2017-09-28 22:21 PDT by Joseph Pecoraro
Modified: 2017-10-17 23:12 PDT (History)
19 users (show)

See Also:


Attachments
[IMAGE] Beacon Icon - Small (44.78 KB, image/png)
2017-09-28 22:49 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Beacon Icon - Large (101.10 KB, image/png)
2017-09-28 22:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Beacon Icon - Small (Alternative) (46.79 KB, image/png)
2017-09-28 22:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Beacon Icon - Large (Alternative) (104.13 KB, image/png)
2017-09-28 22:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Beacon in Network Table (156.21 KB, image/png)
2017-09-28 22:50 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Beacon Response (98.90 KB, image/png)
2017-09-28 22:51 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Beacon Request (98.39 KB, image/png)
2017-09-28 22:51 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (70.59 KB, patch)
2017-09-29 14:30 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (70.54 KB, patch)
2017-09-29 14:36 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (70.78 KB, patch)
2017-09-29 14:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (71.07 KB, patch)
2017-09-29 15:19 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.61 MB, application/zip)
2017-09-29 16:24 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (72.29 KB, patch)
2017-09-29 17:07 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.86 MB, application/zip)
2017-09-29 18:14 PDT, Build Bot
no flags Details
[PATCH] For Bots - Backtrace from ElCapitan (72.53 KB, patch)
2017-09-29 19:18 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.43 MB, application/zip)
2017-09-29 20:09 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (73.60 KB, patch)
2017-09-29 20:29 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.09 MB, application/zip)
2017-09-29 21:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.77 MB, application/zip)
2017-09-29 21:58 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (73.02 KB, patch)
2017-10-02 11:12 PDT, Joseph Pecoraro
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-09-28 22:21:15 PDT
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.
Comment 1 Joseph Pecoraro 2017-09-28 22:21:42 PDT
<rdar://problem/33086839>
Comment 2 Joseph Pecoraro 2017-09-28 22:49:49 PDT
Created attachment 322173 [details]
[IMAGE] Beacon Icon - Small
Comment 3 Joseph Pecoraro 2017-09-28 22:50:05 PDT
Created attachment 322174 [details]
[IMAGE] Beacon Icon - Large
Comment 4 Joseph Pecoraro 2017-09-28 22:50:26 PDT
Created attachment 322175 [details]
[IMAGE] Beacon Icon - Small (Alternative)
Comment 5 Joseph Pecoraro 2017-09-28 22:50:41 PDT
Created attachment 322176 [details]
[IMAGE] Beacon Icon - Large (Alternative)
Comment 6 Joseph Pecoraro 2017-09-28 22:50:54 PDT
Created attachment 322177 [details]
[IMAGE] Beacon in Network Table
Comment 7 Joseph Pecoraro 2017-09-28 22:51:09 PDT
Created attachment 322178 [details]
[IMAGE] Beacon Response
Comment 8 Joseph Pecoraro 2017-09-28 22:51:21 PDT
Created attachment 322179 [details]
[IMAGE] Beacon Request
Comment 9 Joseph Pecoraro 2017-09-28 22:51:42 PDT
Patches to come tomorrow because I just rebased and there are conflicts. Sorry!
Comment 10 Joseph Pecoraro 2017-09-28 22:52:29 PDT
Test page I used for the images:
https://tests.caniuse.com/?feat=beacon
Comment 11 Matt Baker 2017-09-29 12:20:59 PDT
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.
Comment 12 Joseph Pecoraro 2017-09-29 13:58:22 PDT
(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.
Comment 13 Joseph Pecoraro 2017-09-29 14:30:26 PDT
Created attachment 322233 [details]
[PATCH] Proposed Fix
Comment 14 Build Bot 2017-09-29 14:32:57 PDT
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.
Comment 15 Joseph Pecoraro 2017-09-29 14:36:34 PDT
Created attachment 322234 [details]
[PATCH] Proposed Fix
Comment 16 Joseph Pecoraro 2017-09-29 14:46:42 PDT
Created attachment 322236 [details]
[PATCH] Proposed Fix

Address GTK / WPE build issues.
Comment 17 Joseph Pecoraro 2017-09-29 15:19:04 PDT
Created attachment 322245 [details]
[PATCH] Proposed Fix

Addressed a last style issue. Looks like this is good for review!
Comment 18 Devin Rousso 2017-09-29 15:19:58 PDT
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).
Comment 19 Chris Dumez 2017-09-29 15:29:42 PDT
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.
Comment 20 Build Bot 2017-09-29 16:24:25 PDT
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
Comment 21 Build Bot 2017-09-29 16:24:26 PDT
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
Comment 22 Joseph Pecoraro 2017-09-29 16:50:54 PDT
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.
Comment 23 Joseph Pecoraro 2017-09-29 17:07:41 PDT
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.
Comment 24 Build Bot 2017-09-29 18:14:14 PDT
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
Comment 25 Build Bot 2017-09-29 18:14:16 PDT
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
Comment 26 Joseph Pecoraro 2017-09-29 19:18:42 PDT
Created attachment 322265 [details]
[PATCH] For Bots - Backtrace from ElCapitan
Comment 27 Joseph Pecoraro 2017-09-29 20:00:29 PDT
Windows:
> c:\cygwin\home\buildbot\webkit\source\webcore\inspector\inspectornetworkagent.cpp(391): warning C4715: 'WebCore::resourceTypeForLoadType': not all control paths return a value 

Ugh.
Comment 28 Build Bot 2017-09-29 20:09:44 PDT
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
Comment 29 Build Bot 2017-09-29 20:09:47 PDT
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
Comment 30 Joseph Pecoraro 2017-09-29 20:23:06 PDT
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.
Comment 31 Joseph Pecoraro 2017-09-29 20:29:52 PDT
Created attachment 322272 [details]
[PATCH] Proposed Fix
Comment 32 Build Bot 2017-09-29 21:15:19 PDT
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
Comment 33 Build Bot 2017-09-29 21:15:21 PDT
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
Comment 34 Build Bot 2017-09-29 21:58:18 PDT
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
Comment 35 Build Bot 2017-09-29 21:58:20 PDT
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
Comment 36 Joseph Pecoraro 2017-09-30 00:09:23 PDT
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.
Comment 37 Sam Weinig 2017-10-01 12:13:46 PDT
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?
Comment 38 youenn fablet 2017-10-01 15:37:00 PDT
(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.
Comment 39 Sam Weinig 2017-10-01 17:00:21 PDT
(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.
Comment 40 Sam Weinig 2017-10-01 17:00:42 PDT
(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.
Comment 41 Maciej Stachowiak 2017-10-02 01:05:42 PDT
(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.)
Comment 42 youenn fablet 2017-10-02 01:26:31 PDT
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.
Comment 43 Sam Weinig 2017-10-02 09:29:17 PDT
(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.
Comment 44 Sam Weinig 2017-10-02 09:30:26 PDT
(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.
Comment 45 Joseph Pecoraro 2017-10-02 10:59:54 PDT
(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.
Comment 46 Joseph Pecoraro 2017-10-02 11:12:44 PDT
Created attachment 322395 [details]
[PATCH] Proposed Fix
Comment 47 Chris Dumez 2017-10-02 11:42:31 PDT
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.
Comment 48 Joseph Pecoraro 2017-10-02 12:15:46 PDT
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.
Comment 49 Joseph Pecoraro 2017-10-02 12:16:31 PDT
> go ago.

Err: go *away*.
Comment 50 Joseph Pecoraro 2017-10-02 13:24:39 PDT
<https://trac.webkit.org/r222739>