[webkit-reviews] review granted: [Bug 175007] Add initial support for navigator.sendBeacon : [Attachment 316903] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 175007: Add initial support for navigator.sendBeacon
https://bugs.webkit.org/show_bug.cgi?id=175007

Attachment 316903: Patch

https://bugs.webkit.org/attachment.cgi?id=316903&action=review




--- Comment #16 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 316903
  --> https://bugs.webkit.org/attachment.cgi?id=316903
Patch

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

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:44
> +	   return Exception { TypeError, ASCIILiteral("Beacons can only be sent
over HTTP(S)") };

Do we use the "HTTP(S)" in other exception text, or anywhere else?

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:47
> +    // We do not allow sending beacons from detached documents.

This should either explain why or be removed.

> Source/WebCore/loader/PingLoader.cpp:272
> +    startPingLoad(frame, request, ShouldFollowRedirects::Yes);

What happens to a ping load that wants to follow redirects when the page is
closed? Does the NetworkProcess handle this without consulting the WebProcess?
What does that mean for content extensions that would like to block some of
those redirects?

> Source/WebCore/loader/PingLoader.h:62
> +using BodyInit = Variant<RefPtr<Blob>, RefPtr<JSC::ArrayBufferView>,
RefPtr<JSC::ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>,
String>;

At some point we should put this in a header.  You can use FetchBody::Init,
which should be the same.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:349
> +    m_testRunner->setBeaconAPIEnabled(true);

Why not enable this in the normal preferences? Why use the weird ones in the
bundle?