Bug 175280 - Update sendBeacon() to rely on FetchBody instead of the whole FetchRequest
Summary: Update sendBeacon() to rely on FetchBody instead of the whole FetchRequest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://w3c.github.io/beacon/#sec-pro...
Keywords: InRadar
Depends on:
Blocks: 175264
  Show dependency treegraph
 
Reported: 2017-08-07 13:00 PDT by Chris Dumez
Modified: 2017-08-07 16:23 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.24 KB, patch)
2017-08-07 13:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.16 KB, patch)
2017-08-07 14:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1005.25 KB, application/zip)
2017-08-07 15:27 PDT, Build Bot
no flags Details
Patch (3.21 KB, patch)
2017-08-07 15:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-07 13:00:50 PDT
Update sendBeacon() to rely on FetchBody instead of the whole FetchRequest. FetchBody for data destruction is really the only thing we need at the moment.
The new code also properly sets the CORS mode, which will be needed for Bug 175264.
Comment 1 Chris Dumez 2017-08-07 13:03:31 PDT
Created attachment 317454 [details]
Patch
Comment 2 youenn fablet 2017-08-07 14:11:26 PDT
Comment on attachment 317454 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        for data destruction is really the only thing we need at the moment.

data extraction?

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:35
>  #include <runtime/JSCJSValue.h>

Do you still need that include?

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:64
> +    options.redirect = FetchOptions::Redirect::Follow;

No need to set redirect to follow, this is the default and is not spelled out in the spec explicitly.

> Source/WebCore/Modules/beacon/NavigatorBeacon.cpp:65
> +    options.cache = FetchOptions::Cache::NoCache;

I do not see that option set in the spec.
I forgot to ask why it was set in previous patches.
Comment 3 Chris Dumez 2017-08-07 14:17:56 PDT
Created attachment 317463 [details]
Patch
Comment 4 Build Bot 2017-08-07 15:27:30 PDT
Comment on attachment 317463 [details]
Patch

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

New failing tests:
http/tests/blink/sendbeacon/beacon-same-origin.html
Comment 5 Build Bot 2017-08-07 15:27:31 PDT
Created attachment 317480 [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 6 Chris Dumez 2017-08-07 15:30:03 PDT
Created attachment 317482 [details]
Patch
Comment 7 WebKit Commit Bot 2017-08-07 16:22:15 PDT
Comment on attachment 317482 [details]
Patch

Clearing flags on attachment: 317482

Committed r220366: <http://trac.webkit.org/changeset/220366>
Comment 8 WebKit Commit Bot 2017-08-07 16:22:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-08-07 16:23:18 PDT
<rdar://problem/33764771>