Bug 177641

<rdar://problem/33086839>

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

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

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

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

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

Created attachment 322178 [details] [IMAGE] Beacon Response

Created attachment 322179 [details] [IMAGE] Beacon Request

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

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

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.

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

Created attachment 322233 [details] [PATCH] Proposed Fix

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.

Created attachment 322234 [details] [PATCH] Proposed Fix

Created attachment 322236 [details] [PATCH] Proposed Fix Address GTK / WPE build issues.

Created attachment 322245 [details] [PATCH] Proposed Fix Addressed a last style issue. Looks like this is good for review!

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

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

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

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

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

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

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

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.

Created attachment 322272 [details] [PATCH] Proposed Fix

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

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

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

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.

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?

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

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

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

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

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

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

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

Created attachment 322395 [details] [PATCH] Proposed Fix

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

> go ago. Err: go *away*.

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